[PATCH] vfs_gpfs: Retry getacl with DAC capability if necessary

[Christof Schmitt]

On Thu, Jul 21, 2016 at 05:26:43PM -0700, Jeremy Allison wrote:
> On Wed, Jul 20, 2016 at 03:55:40PM -0700, Christof Schmitt wrote:
> > Here is another minor update based on input from Volker. This avoids the
> > bool parameter for the gpfs_getacl_with_capability function.
> 
> > From 60bbceff4f7fb925d35bdabd6e17a73614df1343 Mon Sep 17 00:00:00 2001
> > From: Christof Schmitt <cs at samba.org>
> > Date: Wed, 25 May 2016 15:56:49 -0700
> > Subject: [PATCH] vfs_gpfs: Retry getacl with DAC capability if necessary
> > 
> > Samba always tries to read the ACL of a file and checks it internally.
> > If the READ_ACL permission is missing in GPFS, then then reading the ACL
> > for Samba internal evaluation will be denied and opening the file or
> > directory fails. Change this by retrying reading the ACL with the DAC
> > capability if access was denied.
> > 
> > Signed-off-by: Christof Schmitt <cs at samba.org>
> > ---
> >  source3/modules/vfs_gpfs.c | 30 ++++++++++++++++++++++++++++--
> >  1 file changed, 28 insertions(+), 2 deletions(-)
> > 
> > diff --git a/source3/modules/vfs_gpfs.c b/source3/modules/vfs_gpfs.c
> > index 42a3c72..f096dd5 100644
> > --- a/source3/modules/vfs_gpfs.c
> > +++ b/source3/modules/vfs_gpfs.c
> > @@ -358,6 +358,21 @@ static void gpfs_dumpacl(int level, struct gpfs_acl *gacl)
> >  	}
> >  }
> >  
> > +static int gpfs_getacl_with_capability(const char *fname, int flags, void *buf)
> > +{
> > +	int ret, saved_errno;
> > +
> > +	set_effective_capability(DAC_OVERRIDE_CAPABILITY);
> > +
> > +	ret = gpfswrap_getacl(discard_const_p(char, fname), flags, buf);
> > +	saved_errno = errno;
> > +
> > +	drop_effective_capability(DAC_OVERRIDE_CAPABILITY);
> > +
> > +	errno = saved_errno;
> > +	return ret;
> > +}
> > +
> >  /*
> >   * get the ACL from GPFS, allocated on the specified mem_ctx
> >   * internally retries when initial buffer was too small
> > @@ -378,6 +393,7 @@ static void *vfs_gpfs_getacl(TALLOC_CTX *mem_ctx,
> >  	int ret, flags;
> >  	unsigned int *len;
> >  	size_t struct_size;
> > +	bool use_capability = false;
> >  
> >  again:
> >  
> > @@ -406,8 +422,18 @@ again:
> >  	/* set the length of the buffer as input value */
> >  	*len = size;
> >  
> > -	errno = 0;
> > -	ret = gpfswrap_getacl(discard_const_p(char, fname), flags, aclbuf);
> > +	if (use_capability) {
> > +		ret = gpfs_getacl_with_capability(fname, flags, aclbuf);
> 
> I don't understand the 2 lines above. use_capability is a local
> bool, not a static one and you just set it to false above so this
> can never be true. Am I missing something ?

Yes. There is a retry loop for the case that the ACL does not fit into
the allocaged buffer:

 if ((ret != 0) && (errno == ENOSPC)) {
...
                goto again;

The logic is that if capabilities are required, and the initial buffer
was too small, then the code should only do one additional retry with
the larger buffer and the capability.

At some point, i would like to refactor the loop for ENOSPC into a
simple retry, but that is independent of fallback to use the capability.

> > +	} else {
> > +		ret = gpfswrap_getacl(discard_const_p(char, fname),
> > +				      flags, aclbuf);
> > +		if ((ret != 0) && (errno == EACCES)) {
> > +			DBG_DEBUG("Retry with DAC capability for %s\n", fname);
> > +			use_capability = true;
> > +			ret = gpfs_getacl_with_capability(fname, flags, aclbuf);
> > +		}
> > +	}
> > +
> 
> Looks like the above block (without the else {...}) is all
> you need for this.

The "else" case here is for the initial attempt. We do not want the
overhead of acquiring the capability for every attempt to read the ACL,
only when the READ_ACL is missing and the initial attempt results in
EACCESS.

Christof