From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id C3BE7EB64DD for ; Wed, 9 Aug 2023 13:19:39 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 440326B0071; Wed, 9 Aug 2023 09:19:39 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 3F0C16B0072; Wed, 9 Aug 2023 09:19:39 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2B9D06B0075; Wed, 9 Aug 2023 09:19:39 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 1B7406B0071 for ; Wed, 9 Aug 2023 09:19:39 -0400 (EDT) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id A07621A0EFD for ; Wed, 9 Aug 2023 13:19:38 +0000 (UTC) X-FDA: 81104623236.12.6077037 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf25.hostedemail.com (Postfix) with ESMTP id B5575A0010 for ; Wed, 9 Aug 2023 13:19:35 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=g4RJg9VU; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf25.hostedemail.com: domain of cem@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=cem@kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1691587175; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=exDFmKprEee2T+Nif9Q+uDrH0vojtRBobVhwHnsgicA=; b=AWZm3XcLzWGICzppwXpA2JXH89hD0bCi8fwlCDx0UFqevZXSe0okr1NE1AWSZQAyZLYjxq 2P0lwXEv4m1LH/7aNGkqXMKdVc4AypTSrVOf7CMGyDNJQrtRISwYleciQXD9kwDh9kv1dH sHAEEkU2M/cjeYI+rXIc/IIHlIuWgTc= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=g4RJg9VU; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf25.hostedemail.com: domain of cem@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=cem@kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1691587175; a=rsa-sha256; cv=none; b=sOCYR8vyV1MbdMXuFbqM2pBav6w6YOa1pVjWyPZXG1G3WXOvHm2flKk2fCwODSUeamaVBG miMgcz5tb7ATmb8iwMUPyI/XZzgTAJPBQffuCxP50AYTQdQqCRlDvDBBxZFm5hqB4cNUqG 8pWh8PCZ6D75cDgm8eYuyGe6LzSLb/I= Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id C3DA863A77; Wed, 9 Aug 2023 13:19:34 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3C5E7C433C8; Wed, 9 Aug 2023 13:19:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1691587174; bh=2Jx84I6Iqqzjl47tuP7H2iTV9+YSx98wOoXS84LW4Nc=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=g4RJg9VUNEEWFmxsBgPGl6SIZnHbT8eabNmPxk+mUJi+VuxbsDda1NsNPgT9h0pBK ZzI/Wuker25Lt6urf0ImCYd180PM5tORhL/SKMkaDxwiwYnoJZ7kurVOABzocmv4rr /2TqMB1pab8n8zxSgcLToK96NxjkTWHEBr5ZaON+0XKfqSdfztEJ4Cqp0wdDCCgwH3 frFmewiACCmbhcnTxcPD/NSRcMiGgrxYi4cdKGFwuSRf0S9/ObkiJE+ryuzLI5cONe x1/0+04LNP1VZx03ch0nCGHcesYhOL/pH3VnVAFPzWBvYk60A25jM5rh7Oic4Ax9xG eL1S/zHKFCdTg== Date: Wed, 9 Aug 2023 15:19:25 +0200 From: Carlos Maiolino To: Hugh Dickins Cc: Christian Brauner , Andrew Morton , Oleksandr Tymoshenko , Jeff Layton , Chuck Lever , Jan Kara , Miklos Szeredi , Daniel Xu , Chris Down , Tejun Heo , Greg Kroah-Hartman , Matthew Wilcox , Christoph Hellwig , Pete Zaitcev , Helge Deller , Topi Miettinen , Yu Kuai , 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 Message-ID: <20230809131925.e23mqr762ybvcyad@andromeda> References: <158c6585-2aa7-d4aa-90ff-f7c3f8fe407c@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <158c6585-2aa7-d4aa-90ff-f7c3f8fe407c@google.com> X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: B5575A0010 X-Stat-Signature: bhz3mdqtsyur7rgq1xabyhxree1ijgus X-Rspam-User: X-HE-Tag: 1691587175-626270 X-HE-Meta: U2FsdGVkX1/bPZ8b7S3sJKRSQh6RszPnzxtXxV8Pfuug+7gqMFXUIpFPixK5tzuQSU4pwyHu1hbWbjo+Yt55wZ8jYDidd4YT92A5XkJNVHuQUGEAGR8RQ3DO550xfNAyA5GD1xcjrLHkwR9QgIVZdyvgUaNq/iBibFYquSYRmUKUZqiYLmRuPiFLjHaE0WZfdUPj4h5oiG3d07m3PTk1pnDpLaxYfklYX1G25zUfVk7WXP3me5hbn49SQ9SHiD6/mIA2wmQqjXdCkQA8fl6UaHQRo3W3bQyrpqex/kxNKTKk6nXttSHL7vKO0Rl7Tl/h0wBkx8CZdsWbaA+RHNgxgSDigAd3i7JIgtk0AyX9x8Fta/ZfRs3e4LxSkmz3YiMztRC2b+wCmU7wakLAzB+x5ydi+tiwmlXTB/mxjFA945k4b3ZWPE5b4vXkEiqojTby14a2XmX87Y8UDT6vv3pEabvI4Nc3xqPDjWKBFqJtLudmHPTZKYBMBT7R371Y8m2JP0X51mX6G+qB2ZJKSatdXeFbjfRB58jfjA66I0AxUWbEHx6/WYcPn7iY2VFlaqjfo//HnY9OQyr5PhMof3HUWGi6MrYHB+D7n7tNy8zwuOLObjvU+5pyVN1BzupeFMl4OVUA7gt5b8G9b3U7KNnp7XVWvjJyDNktYtbzF81I4UTFbWYEYi0mumRq2ZgNcyNPuBkXUSi6T7/onLc69zgNUiH+SGaXV7dCIjTLuytFasoJTO0LIjJNFObpBtVb6KqJpINWQCZ2ONyWevDlvPghcBfr/+qhOXtw56pGS+quTIRB4L+NKaOS/cZA+Z4rUcsu/8pWgtByTkmkxBvBeRAyvMM35BjBrg+hQJei+WpwDps0/cMiOkOh6l0gvpct7YqEQyi21KtOERxUz8tasDVppIaF2F/AWl+mRpLDskiw5yS63NboYxU3elToNsNJg370ZHDxr76Ipma5BwM6ybi BhfGmdrh Q3IcHTjqp6Kp8Mndeh8KmgwUlyrUcogc0pFo0jLanxNHf7BXuIn4IXO0Whxa+PqHLujhDFOVJm1YdgY/fqk1RhQYJErf57ho7tV27KHuenMLuq4VlYWoIoA7J0OvYMqB0URsvUeM6yAlzl/GIE41mEvlIrQtW+GGIb6YSkDrxJAgBwfjh7tJod4mRgMl8wZNPD3A6eBree0fM/SI3Lf88nFppnuVFwUjiyHzZLF/nRya1mqwMjCF8gZZ5XN8DBKSKDq+fm24J1KcSdeeLdxbz6KQskVyHy1HucSzlfcDyk67ksus2tdG2HCs3efUieglG5SnJDDp08qO3az8H+Cr1WuPwR19NM154COw09M0wHpFisuBn8KhjOwo8YImq0BfZG9v/m6/GdsBzV7d6QOqJgEAxTxehS+oJwKVr X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: 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 > Signed-off-by: Hugh Dickins > --- > 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 >