Re: [PATCH] fix NFSv4 acl detection on F39

Re: [PATCH] fix NFSv4 acl detection on F39

[Jeff Layton]

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
    
    All codepaths that don't want to implement POSIX ACLs should simply not
    implement the associated inode operations instead of relying on
    IOP_XATTR. That's the case for all filesystems today.
    
    For vfs_listxattr() all filesystems that explicitly turn of xattrs for a
    given inode all set inode->i_op to a dedicated set of inode operations
    that doesn't implement ->listxattr().  We can remove the dependency of
    vfs_listxattr() on IOP_XATTR.
    
    Removing this dependency will allow us to decouple POSIX ACLs from
    IOP_XATTR and they can still be listed even if no other xattr handlers
    are implemented. Otherwise we would have to implement elaborate schemes
    to raise IOP_XATTR even if sb->s_xattr is set to NULL.
    
    Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>


> 
> Now I faintly recall there was an activity in to move POSIX acls calculation from userspace to kernel (now Jeff in CC will hopefully clarify this)
> 

The POSIX<->NFSv4 ACL translation has always been in the kernel server.
It has to be, as the primary purpose is to translate v4 ACLs from the
clients to and from the POSIX ACLs that the exported Linux filesystems
support.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] fix NFSv4 acl detection on F39

[Trond Myklebust]

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().

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH] fix NFSv4 acl detection on F39

[Jeff Layton]

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?

This patch fixes the bug for me, but I haven't done any testing with it:

---------------8<-----------------

[RFC PATCH] nfs: don't set SB_POSIXACL on NFSv4 mounts

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfs/super.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 30e53e93049e..cbb8de6e25dc 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1057,7 +1057,6 @@ static void nfs_fill_super(struct super_block *sb, struct nfs_fs_context *ctx)
 		sb->s_export_op = &nfs_export_ops;
 		break;
 	case 4:
-		sb->s_flags |= SB_POSIXACL;
 		sb->s_time_gran = 1;
 		sb->s_time_min = S64_MIN;
 		sb->s_time_max = S64_MAX;
-- 
2.40.1

Re: [PATCH] fix NFSv4 acl detection on F39

[Trond Myklebust]

On Mon, 2023-05-15 at 13:49 -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?
> 

So, while it may seem reasonable to assume that SB_POSIXACL means
'supports posix ACLs', that doesn't actually match the definition in
include/linux/fs.h (or the equivalent definition for MS_POSIXACL):

#define SB_POSIXACL     (1<<16) /* VFS does not apply the umask */

See the use of mode_strip_umask() in vfs_prepare_mode().

So yes, NFSv4 sets SB_POSIXACL, and it has done so since commit
a8a5da996df7 ("nfs: Set MS_POSIXACL always") because we require the VFS
to not apply the umask. 

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH] fix NFSv4 acl detection on F39

[Christian Brauner]

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. 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?

Re: [PATCH] fix NFSv4 acl detection on F39

[Jeff Layton]

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.
-- 
Jeff Layton <jlayton@redhat.com>

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.