On Tue, Aug 08, 2023 at 09:30:59PM -0700, Hugh Dickins wrote:
> tmpfs wants to support limited user extended attributes, but kernfs
> (or cgroupfs, the only kernfs with KERNFS_ROOT_SUPPORT_USER_XATTR)
> already supports user extended attributes through simple xattrs: but
> limited by a policy (128KiB per inode) too liberal to be used on tmpfs.
>
> To allow a different limiting policy for tmpfs, without affecting the
> policy for kernfs, change simple_xattr_set() to return the replaced or
> removed xattr (if any), leaving the caller to update their accounting
> then free the xattr (by simple_xattr_free(), renamed from the static
> free_simple_xattr()).
>
Reviewed-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
> Signed-off-by: Hugh Dickins <hughd@xxxxxxxxxx>
> ---
> fs/kernfs/inode.c | 46 +++++++++++++++++++++++++---------------
> fs/xattr.c | 51 +++++++++++++++++++--------------------------
> include/linux/xattr.h | 7 ++++---
> mm/shmem.c | 10 +++++----
> 4 files changed, 61 insertions(+), 53 deletions(-)
>
> diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
> index b22b74d1a115..fec5d5f78f07 100644
> --- a/fs/kernfs/inode.c
> +++ b/fs/kernfs/inode.c
> @@ -306,11 +306,17 @@ int kernfs_xattr_get(struct kernfs_node *kn, const char *name,
> int kernfs_xattr_set(struct kernfs_node *kn, const char *name,
> const void *value, size_t size, int flags)
> {
> + struct simple_xattr *old_xattr;
> struct kernfs_iattrs *attrs = kernfs_iattrs(kn);
> if (!attrs)
> return -ENOMEM;
>
> - return simple_xattr_set(&attrs->xattrs, name, value, size, flags, NULL);
> + old_xattr = simple_xattr_set(&attrs->xattrs, name, value, size, flags);
> + if (IS_ERR(old_xattr))
> + return PTR_ERR(old_xattr);
> +
> + simple_xattr_free(old_xattr);
> + return 0;
> }
>
> static int kernfs_vfs_xattr_get(const struct xattr_handler *handler,
> @@ -342,7 +348,7 @@ static int kernfs_vfs_user_xattr_add(struct kernfs_node *kn,
> {
> atomic_t *sz = &kn->iattr->user_xattr_size;
> atomic_t *nr = &kn->iattr->nr_user_xattrs;
> - ssize_t removed_size;
> + struct simple_xattr *old_xattr;
> int ret;
>
> if (atomic_inc_return(nr) > KERNFS_MAX_USER_XATTRS) {
> @@ -355,13 +361,18 @@ static int kernfs_vfs_user_xattr_add(struct kernfs_node *kn,
> goto dec_size_out;
> }
>
> - ret = simple_xattr_set(xattrs, full_name, value, size, flags,
> - &removed_size);
> -
> - if (!ret && removed_size >= 0)
> - size = removed_size;
> - else if (!ret)
> + old_xattr = simple_xattr_set(xattrs, full_name, value, size, flags);
> + if (!old_xattr)
> return 0;
> +
> + if (IS_ERR(old_xattr)) {
> + ret = PTR_ERR(old_xattr);
> + goto dec_size_out;
> + }
> +
> + ret = 0;
> + size = old_xattr->size;
> + simple_xattr_free(old_xattr);
> dec_size_out:
> atomic_sub(size, sz);
> dec_count_out:
> @@ -376,18 +387,19 @@ static int kernfs_vfs_user_xattr_rm(struct kernfs_node *kn,
> {
> atomic_t *sz = &kn->iattr->user_xattr_size;
> atomic_t *nr = &kn->iattr->nr_user_xattrs;
> - ssize_t removed_size;
> - int ret;
> + struct simple_xattr *old_xattr;
>
> - ret = simple_xattr_set(xattrs, full_name, value, size, flags,
> - &removed_size);
> + old_xattr = simple_xattr_set(xattrs, full_name, value, size, flags);
> + if (!old_xattr)
> + return 0;
>
> - if (removed_size >= 0) {
> - atomic_sub(removed_size, sz);
> - atomic_dec(nr);
> - }
> + if (IS_ERR(old_xattr))
> + return PTR_ERR(old_xattr);
>
> - return ret;
> + atomic_sub(old_xattr->size, sz);
> + atomic_dec(nr);
> + simple_xattr_free(old_xattr);
> + return 0;
> }
>
> static int kernfs_vfs_user_xattr_set(const struct xattr_handler *handler,
> diff --git a/fs/xattr.c b/fs/xattr.c
> index e7bbb7f57557..ba37a8f5cfd1 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -1040,12 +1040,12 @@ const char *xattr_full_name(const struct xattr_handler *handler,
> EXPORT_SYMBOL(xattr_full_name);
>
> /**
> - * free_simple_xattr - free an xattr object
> + * simple_xattr_free - free an xattr object
> * @xattr: the xattr object
> *
> * Free the xattr object. Can handle @xattr being NULL.
> */
> -static inline void free_simple_xattr(struct simple_xattr *xattr)
> +void simple_xattr_free(struct simple_xattr *xattr)
> {
> if (xattr)
> kfree(xattr->name);
> @@ -1164,7 +1164,6 @@ int simple_xattr_get(struct simple_xattrs *xattrs, const char *name,
> * @value: the value to store along the xattr
> * @size: the size of @value
> * @flags: the flags determining how to set the xattr
> - * @removed_size: the size of the removed xattr
> *
> * Set a new xattr object.
> * If @value is passed a new xattr object will be allocated. If XATTR_REPLACE
> @@ -1181,29 +1180,27 @@ int simple_xattr_get(struct simple_xattrs *xattrs, const char *name,
> * nothing if XATTR_CREATE is specified in @flags or @flags is zero. For
> * XATTR_REPLACE we fail as mentioned above.
> *
> - * Return: On success zero and on error a negative error code is returned.
> + * Return: On success, the removed or replaced xattr is returned, to be freed
> + * by the caller; or NULL if none. On failure a negative error code is returned.
> */
> -int simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
> - const void *value, size_t size, int flags,
> - ssize_t *removed_size)
> +struct simple_xattr *simple_xattr_set(struct simple_xattrs *xattrs,
> + const char *name, const void *value,
> + size_t size, int flags)
> {
> - struct simple_xattr *xattr = NULL, *new_xattr = NULL;
> + struct simple_xattr *old_xattr = NULL, *new_xattr = NULL;
> struct rb_node *parent = NULL, **rbp;
> int err = 0, ret;
>
> - if (removed_size)
> - *removed_size = -1;
> -
> /* value == NULL means remove */
> if (value) {
> new_xattr = simple_xattr_alloc(value, size);
> if (!new_xattr)
> - return -ENOMEM;
> + return ERR_PTR(-ENOMEM);
>
> new_xattr->name = kstrdup(name, GFP_KERNEL);
> if (!new_xattr->name) {
> - free_simple_xattr(new_xattr);
> - return -ENOMEM;
> + simple_xattr_free(new_xattr);
> + return ERR_PTR(-ENOMEM);
> }
> }
>
> @@ -1217,12 +1214,12 @@ int simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
> else if (ret > 0)
> rbp = &(*rbp)->rb_right;
> else
> - xattr = rb_entry(*rbp, struct simple_xattr, rb_node);
> - if (xattr)
> + old_xattr = rb_entry(*rbp, struct simple_xattr, rb_node);
> + if (old_xattr)
> break;
> }
>
> - if (xattr) {
> + if (old_xattr) {
> /* Fail if XATTR_CREATE is requested and the xattr exists. */
> if (flags & XATTR_CREATE) {
> err = -EEXIST;
> @@ -1230,12 +1227,10 @@ int simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
> }
>
> if (new_xattr)
> - rb_replace_node(&xattr->rb_node, &new_xattr->rb_node,
> - &xattrs->rb_root);
> + rb_replace_node(&old_xattr->rb_node,
> + &new_xattr->rb_node, &xattrs->rb_root);
> else
> - rb_erase(&xattr->rb_node, &xattrs->rb_root);
> - if (!err && removed_size)
> - *removed_size = xattr->size;
> + rb_erase(&old_xattr->rb_node, &xattrs->rb_root);
> } else {
> /* Fail if XATTR_REPLACE is requested but no xattr is found. */
> if (flags & XATTR_REPLACE) {
> @@ -1260,12 +1255,10 @@ int simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
>
> out_unlock:
> write_unlock(&xattrs->lock);
> - if (err)
> - free_simple_xattr(new_xattr);
> - else
> - free_simple_xattr(xattr);
> - return err;
> -
> + if (!err)
> + return old_xattr;
> + simple_xattr_free(new_xattr);
> + return ERR_PTR(err);
> }
>
> static bool xattr_is_trusted(const char *name)
> @@ -1386,7 +1379,7 @@ void simple_xattrs_free(struct simple_xattrs *xattrs)
> rbp_next = rb_next(rbp);
> xattr = rb_entry(rbp, struct simple_xattr, rb_node);
> rb_erase(&xattr->rb_node, &xattrs->rb_root);
> - free_simple_xattr(xattr);
> + simple_xattr_free(xattr);
> rbp = rbp_next;
> }
> }
> diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> index d591ef59aa98..e37fe667ae04 100644
> --- a/include/linux/xattr.h
> +++ b/include/linux/xattr.h
> @@ -116,11 +116,12 @@ struct simple_xattr {
> void simple_xattrs_init(struct simple_xattrs *xattrs);
> void simple_xattrs_free(struct simple_xattrs *xattrs);
> struct simple_xattr *simple_xattr_alloc(const void *value, size_t size);
> +void simple_xattr_free(struct simple_xattr *xattr);
> int simple_xattr_get(struct simple_xattrs *xattrs, const char *name,
> void *buffer, size_t size);
> -int simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
> - const void *value, size_t size, int flags,
> - ssize_t *removed_size);
> +struct simple_xattr *simple_xattr_set(struct simple_xattrs *xattrs,
> + const char *name, const void *value,
> + size_t size, int flags);
> ssize_t simple_xattr_list(struct inode *inode, struct simple_xattrs *xattrs,
> char *buffer, size_t size);
> void simple_xattr_add(struct simple_xattrs *xattrs,
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 0f83d86fd8b4..df3cabf54206 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -3595,15 +3595,17 @@ static int shmem_xattr_handler_set(const struct xattr_handler *handler,
> size_t size, int flags)
> {
> struct shmem_inode_info *info = SHMEM_I(inode);
> - int err;
> + struct simple_xattr *old_xattr;
>
> name = xattr_full_name(handler, name);
> - err = simple_xattr_set(&info->xattrs, name, value, size, flags, NULL);
> - if (!err) {
> + old_xattr = simple_xattr_set(&info->xattrs, name, value, size, flags);
> + if (!IS_ERR(old_xattr)) {
> + simple_xattr_free(old_xattr);
> + old_xattr = NULL;
> inode->i_ctime = current_time(inode);
> inode_inc_iversion(inode);
> }
> - return err;
> + return PTR_ERR(old_xattr);
> }
>
> static const struct xattr_handler shmem_security_xattr_handler = {
> --
> 2.35.3
>
