Re: [PATCH RFC] nfs4: add a get_acl stub handler

[Jeff Layton]

On Fri, 2023-09-08 at 14:55 +0200, Christian Brauner wrote:
> On Thu, Sep 07, 2023 at 01:32:36PM -0400, Jeff Layton wrote:
> > In older kernels, attempting to fetch that system.posix_acl_access on
> > NFSv4 would return -EOPNOTSUPP, but in more recent kernels that returns
> > -ENODATA.
> > 
> > Most filesystems that don't support POSIX ACLs leave the SB_POSIXACL
> > flag clear, which cues the VFS to return -EOPNOTSUPP in this situation.
> > We can't do that with NFSv4 since that flag also cues the VFS to avoid
> > applying the umask early.
> > 
> > Fix this by adding a stub get_acl handler for NFSv4 that always returns
> > -EOPNOTSUPP.
> > 
> > Reported-by: Ondrej Valousek <ondrej.valousek.xm@xxxxxxxxxxx>
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> > I suspect that this problem popped in due to some VFS layer changes. I
> > haven't identified the patch that broke it, but I think this is probably
> > the least invasive way to fix it.
> > 
> > Another alternative would be to return -EOPNOTSUPP on filesystems that
> > set SB_POSIXACL but that don't set get_acl or get_inode_acl.
> > 
> > Thoughts?
> 
> Yes: I hate POSIX ACLs. ;)

The API is quite weird, yes.

> Before the VFS rework to only rely on i_op->*acl* methods POSIX ACLs
> were set using sb->s_xattr handlers. So when a filesystem raised
> SB_POSIXACL but didn't set sb->s_xattr handlers for POSIX ACLs we would:
> 
> __vfs_getxattr()
> -> xattr_resolve_name()
>   // no match so return EOPNOTSUPP
> 
> No we have
> 
> vfs_get_acl()
> -> __get_acl()
>    -> i_op->get_acl
>    // no get_acl inode method return ENODATA
> 
> So as a bugfix to backport I think you should do exactly what you do
> here because I'm not sure if some fs relies on ENODATA to be returned if
> no get_acl inode method is set. There's a lot of quirkiness everywhere.
> But we should look through all callers and if nothing relies on EINVAL
> just start returning EOPNOTSUPP if no get_acl i_op is set.
> 
> Looks good to me,
> Acked-by: Christian Brauner <brauner@xxxxxxxxxx>

Thanks. I did some rudimentary git grepping, and I don't see any that
set SB_POSIXACL but don't set either get_acl or get_inode_acl. I'll spin
up a patch and we can get it into -next for a while and see if anything
shakes out.
-- 
Jeff Layton <jlayton@xxxxxxxxxx>