From: Carlos Maiolino <cem@kernel.org>
To: Hugh Dickins <hughd@google.com>
Cc: Christian Brauner <brauner@kernel.org>,
Andrew Morton <akpm@linux-foundation.org>,
Oleksandr Tymoshenko <ovt@google.com>,
Jeff Layton <jlayton@kernel.org>,
Chuck Lever <chuck.lever@oracle.com>, Jan Kara <jack@suse.cz>,
Miklos Szeredi <miklos@szeredi.hu>, Daniel Xu <dxu@dxuuu.xyz>,
Chris Down <chris@chrisdown.name>, Tejun Heo <tj@kernel.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Matthew Wilcox <willy@infradead.org>,
Christoph Hellwig <hch@infradead.org>,
Pete Zaitcev <zaitcev@redhat.com>, Helge Deller <deller@gmx.de>,
Topi Miettinen <toiwoton@gmail.com>, Yu Kuai <yukuai3@huawei.com>,
linux-fsdevel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH vfs.tmpfs 1/5] xattr: simple_xattr_set() return old_xattr to be freed
Date: Wed, 9 Aug 2023 15:19:25 +0200 [thread overview]
Message-ID: <20230809131925.e23mqr762ybvcyad@andromeda> (raw)
In-Reply-To: <158c6585-2aa7-d4aa-90ff-f7c3f8fe407c@google.com>
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@redhat.com>
> Signed-off-by: Hugh Dickins <hughd@google.com>
> ---
> 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
>
next prev parent reply other threads:[~2023-08-09 13:19 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-09 4:28 [PATCH vfs.tmpfs 0/5] tmpfs: user xattrs and direct IO Hugh Dickins
2023-08-09 4:30 ` [PATCH vfs.tmpfs 1/5] xattr: simple_xattr_set() return old_xattr to be freed Hugh Dickins
2023-08-09 9:21 ` Jan Kara
2023-08-09 11:37 ` Christian Brauner
2023-08-09 13:19 ` Carlos Maiolino [this message]
2023-08-09 4:32 ` [PATCH vfs.tmpfs 2/5] tmpfs: track free_ispace instead of free_inodes Hugh Dickins
2023-08-09 9:33 ` Jan Kara
2023-08-09 13:29 ` Carlos Maiolino
2023-08-09 4:33 ` [PATCH vfs.tmpfs 3/5] tmpfs,xattr: enable limited user extended attributes Hugh Dickins
2023-08-09 9:50 ` Jan Kara
2023-08-09 13:52 ` Carlos Maiolino
2023-08-09 4:34 ` [PATCH vfs.tmpfs 4/5] tmpfs: trivial support for direct IO Hugh Dickins
2023-08-09 9:54 ` Jan Kara
2023-08-09 13:41 ` Christoph Hellwig
2023-08-10 23:41 ` Darrick J. Wong
2023-08-11 6:16 ` Hugh Dickins
2023-08-11 8:34 ` Christoph Hellwig
2023-08-11 6:08 ` Hugh Dickins
2023-08-11 6:27 ` [PATCH vfs.tmpfs v2 " Hugh Dickins
2023-08-11 8:35 ` Christoph Hellwig
2023-08-11 8:56 ` Jan Kara
2023-08-11 11:00 ` (subset) " Christian Brauner
2023-08-09 4:36 ` [PATCH vfs.tmpfs 5/5] mm: invalidation check mapping before folio_contains Hugh Dickins
2023-08-09 9:27 ` Jan Kara
2023-08-09 6:45 ` [PATCH vfs.tmpfs 0/5] tmpfs: user xattrs and direct IO Christian Brauner
2023-08-09 11:33 ` Christian Brauner
2023-08-10 5:50 ` Hugh Dickins
2023-08-10 10:07 ` Christian Brauner
2023-08-21 17:39 ` [PATCH vfs.tmpfs] tmpfs,xattr: GFP_KERNEL_ACCOUNT for simple xattrs Hugh Dickins
2023-08-21 17:57 ` Jan Kara
2023-08-22 8:58 ` (subset) " Christian Brauner
2023-08-10 23:23 ` [PATCH vfs.tmpfs 0/5] tmpfs: user xattrs and direct IO Pete Zaitcev
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230809131925.e23mqr762ybvcyad@andromeda \
--to=cem@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=brauner@kernel.org \
--cc=chris@chrisdown.name \
--cc=chuck.lever@oracle.com \
--cc=deller@gmx.de \
--cc=dxu@dxuuu.xyz \
--cc=gregkh@linuxfoundation.org \
--cc=hch@infradead.org \
--cc=hughd@google.com \
--cc=jack@suse.cz \
--cc=jlayton@kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=miklos@szeredi.hu \
--cc=ovt@google.com \
--cc=tj@kernel.org \
--cc=toiwoton@gmail.com \
--cc=willy@infradead.org \
--cc=yukuai3@huawei.com \
--cc=zaitcev@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).