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] [day] [month] [year] [list]

Date:   Fri, 08 Sep 2023 09:47:54 -0400
From:   Jeff Layton <jlayton@...nel.org>
To:     Christian Brauner <brauner@...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 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@...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. ;)

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

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