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>
