[PATCH] Storing the ACL control flags with vfs gpfs.
Christof Schmitt
cs at samba.org
Thu Mar 13 13:28:53 MDT 2014
Hi Alex,
On Wed, Mar 12, 2014 at 02:24:48PM +0100, Alexander Werth wrote:
> Hi,
>
> The current nfs4 module drops the ACL control flags from the security
> descriptor. The NFS4.1 spec also specifies ACL flags so I added a way to
> pass these control flags on to the nfs4 filesystems.
>
> The first patch adds hocks to the nfs4_acls common code to store and
> retrieve the control flags in the acl struct.
>
>
> And I wrote a patch that allows to store these ACL control flags in the
> vfs gpfs module which is supported with gpfs 3.5 and up.
> The vfs gpfs patch allows to build without the control flags on older
> gpfs versions and also has the ability to run the same vfs module on
> older and newer gpfs versions.
>
> Both patches apply to the current master.
> Please comment.
In the past, we have avoided ifdefs in the vfs_gpfs module. I am
wondering if it would be possible to access the flags field through a
pointer and avoid the gpfs 3.5 specific parts in the code. Once we no
longer care about older gpfs versions, we can cleanup the code and use
the proper struct members.
None of the solutions (ifdef or pointer) is perfect, but i would be
leaning towards always getting the same module from a compile, and avoid
the ifdefs.
A smaller comment would be avoid the goto again loop, and simply
issuing the call again. I think the intent is to just retry once with
different parameters, so we don't need the loop.
And a very minor thing is to use the C99 style integers (e.g. uint_16)
instead of e.g. uin16:
README.Coding
..., new code should adhere to the following conventions:
...
* Exact width integers are of type [u]int[8|16|32|64]_t
Regards,
Christof
> >From ed37ead553c396ce5c367ca80e0388fd77cbc416 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.
>
> ---
> 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..e55f504 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 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 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 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..40ef5ee 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 smb_get_controlflags(SMB4ACL_T *theacl);
> +
> +bool smb_set_controlflags(SMB4ACL_T *theacl, uint16 controlflags);
> +
> NTSTATUS smb_fget_nt_acl_nfs4(files_struct *fsp,
> uint32 security_info,
> TALLOC_CTX *mem_ctx,
> --
> 1.8.3.2
>
> >From 72af92bf793f8ff67d46639f3c0542702cca2e4a 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.
>
> ---
> source3/modules/vfs_gpfs.c | 120 +++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 105 insertions(+), 15 deletions(-)
>
> diff --git a/source3/modules/vfs_gpfs.c b/source3/modules/vfs_gpfs.c
> index 760d27f..259be45 100644
> --- a/source3/modules/vfs_gpfs.c
> +++ b/source3/modules/vfs_gpfs.c
> @@ -51,6 +51,33 @@ struct gpfs_config_data {
> bool settimes;
> };
>
> +#ifdef GPFS_ACL_LEVEL_BASE
> +#define USE_CONTROL_FLAGS true
> +#else
> +#define USE_CONTROL_FLAGS false
> +#endif
> +
> +#ifndef GPFS_ACL_LEVEL_V4FLAGS
> +#define GPFS_ACL_LEVEL_V4FLAGS 1
> +#endif
> +
> +#ifndef ACL_FLAGS
> +#ifdef GPFS_ACL_LEVEL_BASE
> +#define ACL_FLAGS(gacl) (gacl->acl_level == GPFS_ACL_LEVEL_V4FLAGS ? \
> + gacl->v4Level1.acl_flags : 0)
> +#else
> +#define ACL_FLAGS(gacl) 0
> +#endif
> +#endif
> +
> +#ifndef PTR_ACE
> +#ifdef GPFS_ACL_LEVEL_BASE
> +#define PTR_ACE(gacl, i) (gacl->acl_level == GPFS_ACL_LEVEL_V4FLAGS ? \
> + &gacl->v4Level1.ace_v4[i] : &gacl->ace_v4[i])
> +#else
> +#define PTR_ACE(gacl, i) &gacl->ace_v4[i]
> +#endif
> +#endif
>
> static int vfs_gpfs_kernel_flock(vfs_handle_struct *handle, files_struct *fsp,
> uint32 share_mode, uint32 access_mask)
> @@ -207,6 +234,36 @@ static int vfs_gpfs_get_real_filename(struct vfs_handle_struct *handle,
> return 0;
> }
>
> +static void sd2gpfs_control(uint16 control, struct gpfs_acl *gacl)
> +{
> + uint32 gpfs_aclflags = 0;
> +#ifdef ACL4_FLAG_DACL_PRESENT
> + gpfs_aclflags = control << 8;
> + gpfs_aclflags &=
> + 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;
> + gpfs_aclflags |= control &
> + SEC_DESC_DACL_PRESENT ? 0 : ACL4_FLAG_NULL_DACL;
> + gpfs_aclflags |= control &
> + SEC_DESC_SACL_PRESENT ? 0 : ACL4_FLAG_NULL_SACL;
> + gacl->acl_level = GPFS_ACL_LEVEL_V4FLAGS;
> + gacl->v4Level1.acl_flags = gpfs_aclflags;
> +#endif
> +}
> +
> +static uint16 gpfs2sd_control(uint32 gpfs_aclflags)
> +{
> + uint16 control = gpfs_aclflags >> 8;
> + control |= SEC_DESC_SELF_RELATIVE;
> + return control;
> +}
> +
> static void gpfs_dumpacl(int level, struct gpfs_acl *gacl)
> {
> gpfs_aclCount_t i;
> @@ -216,14 +273,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, 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 = PTR_ACE(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));
> }
> }
>
> @@ -247,6 +308,7 @@ static void *vfs_gpfs_getacl(TALLOC_CTX *mem_ctx,
> int ret, flags;
> unsigned int *len;
> size_t struct_size;
> + bool getcontrolflags = USE_CONTROL_FLAGS;
>
> again:
>
> @@ -265,6 +327,9 @@ again:
> } else {
> struct gpfs_acl *buf = (struct gpfs_acl *) aclbuf;
> buf->acl_type = type;
> + if (getcontrolflags) {
> + buf->acl_level = GPFS_ACL_LEVEL_V4FLAGS;
> + }
> flags = GPFS_GETACL_STRUCT;
> len = &(buf->acl_len);
> struct_size = sizeof(struct gpfs_acl);
> @@ -290,6 +355,13 @@ again:
> goto again;
> }
>
> + if ((ret != 0) && (errno == EINVAL) && getcontrolflags) {
> + DEBUG(10, ("Retry without nfs41 acls\n"));
> + getcontrolflags = false;
> + talloc_free(aclbuf);
> + goto again;
> + }
> +
> if (ret != 0) {
> DEBUG(5, ("smbd_gpfs_getacl failed with %s\n",
> strerror(errno)));
> @@ -330,12 +402,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 == GPFS_ACL_LEVEL_V4FLAGS) {
> + uint16 control = gpfs2sd_control(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, 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 = PTR_ACE(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 +445,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 = PTR_ACE(gacl, i-1);
> if (prev->aceType == SMB_ACE4_ACCESS_ALLOWED_ACE_TYPE &&
> prev->aceFlags == gace->aceFlags &&
> prev->aceIFlags == gace->aceIFlags &&
> @@ -376,7 +453,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;
> @@ -490,9 +567,10 @@ static bool gpfsacl_process_smbacl(vfs_handle_struct *handle, files_struct *fsp,
> SMB4ACE_T *smbace;
> struct gpfs_acl *gacl;
> TALLOC_CTX *mem_ctx = talloc_tos();
> + bool putcontrolflags = USE_CONTROL_FLAGS;
>
> - 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) + 8
> + + smb_get_naces(smbacl) * sizeof(gpfs_ace_v4_t);
>
> gacl = (struct gpfs_acl *)TALLOC_SIZE(mem_ctx, gacl_len);
> if (gacl == NULL) {
> @@ -501,14 +579,19 @@ static bool gpfsacl_process_smbacl(vfs_handle_struct *handle, files_struct *fsp,
> return False;
> }
>
> - gacl->acl_len = gacl_len;
> +again:
> +
> gacl->acl_level = 0;
> gacl->acl_version = GPFS_ACL_VERSION_NFS4;
> gacl->acl_type = GPFS_ACL_TYPE_NFS4;
> gacl->acl_nace = 0; /* change later... */
>
> + if (putcontrolflags) {
> + 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 = PTR_ACE(gacl, gacl->acl_nace);
> SMB_ACE4PROP_T *aceprop = smb_get_ace4(smbace);
>
> gace->aceType = aceprop->aceType;
> @@ -566,9 +649,16 @@ static bool gpfsacl_process_smbacl(vfs_handle_struct *handle, files_struct *fsp,
>
> gacl->acl_nace++;
> }
> + gacl->acl_len = (char *)PTR_ACE(gacl, gacl->acl_nace) - (char *)gacl;
>
> ret = smbd_gpfs_putacl(fsp->fsp_name->base_name,
> GPFS_PUTACL_STRUCT | GPFS_ACL_SAMBA, gacl);
> + if ((ret != 0) && (errno == EINVAL) && putcontrolflags) {
> + DEBUG(10, ("Retry without nfs41 control flags\n"));
> + putcontrolflags = false;
> + goto again;
> + }
> +
> 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