Re: [PATCH 3/6] acl: add vfs_set_acl_prepare()

[Christian Brauner <brauner@kernel.org>]

On Tue, Sep 06, 2022 at 10:24:32AM +0200, Christian Brauner wrote:
> On Tue, Sep 06, 2022 at 10:15:10AM +0200, Christoph Hellwig wrote:
> > On Tue, Sep 06, 2022 at 10:07:44AM +0200, Christian Brauner wrote:
> > > I've tried switching all filesystem to simply rely on
> > > i_op->{g,s}et_acl() but this doesn't work for at least 9p and cifs
> > > because they need access to the dentry. cifs hasn't even implemented
> > > i_op->get_acl() and I don't think they can because of the lack of a
> > > dentry argument.
> > > 
> > > The problem is not just that i_op->{g,s}et_acl() don't take a dentry
> > > argument it's in principle also super annoying to pass it to them
> > > because i_op->get_acl() is used to retrieve POSIX ACLs during permission
> > > checking and thus is called from generic_permission() and thus
> > > inode_permission() and I don't think we want or even can pass down a
> > > dentry everywhere for those. So I stopped short of finishing this
> > > implementation because of that.
> > > 
> > > So in order to make this work for cifs and 9p we would probably need a
> > > new i_op method that is separate from the i_op->get_acl() one used in
> > > the acl_permission_check() and friends...
> > 
> > Even if we can't use the existing methods, I think adding new
> > set_denstry_acl/get_dentry_acl (or whatever we name them) methods is
> > still better than doing this overload of the xattr methods
> > (just like the uapi overload instead of separate syscalls, but we
> > can't fix that).
> 
> Let me explore and see if I can finish the branch using dedicated i_op
> methods instead of updating i_op->get_acl().
> 
> I think any data that requires to be interpreteted by the VFS needs to
> have dedicated methods. Seth's branch for example, tries to add
> i_op->{g,s}et_vfs_caps() for vfs caps which also store ownership
> information instead of hacking it through the xattr api like we do now.

I finished a draft of the series. It severly lacks in meangingful commit
messages and I won't be able to finish it before Plumbers next week.
If people want to take a look the branch is available on gitlab and
kernel.org:

https://gitlab.com/brauner/linux/-/commits/fs.posix_acl.vfsuid/
https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/log/?h=fs.posix_acl.vfsuid

This passes xfstests (ext4, xfs, btrfs, overlayfs with and without
idmapped layers, and LTP). I only needed to add i_op->get_dentry_acl()
as it was possible to adapt ->set_acl() to take a dentry argument and
not an inode argument.

So we have a dedicated POSIX ACL api:

struct posix_acl *vfs_get_acl(struct user_namespace *mnt_userns,
                              struct dentry *dentry, const char *acl_name)
int vfs_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
                const char *acl_name, struct posix_acl *kacl)
int vfs_remove_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
                   const char *acl_name)

only relying on i_op->get_dentry_acl() and i_op->set_acl() removing the
void * and uapi POSIX ACL abuse completely.