Re: [PATCH] fs: fix regression querying for ACL on fs's that don't support them

[Jeff Layton]

On Sun, 2023-09-10 at 12:14 +0200, Christian Brauner wrote:
> On Fri, Sep 08, 2023 at 05:05:27PM -0400, Jeff Layton wrote:
> > In the not too distant past, the VFS ACL infrastructure would return
> > -EOPNOTSUPP on filesystems (like NFS) that set SB_POSIXACL but that
> > don't supply a get_acl or get_inode_acl method. On more recent kernels
> > this returns -ENODATA, which breaks one method of detecting when ACLs
> > are supported.
> > 
> > Fix __get_acl to also check whether the inode has a "get_(inode_)?acl"
> > method and to just return -EOPNOTSUPP if not.
> > 
> > Reported-by: Ondrej Valousek <ondrej.valousek.xm@xxxxxxxxxxx>
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> > This patch is another approach to fixing this issue. I don't care too
> > much either way which approach we take, but this may fix the problem
> > for other filesystems too. Should we take a belt and suspenders
> > approach here and fix it in both places?
> > ---
> >  fs/posix_acl.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> > index a05fe94970ce..4c7c62040c43 100644
> > --- a/fs/posix_acl.c
> > +++ b/fs/posix_acl.c
> > @@ -130,8 +130,12 @@ static struct posix_acl *__get_acl(struct mnt_idmap *idmap,
> >  	if (!is_uncached_acl(acl))
> >  		return acl;
> >  
> > -	if (!IS_POSIXACL(inode))
> > -		return NULL;
> > +	/*
> > +	 * NB: checking this after checking for a cached ACL allows tmpfs
> > +	 * (which doesn't specify a get_acl operation) to work properly.
> > +	 */
> > +	if (!IS_POSIXACL(inode) || (!inode->i_op->get_acl && !inode->i_op->get_inode_acl))
> > +		return ERR_PTR(-EOPNOTSUPP);
> 
> Hmmm, I think that'll cause issues for permission checking during
> lookup:
> 
> generic_permission()
> -> acl_permission_check()
>    -> check_acl()
>       -> get_inode_acl()
>          -> __get_acl()
>             // return ERR_PTR(-EOPNOTSUPP) instead of NULL
> 
> Before this change this would've returned NULL and thus check_acl()
> would've returned EAGAIN which would've informed acl_permission_check()
> to continue with non-ACL based permission checking.
> 
> Now you're going to error out with EOPNOTSUPP and cause permission
> checking to fallback to CAP_DAC_READ_SEARCH/CAP_DAC_OVERRIDE.
> 
> So if you want this change you'll either need to change check_acl() as well.
> Unless I'm misreading.

Ok, I didn't see problems in testing this with xfstests, but maybe it
didn't tickle that bug in the right way.

Instead of this, what if we were to add a new SB_NOUMASK flag? NFS could
set that, and then we could fix the places that NFS needs to use that
instead. That might bring more clarity to this code -- SB_POSIXACL would
really mean that ACLs were supported.

I'll see what I can put together...

Thanks!
--
Jeff Layton <jlayton@xxxxxxxxxx>