Re: [PATCH] fix NFSv4 acl detection on F39

[Jeff Layton <jlayton@redhat.com>]

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>