[PATCH] Storing the ACL control flags with vfs gpfs.
Christof Schmitt
cs at samba.org
Fri Mar 21 16:07:44 MDT 2014
On Fri, Mar 21, 2014 at 04:47:39PM +0100, Alexander Werth wrote:
> On Wed, 2014-03-19 at 15:10 -0700, Christof Schmitt wrote:
> > Should we use uint16_t throughout these patches to move the code towards
> > the C99 types? It is different from the existing code in the file, but
> > changing it has to start at some point.
>
> Ok, changed. Opening the stage for clean-up patches.
Feel free to send cleanup patches :-)
>
> > 0002-vfs-Store-ACL-control-flags-in-gpfs-vfs-module.patch
> >
> > --- a/source3/modules/vfs_gpfs.c
> > +++ b/source3/modules/vfs_gpfs.c
> > ...
> > @@ -207,6 +225,34 @@ static int vfs_gpfs_get_real_filename(struct
> > vfs_handle_struct *handle,
> > return 0;
> > }
> >
> > +static void sd2gpfs_control(uint16_t control, struct gpfs_acl *gacl)
> > +{
> > + unsigned int gpfs_aclflags = 0;
> > + gpfs_aclflags = control << 8;
> > + gpfs_aclflags &= 0x3c3c00; /* ACL4_FLAG_DACL_PROTECTED |
> > + ACL4_FLAG_SACL_PROTECTED |
> > + ACL4_FLAG_DACL_AUTO_INHERITED |
> > + ACL4_FLAG_SACL_AUTO_INHERITED |
> > + ACL4_FLAG_DACL_DEFAULTED |
> > + ACL4_FLAG_SACL_DEFAULTED |
> > + ACL4_FLAG_DACL_PRESENT |
> > + ACL4_FLAG_SACL_PRESENT; */
> > + if (!(control & 0x00000400)) /* SEC_DESC_DACL_PRESENT */
> > + gpfs_aclflags |= 0x00800000; /* ACL4_FLAG_NULL_DACL; */
> > + if (!(control & 0x00001000)) /* SEC_DESC_SACL_PRESENT */
> > + gpfs_aclflags |= 0x01000000; /* ACL4_FLAG_NULL_SACL; */
> >
> > The if statements should use braces. Also the SEC_DESC_... defines are
> > from Samba, not GPFS, so they can be used without problems.
>
> True.
>
> > + gacl->acl_level = 1; /* GPFS_ACL_LEVEL_V4FLAGS*/
> > + /* gacl->v4Level1.acl_flags requires gpfs 3.5 */
> > + *(unsigned int *)&gacl->ace_v4 = gpfs_aclflags;
> > +}
> > +
> > +static uint16_t gpfs2sd_control(unsigned int gpfs_aclflags)
> > +{
> > + uint16_t control = gpfs_aclflags >> 8;
> > + control |= SEC_DESC_SELF_RELATIVE;
> > + return control;
> > +}
> >
> > It is probably unlikely that someone would be modifying the flags in the
> > filesystem without going through Samba, but should gpfs2sd_control use
> > the same mask as sd2gpfs_control for consistency? Maybe create a
> > static uint16_t gpfs_aclflags_mask = 0x3c3c00; and use it in both
> > functions?
>
> I found a different solution: Filter while we are dealing with the
> security descriptor where we have the defines instead of filtering the
> gpfs control flags where we need the literals.
That is a good approach. I would still prefer to define the mask once
to avoid repeating it in the code, but i leave that up to you.
>
> > Also the ACL4_FLAG_NULL_DACL and ACL4_FLAG_NULL_SACL mappings are
> > missing from gpfs2sd_control. Are they not needed here?
>
> Yes, they are not relevant at the moment. The code doesn't yet
> distinguish between a null dacl and an empty dacl. That's a future
> change that's possible through the stored control flags.
Ok.
The patches look good to me:
Reviewed-by: Christof Schmitt <cs at samba.org>
Can we get a second review?
> >From e24534475d25486761927025f59e8e39fa552474 Mon Sep 17 00:00:00 2001
> From: Alexander Werth <alexander.werth at de.ibm.com>
> Date: Mon, 20 Jan 2014 15:12:42 +0100
> Subject: [PATCH 1/2] vfs: Support NFS control flags in nfs4_acls.c.
>
> The ACL control flags stores in particular the dacl protected bit
> which is responsible for the "Include inherited permissions from
> this object's parent" checkbox. This stores the information in the
> ACL struct passed to and from file system specific vfs modules.
>
> Signed-off-by: Alexander Werth <alexander.werth at de.ibm.com>
> ---
> source3/modules/nfs4_acls.c | 24 +++++++++++++++++++++++-
> source3/modules/nfs4_acls.h | 4 ++++
> 2 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/source3/modules/nfs4_acls.c b/source3/modules/nfs4_acls.c
> index 774c40e..3edb321 100644
> --- a/source3/modules/nfs4_acls.c
> +++ b/source3/modules/nfs4_acls.c
> @@ -49,6 +49,7 @@ typedef struct _SMB_ACE4_INT_T
> typedef struct _SMB_ACL4_INT_T
> {
> uint32 magic;
> + uint16_t controlflags;
> uint32 naces;
> SMB_ACE4_INT_T *first;
> SMB_ACE4_INT_T *last;
> @@ -218,6 +219,7 @@ SMB4ACL_T *smb_create_smb4acl(TALLOC_CTX *mem_ctx)
> return NULL;
> }
> theacl->magic = SMB_ACL4_INT_MAGIC;
> + theacl->controlflags = SEC_DESC_SELF_RELATIVE;
> /* theacl->first, last = NULL not needed */
> return (SMB4ACL_T *)theacl;
> }
> @@ -288,6 +290,25 @@ uint32 smb_get_naces(SMB4ACL_T *theacl)
> return aclint->naces;
> }
>
> +uint16_t smb_get_controlflags(SMB4ACL_T *theacl)
> +{
> + SMB_ACL4_INT_T *aclint = get_validated_aclint(theacl);
> + if (aclint==NULL)
> + return 0;
> +
> + return aclint->controlflags;
> +}
> +
> +bool smb_set_controlflags(SMB4ACL_T *theacl, uint16_t controlflags)
> +{
> + SMB_ACL4_INT_T *aclint = get_validated_aclint(theacl);
> + if (aclint==NULL)
> + return false;
> +
> + aclint->controlflags = controlflags;
> + return true;
> +}
> +
> static int smbacl4_GetFileOwner(struct connection_struct *conn,
> const char *filename,
> SMB_STRUCT_STAT *psbuf)
> @@ -543,7 +564,7 @@ static NTSTATUS smb_get_nt_acl_nfs4_common(const SMB_STRUCT_STAT *sbuf,
>
> DEBUG(10,("after make sec_acl\n"));
> *ppdesc = make_sec_desc(
> - mem_ctx, SD_REVISION, SEC_DESC_SELF_RELATIVE,
> + mem_ctx, SD_REVISION, smb_get_controlflags(theacl),
> (security_info & SECINFO_OWNER) ? &sid_owner : NULL,
> (security_info & SECINFO_GROUP) ? &sid_group : NULL,
> NULL, psa, &sd_size);
> @@ -1028,6 +1049,7 @@ NTSTATUS smb_set_nt_acl_nfs4(vfs_handle_struct *handle, files_struct *fsp,
> return map_nt_error_from_unix(errno);
> }
>
> + smb_set_controlflags(theacl, psd->type);
> smbacl4_dump_nfs4acl(10, theacl);
>
> if (set_acl_as_root) {
> diff --git a/source3/modules/nfs4_acls.h b/source3/modules/nfs4_acls.h
> index 1bde81b..75bc2a0 100644
> --- a/source3/modules/nfs4_acls.h
> +++ b/source3/modules/nfs4_acls.h
> @@ -130,6 +130,10 @@ SMB4ACE_T *smb_next_ace4(SMB4ACE_T *ace);
>
> uint32 smb_get_naces(SMB4ACL_T *theacl);
>
> +uint16_t smb_get_controlflags(SMB4ACL_T *theacl);
> +
> +bool smb_set_controlflags(SMB4ACL_T *theacl, uint16_t controlflags);
> +
> NTSTATUS smb_fget_nt_acl_nfs4(files_struct *fsp,
> uint32 security_info,
> TALLOC_CTX *mem_ctx,
> --
> 1.8.3.2
>
> >From 062bcba004a395b3bb2d2a1b2e94472aa3d62995 Mon Sep 17 00:00:00 2001
> From: Alexander Werth <alexander.werth at de.ibm.com>
> Date: Tue, 4 Feb 2014 17:50:54 +0100
> Subject: [PATCH 2/2] vfs: Store ACL control flags in gpfs vfs module.
>
> Use literals to allow a compile and execution on gpfs 3.4.
>
> Signed-off-by: Alexander Werth <alexander.werth at de.ibm.com>
> ---
> source3/modules/vfs_gpfs.c | 137 +++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 114 insertions(+), 23 deletions(-)
>
> diff --git a/source3/modules/vfs_gpfs.c b/source3/modules/vfs_gpfs.c
> index 760d27f..0baa8ce 100644
> --- a/source3/modules/vfs_gpfs.c
> +++ b/source3/modules/vfs_gpfs.c
> @@ -51,6 +51,24 @@ struct gpfs_config_data {
> bool settimes;
> };
>
> +static inline unsigned int gpfs_acl_flags(gpfs_acl_t *gacl)
> +{
> + if (gacl->acl_level == 1) { /* GPFS_ACL_LEVEL_V4FLAGS */
> + /* gacl->v4Level1.acl_flags requires gpfs 3.5 */
> + return *(unsigned int *)&gacl->ace_v4;
> + }
> + return 0;
> +}
> +
> +static inline gpfs_ace_v4_t *gpfs_ace_ptr(gpfs_acl_t *gacl, unsigned int i)
> +{
> + if (gacl->acl_level == 1) { /* GPFS_ACL_LEVEL_V4FLAGS */
> + /* &gacl->v4Level1.ace_v4[i] requires gpfs 3.5 */
> + char *ptr = (char *)&gacl->ace_v4[i] + sizeof(unsigned int);
> + return (gpfs_ace_v4_t *)ptr;
> + }
> + return &gacl->ace_v4[i];
> +}
>
> static int vfs_gpfs_kernel_flock(vfs_handle_struct *handle, files_struct *fsp,
> uint32 share_mode, uint32 access_mask)
> @@ -207,6 +225,34 @@ static int vfs_gpfs_get_real_filename(struct vfs_handle_struct *handle,
> return 0;
> }
>
> +static void sd2gpfs_control(uint16_t control, struct gpfs_acl *gacl)
> +{
> + unsigned int gpfs_aclflags = 0;
> + control &= SEC_DESC_DACL_PROTECTED | SEC_DESC_SACL_PROTECTED |
> + SEC_DESC_DACL_AUTO_INHERITED | SEC_DESC_SACL_AUTO_INHERITED |
> + SEC_DESC_DACL_DEFAULTED | SEC_DESC_SACL_DEFAULTED |
> + SEC_DESC_DACL_PRESENT | SEC_DESC_SACL_PRESENT;
> + gpfs_aclflags = control << 8;
> + if (!(control & SEC_DESC_DACL_PRESENT))
> + gpfs_aclflags |= 0x00800000; /* ACL4_FLAG_NULL_DACL; */
> + if (!(control & SEC_DESC_SACL_PRESENT))
> + gpfs_aclflags |= 0x01000000; /* ACL4_FLAG_NULL_SACL; */
> + gacl->acl_level = 1; /* GPFS_ACL_LEVEL_V4FLAGS*/
> + /* gacl->v4Level1.acl_flags requires gpfs 3.5 */
> + *(unsigned int *)&gacl->ace_v4 = gpfs_aclflags;
> +}
> +
> +static uint16_t gpfs2sd_control(unsigned int gpfs_aclflags)
> +{
> + uint16_t control = gpfs_aclflags >> 8;
> + control &= SEC_DESC_DACL_PROTECTED | SEC_DESC_SACL_PROTECTED |
> + SEC_DESC_DACL_AUTO_INHERITED | SEC_DESC_SACL_AUTO_INHERITED |
> + SEC_DESC_DACL_DEFAULTED | SEC_DESC_SACL_DEFAULTED |
> + SEC_DESC_DACL_PRESENT | SEC_DESC_SACL_PRESENT;
> + control |= SEC_DESC_SELF_RELATIVE;
> + return control;
> +}
> +
> static void gpfs_dumpacl(int level, struct gpfs_acl *gacl)
> {
> gpfs_aclCount_t i;
> @@ -216,14 +262,18 @@ static void gpfs_dumpacl(int level, struct gpfs_acl *gacl)
> return;
> }
>
> - DEBUG(level, ("gpfs acl: nace: %d, type:%d, version:%d, level:%d, len:%d\n",
> - gacl->acl_nace, gacl->acl_type, gacl->acl_version, gacl->acl_level, gacl->acl_len));
> + DEBUG(level, ("len: %d, level: %d, version: %d, nace: %d, "
> + "control: %x\n",
> + gacl->acl_len, gacl->acl_level, gacl->acl_version,
> + gacl->acl_nace, gpfs_acl_flags(gacl)));
> +
> for(i=0; i<gacl->acl_nace; i++)
> {
> - struct gpfs_ace_v4 *gace = gacl->ace_v4 + i;
> - DEBUG(level, ("\tace[%d]: type:%d, flags:0x%x, mask:0x%x, iflags:0x%x, who:%u\n",
> - i, gace->aceType, gace->aceFlags, gace->aceMask,
> - gace->aceIFlags, gace->aceWho));
> + struct gpfs_ace_v4 *gace = gpfs_ace_ptr(gacl, i);
> + DEBUG(level, ("\tace[%d]: type:%d, flags:0x%x, mask:0x%x, "
> + "iflags:0x%x, who:%u\n",
> + i, gace->aceType, gace->aceFlags, gace->aceMask,
> + gace->aceIFlags, gace->aceWho));
> }
> }
>
> @@ -265,9 +315,11 @@ again:
> } else {
> struct gpfs_acl *buf = (struct gpfs_acl *) aclbuf;
> buf->acl_type = type;
> + buf->acl_level = 1; /* GPFS_ACL_LEVEL_V4FLAGS */
> flags = GPFS_GETACL_STRUCT;
> len = &(buf->acl_len);
> - struct_size = sizeof(struct gpfs_acl);
> + /* reserve space for control flags in gpfs 3.5 and beyond */
> + struct_size = sizeof(struct gpfs_acl) + sizeof(unsigned int);
> }
>
> /* set the length of the buffer as input value */
> @@ -330,12 +382,17 @@ static int gpfs_get_nfs4_acl(TALLOC_CTX *mem_ctx, const char *fname, SMB4ACL_T *
>
> *ppacl = smb_create_smb4acl(mem_ctx);
>
> - DEBUG(10, ("len: %d, level: %d, version: %d, nace: %d\n",
> + if (gacl->acl_level == 1) { /* GPFS_ACL_LEVEL_V4FLAGS */
> + uint16_t control = gpfs2sd_control(gpfs_acl_flags(gacl));
> + smb_set_controlflags(*ppacl, control);
> + }
> +
> + DEBUG(10, ("len: %d, level: %d, version: %d, nace: %d, control: %x\n",
> gacl->acl_len, gacl->acl_level, gacl->acl_version,
> - gacl->acl_nace));
> + gacl->acl_nace, gpfs_acl_flags(gacl)));
>
> for (i=0; i<gacl->acl_nace; i++) {
> - struct gpfs_ace_v4 *gace = &gacl->ace_v4[i];
> + struct gpfs_ace_v4 *gace = gpfs_ace_ptr(gacl, i);
> SMB_ACE4PROP_T smbace;
> DEBUG(10, ("type: %d, iflags: %x, flags: %x, mask: %x, "
> "who: %d\n", gace->aceType, gace->aceIFlags,
> @@ -368,7 +425,7 @@ static int gpfs_get_nfs4_acl(TALLOC_CTX *mem_ctx, const char *fname, SMB4ACL_T *
>
> /* remove redundant deny entries */
> if (i > 0 && gace->aceType == SMB_ACE4_ACCESS_DENIED_ACE_TYPE) {
> - struct gpfs_ace_v4 *prev = &gacl->ace_v4[i-1];
> + struct gpfs_ace_v4 *prev = gpfs_ace_ptr(gacl, i - 1);
> if (prev->aceType == SMB_ACE4_ACCESS_ALLOWED_ACE_TYPE &&
> prev->aceFlags == gace->aceFlags &&
> prev->aceIFlags == gace->aceIFlags &&
> @@ -376,7 +433,7 @@ static int gpfs_get_nfs4_acl(TALLOC_CTX *mem_ctx, const char *fname, SMB4ACL_T *
> gace->aceWho == prev->aceWho) {
> /* it's redundant - skip it */
> continue;
> - }
> + }
> }
>
> smbace.aceType = gace->aceType;
> @@ -483,32 +540,37 @@ static NTSTATUS gpfsacl_get_nt_acl(vfs_handle_struct *handle,
> return map_nt_error_from_unix(errno);
> }
>
> -static bool gpfsacl_process_smbacl(vfs_handle_struct *handle, files_struct *fsp, SMB4ACL_T *smbacl)
> +static struct gpfs_acl *vfs_gpfs_smbacl2gpfsacl(TALLOC_CTX *mem_ctx,
> + files_struct *fsp,
> + SMB4ACL_T *smbacl,
> + bool controlflags)
> {
> - int ret;
> - gpfs_aclLen_t gacl_len;
> - SMB4ACE_T *smbace;
> struct gpfs_acl *gacl;
> - TALLOC_CTX *mem_ctx = talloc_tos();
> + gpfs_aclLen_t gacl_len;
> + SMB4ACE_T *smbace;
>
> - gacl_len = offsetof(gpfs_acl_t, ace_v4) + smb_get_naces(smbacl) *
> - sizeof(gpfs_ace_v4_t);
> + gacl_len = offsetof(gpfs_acl_t, ace_v4) + sizeof(unsigned int)
> + + smb_get_naces(smbacl) * sizeof(gpfs_ace_v4_t);
>
> gacl = (struct gpfs_acl *)TALLOC_SIZE(mem_ctx, gacl_len);
> if (gacl == NULL) {
> DEBUG(0, ("talloc failed\n"));
> errno = ENOMEM;
> - return False;
> + return NULL;
> }
>
> - gacl->acl_len = gacl_len;
> - gacl->acl_level = 0;
> + gacl->acl_level = 0; /* GPFS_ACL_LEVEL_BASE */
> gacl->acl_version = GPFS_ACL_VERSION_NFS4;
> gacl->acl_type = GPFS_ACL_TYPE_NFS4;
> gacl->acl_nace = 0; /* change later... */
>
> + if (controlflags) {
> + gacl->acl_level = 1; /* GPFS_ACL_LEVEL_V4FLAGS */
> + sd2gpfs_control(smb_get_controlflags(smbacl), gacl);
> + }
> +
> for (smbace=smb_first_ace4(smbacl); smbace!=NULL; smbace = smb_next_ace4(smbace)) {
> - struct gpfs_ace_v4 *gace = &gacl->ace_v4[gacl->acl_nace];
> + struct gpfs_ace_v4 *gace = gpfs_ace_ptr(gacl, gacl->acl_nace);
> SMB_ACE4PROP_T *aceprop = smb_get_ace4(smbace);
>
> gace->aceType = aceprop->aceType;
> @@ -566,9 +628,38 @@ static bool gpfsacl_process_smbacl(vfs_handle_struct *handle, files_struct *fsp,
>
> gacl->acl_nace++;
> }
> + gacl->acl_len = (char *)gpfs_ace_ptr(gacl, gacl->acl_nace)
> + - (char *)gacl;
> + return gacl;
> +}
>
> +static bool gpfsacl_process_smbacl(vfs_handle_struct *handle,
> + files_struct *fsp,
> + SMB4ACL_T *smbacl)
> +{
> + int ret;
> + struct gpfs_acl *gacl;
> + TALLOC_CTX *mem_ctx = talloc_tos();
> +
> + gacl = vfs_gpfs_smbacl2gpfsacl(mem_ctx, fsp, smbacl, true);
> + if (gacl == NULL) { /* out of memory */
> + return False;
> + }
> ret = smbd_gpfs_putacl(fsp->fsp_name->base_name,
> GPFS_PUTACL_STRUCT | GPFS_ACL_SAMBA, gacl);
> +
> + if ((ret != 0) && (errno == EINVAL)) {
> + DEBUG(10, ("Retry without nfs41 control flags\n"));
> + talloc_free(gacl);
> + gacl = vfs_gpfs_smbacl2gpfsacl(mem_ctx, fsp, smbacl, false);
> + if (gacl == NULL) { /* out of memory */
> + return False;
> + }
> + ret = smbd_gpfs_putacl(fsp->fsp_name->base_name,
> + GPFS_PUTACL_STRUCT | GPFS_ACL_SAMBA,
> + gacl);
> + }
> +
> if (ret != 0) {
> DEBUG(8, ("gpfs_putacl failed with %s\n", strerror(errno)));
> gpfs_dumpacl(8, gacl);
> --
> 1.8.3.2
>
More information about the samba-technical
mailing list