Re: [PATCH v2 17/30] acl: add vfs_remove_acl()

[Christian Brauner <brauner@kernel.org>]

On Tue, Sep 27, 2022 at 06:55:37PM -0400, Paul Moore wrote:
> On Mon, Sep 26, 2022 at 11:24 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> > In previous patches we implemented get and set inode operations for all
> > non-stacking filesystems that support posix acls but didn't yet
> > implement get and/or set acl inode operations. This specifically
> > affected cifs and 9p.
> >
> > Now we can build a posix acl api based solely on get and set inode
> > operations. We add a new vfs_remove_acl() api that can be used to set
> > posix acls. This finally removes all type unsafety and type conversion
> > issues explained in detail in [1] that we aim to get rid of.
> >
> > After we finished building the vfs api we can switch stacking
> > filesystems to rely on the new posix api and then finally switch the
> > xattr system calls themselves to rely on the posix acl api.
> >
> > Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org [1]
> > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> > ---
> >
> > Notes:
> >     /* v2 */
> >     unchanged
> >
> >  fs/posix_acl.c            | 65 +++++++++++++++++++++++++++++++++++++++
> >  include/linux/posix_acl.h |  8 +++++
> >  2 files changed, 73 insertions(+)
> 
> ...
> 
> > diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> > index 18873be583a9..40038851bfe1 100644
> > --- a/fs/posix_acl.c
> > +++ b/fs/posix_acl.c
> > @@ -1484,3 +1484,68 @@ struct posix_acl *vfs_get_acl(struct user_namespace *mnt_userns,
> >         return acl;
> >  }
> >  EXPORT_SYMBOL(vfs_get_acl);
> > +
> > +/**
> > + * vfs_remove_acl - remove posix acls
> > + * @mnt_userns: user namespace of the mount
> > + * @dentry: the dentry based on which to retrieve the posix acls
> > + * @acl_name: the name of the posix acl
> > + *
> > + * This function removes posix acls.
> > + *
> > + * Return: On success 0, on error negative errno.
> > + */
> > +int vfs_remove_acl(struct user_namespace *mnt_userns, struct dentry *dentry,
> > +                  const char *acl_name)
> > +{
> > +       int acl_type;
> > +       int error;
> > +       struct inode *inode = d_inode(dentry);
> > +       struct inode *delegated_inode = NULL;
> > +
> > +       acl_type = posix_acl_type(acl_name);
> > +       if (acl_type < 0)
> > +               return -EINVAL;
> > +
> > +retry_deleg:
> > +       inode_lock(inode);
> > +
> > +       /*
> > +        * We only care about restrictions the inode struct itself places upon
> > +        * us otherwise POSIX ACLs aren't subject to any VFS restrictions.
> > +        */
> > +       error = xattr_permission(mnt_userns, inode, acl_name, MAY_WRITE);
> > +       if (error)
> > +               goto out_inode_unlock;
> > +
> > +       error = security_inode_removexattr(mnt_userns, dentry, acl_name);
> > +       if (error)
> > +               goto out_inode_unlock;
> 
> Similar to my comments in patch 16/30 for vfs_get_acl(), I would
> suggest a dedicated ACL remove hook here.  Yes, it's still a little
> bit silly, but if we are going to make one dedicated hook, we might as
> well do them all.

Sure, I don't mind doing that. I'll add the infrastructure and then the
individual LSMs can add their own hooks.