Re: [PATCH] fix NFSv4 acl detection on F39

[Christian Brauner]

On Tue, May 16, 2023 at 08:20:33AM -0400, Jeff Layton wrote:
> On Tue, 2023-05-16 at 11:17 +0200, Christian Brauner wrote:
> > On Mon, May 15, 2023 at 01:49:21PM -0400, Jeff Layton wrote:
> > > On Mon, 2023-05-15 at 17:28 +0000, Trond Myklebust wrote:
> > > > On Mon, 2023-05-15 at 13:11 -0400, Jeff Layton wrote:
> > > > > On Mon, 2023-05-15 at 11:50 +0000, Ondrej Valousek wrote:
> > > > > > Hi Paul,
> > > > > > 
> > > > > > Ok first of all, thanks for taking initiative on this, I am unable
> > > > > > to proceed on this on my own at the moment.
> > > > > > I see few problems with this:
> > > > > > 
> > > > > > 1. The calculation of the 'listbufsize' is incorrect in your patch.
> > > > > > It will _not_work as you expected and won't limit the number of
> > > > > > syscalls (which is why we came up with this patch, right?). Check
> > > > > > with my original proposal, we really need to check for
> > > > > > 'system.nfs4' xattr name presence here
> > > > > > 2. It mistakenly detects an ACL presence on files which do not have
> > > > > > any ACL on NFSv4 filesystem. Digging further it seems that kernel
> > > > > > in F39 behaves differently to the previous kernels:
> > > > > > 
> > > > > > F38: 
> > > > > > # getfattr -m . /path_to_nfs4_file
> > > > > > # file: path_to_nfs4_file
> > > > > > system.nfs4_acl                                    <---- only
> > > > > > single xattr detected
> > > > > > 
> > > > > > F39:
> > > > > > # getfattr -m . /path_to_nfs4_file
> > > > > > # file: path_to_nfs4_file
> > > > > > system.nfs4_acl
> > > > > > system.posix_acl_default
> > > > > > /* SOMETIMES even shows this */
> > > > > > system.posix_acl_default
> > > > > 
> > > > > (cc'ing Christian and relevant kernel lists)
> > > > > 
> > > > > I assume the F39 kernel is v6.4-rc based? If so, then I think that's
> > > > > a
> > > > > regression. NFSv4 client inodes should _not_ report a POSIX ACL
> > > > > attribute since the protocol doesn't support them.
> > > > > 
> > > > > In fact, I think the rationale in the kernel commit below is wrong.
> > > > > NFSv4 has a listxattr operation, but doesn't support POSIX ACLs.
> > > > > 
> > > > > Christian, do we need to revert this?
> > > > > 
> > > > > commit e499214ce3ef50c50522719e753a1ffc928c2ec1
> > > > > Author: Christian Brauner <brauner@kernel.org>
> > > > > Date:   Wed Feb 1 14:15:01 2023 +0100
> > > > > 
> > > > >     acl: don't depend on IOP_XATTR
> > > > >     
> > > > > 
> > > > 
> > > > 
> > > > No. The problem is commit f2620f166e2a ("xattr: simplify listxattr
> > > > helpers") which helpfully inserts posix acl handlers into
> > > > generic_listxattr(), and makes it impossible to call from
> > > > nfs4_listxattr().
> > > > 
> > > 
> > > 
> > > Ahh ok. Looking at that function though, it seems like it'd only report
> > > these for mounts that set SB_POSIXACL. Any reason that we have that
> > > turned on with v4 mounts?
> > 
> > You seem to just be calling generic_listxattr() in fs/nfs/nfs4proc.c and
> > not using it as an inode operation.
> > 
> 
> Correct, but even if we were, this would be doing the wrong thing. As
> Trond pointed out, f2620f166e2a changed the behavior of
> generic_listxattr to make it include the POSIX ACL entries.
> 
> > So imho just add a tiny helper into
> > fs/xattr.c that takes a boolean argument and skips over POSIX ACLs that
> > you can call in nfs4. That should be enough, no?
> > 
> 
> The only other user of generic_listxattr is HFS, and I don't think it
> supports POSIX ACLs either. I think we should probably just remove the
> call to posix_acl_listxattr from generic_listxattr.

Ok, I see. Thanks for spotting this.

The reason POSIX ACLs were moved into generic_listxattr() was because
they would've been included before too. If any filesystem would have
registered sb->s_xattr handlers for POSIX ACLs they would've been
included in generic_listxattr().

Can you send a patch to me so I can route it to Linus? Please add a
comment that this function doesn't return POSIX ACLs.