linux-kernel - Re: [PATCH RFC] nfs4: add a get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230908-bandbreite-orgel-065607d1b281@brauner>
Date:   Fri, 8 Sep 2023 14:55:15 +0200
From:   Christian Brauner <brauner@...nel.org>
To:     Jeff Layton <jlayton@...nel.org>
Cc:     Trond Myklebust <trond.myklebust@...merspace.com>,
        Anna Schumaker <anna@...nel.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org,
        Ondrej Valousek <ondrej.valousek.xm@...esas.com>
Subject: Re: [PATCH RFC] nfs4: add a get_acl stub handler

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@...esas.com>
> Signed-off-by: Jeff Layton <jlayton@...nel.org>
> ---
> 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. ;)

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@...nel.org>