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

From: Paul Moore <paul@paul-moore.com>
To: Christian Brauner <brauner@kernel.org>
Cc: linux-fsdevel@vger.kernel.org, Seth Forshee <sforshee@kernel.org>,
	Christoph Hellwig <hch@lst.de>, Al Viro <viro@zeniv.linux.org.uk>,
	linux-security-module@vger.kernel.org
Subject: Re: [PATCH v2 17/30] acl: add vfs_remove_acl()
Date: Tue, 27 Sep 2022 18:55:37 -0400	[thread overview]
Message-ID: <CAHC9VhRZBP6fXtBseJJ_zHy+yoHMkVkSUbAFXmer7bKpt1Qvow@mail.gmail.com> (raw)
In-Reply-To: <20220926140827.142806-18-brauner@kernel.org>

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.

> +       error = try_break_deleg(inode, &delegated_inode);
> +       if (error)
> +               goto out_inode_unlock;
> +
> +       if (inode->i_opflags & IOP_XATTR)
> +               error = set_posix_acl(mnt_userns, dentry, acl_type, NULL);
> +       else if (unlikely(is_bad_inode(inode)))
> +               error = -EIO;
> +       else
> +               error = -EOPNOTSUPP;
> +       if (!error) {
> +               fsnotify_xattr(dentry);
> +               evm_inode_post_removexattr(dentry, acl_name);
> +       }
> +
> +out_inode_unlock:
> +       inode_unlock(inode);
> +
> +       if (delegated_inode) {
> +               error = break_deleg_wait(&delegated_inode);
> +               if (!error)
> +                       goto retry_deleg;
> +       }
> +
> +       return error;
> +}
> +EXPORT_SYMBOL(vfs_remove_acl);

--
paul-moore.com