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 60386C36010 for ; Mon, 7 Apr 2025 09:38:09 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 181816B000A; Mon, 7 Apr 2025 05:38:08 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 157026B000C; Mon, 7 Apr 2025 05:38:08 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 021266B000D; Mon, 7 Apr 2025 05:38:07 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id DA8F16B000A for ; Mon, 7 Apr 2025 05:38:07 -0400 (EDT) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 46975BAE90 for ; Mon, 7 Apr 2025 09:38:08 +0000 (UTC) X-FDA: 83306746656.11.A5B6E3B Received: from invmail4.hynix.com (exvmail4.hynix.com [166.125.252.92]) by imf14.hostedemail.com (Postfix) with ESMTP id 6F20D10000B for ; Mon, 7 Apr 2025 09:38:06 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=none; dmarc=none; spf=pass (imf14.hostedemail.com: domain of rakie.kim@sk.com designates 166.125.252.92 as permitted sender) smtp.mailfrom=rakie.kim@sk.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1744018686; a=rsa-sha256; cv=none; b=lBpqOc5xeVbsrU42yCGVHUbrLugIH25U6k2E2+XnRzUqpIhA16VlFQyYpagBgYL74VDnPp edLlm4HKRtLt/Hz+gZH8QFsyFmeT1R8qNIHBzxBjhHWqH22pEtAvLErri4E2jMKCS5qGPa PoG7xWdAURzN7g59J/zKtaZOvbYdWoc= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=none; dmarc=none; spf=pass (imf14.hostedemail.com: domain of rakie.kim@sk.com designates 166.125.252.92 as permitted sender) smtp.mailfrom=rakie.kim@sk.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1744018686; 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-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=6ZmMe380PQ2moOWGlB1XvjbE3c1zurjKnGrZZORzATc=; b=F8MmCA5/n76S3jbICk1ELyF2iEEkSpL7Kv9D2TdsZguza3VhLFDFWf8PK+3UuKKUdfDGRU LLT+xUc0xHp3uZJobbkfaQNSB9RdHDO8bvs8Fz4/r9aQRUbUlIEJrdFLsV04chfWtCA25P Ni6febINTCdhXnnTMxQM8WwSR0EVkfU= X-AuditID: a67dfc5b-669ff7000002311f-f8-67f39cfdb816 From: Rakie Kim To: Jonathan Cameron Cc: akpm@linux-foundation.org, gourry@gourry.net, linux-mm@kvack.org, linux-kernel@vger.kernel.org, linux-cxl@vger.kernel.org, joshua.hahnjy@gmail.com, dan.j.williams@intel.com, ying.huang@linux.alibaba.com, david@redhat.com, osalvador@suse.de, kernel_team@skhynix.com, honggyu.kim@sk.com, yunjeong.mun@sk.com, Rakie Kim Subject: Re: [PATCH v6 1/3] mm/mempolicy: Fix memory leaks in weighted interleave sysfs Date: Mon, 7 Apr 2025 18:37:53 +0900 Message-ID: <20250407093800.417-1-rakie.kim@sk.com> X-Mailer: git-send-email 2.48.1.windows.1 In-Reply-To: <20250404135906.0000308e@huawei.com> References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrGLMWRmVeSWpSXmKPExsXC9ZZnoe7fOZ/TDc4c5rCYs34Nm8X0qRcY Lb6u/8Vs8fPucXaLVQuvsVkc3zqP3eL8rFMsFpd3zWGzuLfmP6vFmWlFFqvXZDhwe+ycdZfd o7vtMrtHy5G3rB6L97xk8tj0aRK7x4kZv1k8dj609Hi/7yqbx+bT1R6fN8kFcEVx2aSk5mSW pRbp2yVwZaz8+JGt4IVpxabTj5gaGFdqdjFyckgImEg8vtbE1MXIAWbv2xoOYrIJKEkc2xsD UiEiYCTx7sYkxi5GLg5mgbdMEg9+7AErFxYIl5i7wB/EZBFQlZh4iAmknFfAWGLD5MOsEMM1 JRou3QOLcwoYSrS8XQgWFxLgkXi1YT8jRL2gxMmZT1hAbGYBeYnmrbOZQVZJCHxmk3jbD5GQ EJCUOLjiBssERv5ZSHpmIelZwMi0ilEoM68sNzEzx0QvozIvs0IvOT93EyMw9JfV/onewfjp QvAhRgEORiUe3h1un9OFWBPLiitzDzFKcDArifBanvqULsSbklhZlVqUH19UmpNafIhRmoNF SZzX6Ft5ipBAemJJanZqakFqEUyWiYNTqoHRJsTnCFPN6StJtarvboSl6V4S//6gQ+VYYv+h n18e9WW+u+bY053Mw2ty7GqMMhv3jIs5xToPeTYH3152hXPTdZ4G3g/fRDY0SGa8jWa8PkXN 8lHgEqU7csdXfLikFi1o4HVe7OXe3I2fdguYv3SNOCjn2Gblobmd5bt0QKD6zLjrZjFs5m5K LMUZiYZazEXFiQBnpm4beQIAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrALMWRmVeSWpSXmKPExsXCNUNNS/fPnM/pBo2NVhZz1q9hs5g+9QKj xdf1v5gtft49zm7x+dlrZotVC6+xWRzfOo/d4vDck6wW52edYrG4vGsOm8W9Nf9ZLc5MK7I4 dO05q8XqNRkWv7etYHPg99g56y67R3fbZXaPliNvWT0W73nJ5LHp0yR2jxMzfrN47Hxo6fF+ 31U2j2+3PTwWv/jA5LH5dLXH501yATxRXDYpqTmZZalF+nYJXBkrP35kK3hhWrHp9COmBsaV ml2MHBwSAiYS+7aGg5hsAkoSx/bGdDFycogIGEm8uzGJsYuRi4NZ4C2TxIMfe5hAaoQFwiXm LvAHMVkEVCUmHmICKecVMJbYMPkwK4gtIaAp0XDpHlicU8BQouXtQrC4kACPxKsN+xkh6gUl Ts58wgJiMwvISzRvnc08gZFnFpLULCSpBYxMqxhFMvPKchMzc0z1irMzKvMyK/SS83M3MQLD fVntn4k7GL9cdj/EKMDBqMTDe6PxU7oQa2JZcWXuIUYJDmYlEV7LU0Ah3pTEyqrUovz4otKc 1OJDjNIcLErivF7hqQlCAumJJanZqakFqUUwWSYOTqkGxr1d/LqBmr/mX34hYpvDtW7xR1mv p50lM54t2/DMeHlejGjcCpFsnuT/Jm11gdGsrJm/ZnP/D5jFkmIhmXvmErd1uORTr557XAf4 rsX/ig5jmN14r9n+o/6CCQUduzPaJkQ+mP16nXH5i8Rf4llKXnwBHEd99P6Efp9isup59o3M 3bP2aia8U2Ipzkg01GIuKk4EAJgVBiZzAgAA X-CFilter-Loop: Reflected X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 6F20D10000B X-Stat-Signature: u8sr84rpry8dnqy1mosbj7q3wxnsk343 X-Rspam-User: X-HE-Tag: 1744018686-774179 X-HE-Meta: U2FsdGVkX19F+qtsRITo1PV7b5IgOBPdWZYlGR6GMHZz+xQAeckV2INlMBP9bZ6Z1YPcOKcGlRmwBR/xI1siXrlaG6zgI/itIk8eWlVJMmSM0F5iLwwGNkiJ8ln6jCR6Y0NaKG/PGhnr9C9zWqswpHVhFZ38ZHImMd33XUD/H4vOzeLnqgitEe936yy5d4ZWAMSXi6azWJFNo7ommwUPpSUt15qdEL5bfeXlde5J281cU8tJrcRJVsJ7/eCY9P1byupBzqOFnOIzRV8xfankldcYhX67FxOJW84mMusffbpcp+P2dHNxd1HbNm9MpGzk+DL48eecuknSW5uPnUfzd/D5fOxhi0LRzQhEAGN66tUEJup3/8h6l6R/Wj7LsUtOKBZBzEZPTZ1eUYAQ/fiAHYRHh02SKurqexhpLkE9292ENMkDTJQuHg4a21fLIBz0UIB/u2BpTUBYOJiMnPa2JBxuNCme7gP1EeBXDVOcOou6N62Vv6AL0eGS8tnOifqJAh20M5hlKJ2VdO1VwU1oeXOSu2xqwYYdVdhXnpzKBGsU1RTXGKrjBXsgEhxHkf+2r7fBuOlhjQKCi0h8KjKvjAZTOVPiP+dBxJR2x8wr7Mx4qLKgzs/yorJBa03Hz/BST3dbHx/CgWv47jPaHyFuZF2Lyz1+34Jv0xfGWrzFYwp3iWQVXI/ApZmpIsIbE3MjoFI10o32yfYEpEqHw7aFuiyuawkkAGD/Dq/leRk7CRZRfm1iaYLtzJ+eIxjKR2fmGDu/Y1aomtMo0B+6RjvnyxvrQAoyCAinfRblEJv1XYYtPJyef7K8Ta1iEa9rbHzQ 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: List-Subscribe: List-Unsubscribe: On Fri, 4 Apr 2025 13:59:06 +0100 Jonathan Cameron wrote: > On Fri, 4 Apr 2025 16:46:19 +0900 > Rakie Kim wrote: > > > Memory leaks occurred when removing sysfs attributes for weighted > > interleave. Improper kobject deallocation led to unreleased memory > > when initialization failed or when nodes were removed. > > > > This patch resolves the issue by replacing unnecessary `kfree()` > > calls with proper `kobject_del()` and `kobject_put()` sequences, > > ensuring correct teardown and preventing memory leaks. > > > > By explicitly calling `kobject_del()` before `kobject_put()`, > > the release function is now invoked safely, and internal sysfs > > state is correctly cleaned up. This guarantees that the memory > > associated with the kobject is fully released and avoids > > resource leaks, thereby improving system stability. > > > > Fixes: dce41f5ae253 ("mm/mempolicy: implement the sysfs-based weighted_interleave interface") > > Signed-off-by: Rakie Kim > > Signed-off-by: Honggyu Kim > > Signed-off-by: Yunjeong Mun > > Reviewed-by: Gregory Price > I've pretty much forgotten earlier discussions so apologies if I revisit > old ground! > > The fix is fine I think. But the resulting code structure > could be improved, without (I think) complicating what is here by much. > Thank you for your response regarding this patch. I appreciate your willingness to revisit this code and share your thoughts. Please feel free to provide suggestions anytime. > > --- > > mm/mempolicy.c | 64 +++++++++++++++++++++++++++----------------------- > > 1 file changed, 34 insertions(+), 30 deletions(-) > > > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > > index bbaadbeeb291..af3753925573 100644 > > --- a/mm/mempolicy.c > > +++ b/mm/mempolicy.c > > @@ -3448,7 +3448,9 @@ static void sysfs_wi_release(struct kobject *wi_kobj) > > > > for (i = 0; i < nr_node_ids; i++) > > sysfs_wi_node_release(node_attrs[i], wi_kobj); > > - kobject_put(wi_kobj); > > + > > + kfree(node_attrs); > > + kfree(wi_kobj); > > } > > > > static const struct kobj_type wi_ktype = { > > @@ -3494,15 +3496,22 @@ static int add_weighted_interleave_group(struct kobject *root_kobj) > > struct kobject *wi_kobj; > > int nid, err; > > > > - wi_kobj = kzalloc(sizeof(struct kobject), GFP_KERNEL); > > - if (!wi_kobj) > > + node_attrs = kcalloc(nr_node_ids, sizeof(struct iw_node_attr *), > > + GFP_KERNEL); > > + if (!node_attrs) > > return -ENOMEM; > > > > + wi_kobj = kzalloc(sizeof(struct kobject), GFP_KERNEL); > > + if (!wi_kobj) { > > + err = -ENOMEM; > > + goto node_out; > As this is only place where we do kfree(node_attrs) > why not just do that here and return directly. Regarding your suggestion: Is the following change what you had in mind? wi_kobj = kzalloc(sizeof(struct kobject), GFP_KERNEL); if (!wi_kobj) { kfree(node_attrs); return -ENOMEM; } I will apply this change accordingly. > > > > + } > > + > > err = kobject_init_and_add(wi_kobj, &wi_ktype, root_kobj, > > "weighted_interleave"); > > if (err) { > > - kfree(wi_kobj); > > - return err; > > + kobject_put(wi_kobj); > > + goto err_out; > > } > > > > for_each_node_state(nid, N_POSSIBLE) { > > @@ -3512,9 +3521,18 @@ static int add_weighted_interleave_group(struct kobject *root_kobj) > > break; > > } > > } > > - if (err) > > + if (err) { > > + kobject_del(wi_kobj); > > kobject_put(wi_kobj); > > For this and the one above, a unified exit kind of makes sense as > can do two labels and have the put only once. > > If not, why not move this up into the loop and return directly? > If you move to an error handling block > > err_del_obj: > kobject_del(wi_kobj); > err_put_obj: > kobject_put(wi_kobj); > > then you could also do the goto from within that loop. > As for your second suggestion about restructuring the error handling, you are right that having unified labels helps make the flow cleaner. I will update the code to use: err = kobject_init_and_add(wi_kobj, &wi_ktype, root_kobj, "weighted_interleave"); if (err) goto err_put_kobj; for_each_node_state(nid, N_POSSIBLE) { err = add_weight_node(nid, wi_kobj); if (err) { pr_err("failed to add sysfs [node%d]\n", nid); goto err_del_kobj; } } err_del_kobj: kobject_del(wi_kobj); err_put_kobj: kobject_put(wi_kobj); return err; This structure keeps error handling more consistent and avoids redundancy. > > > + goto err_out; > > + } > > + > > return 0; > > + > > +node_out: > > + kfree(node_attrs); > > +err_out: > > + return err; > > } > > > > static void mempolicy_kobj_release(struct kobject *kobj) > > @@ -3528,7 +3546,6 @@ static void mempolicy_kobj_release(struct kobject *kobj) > > mutex_unlock(&iw_table_lock); > > synchronize_rcu(); > > kfree(old); > > - kfree(node_attrs); > > kfree(kobj); > > } > > > > @@ -3542,37 +3559,24 @@ static int __init mempolicy_sysfs_init(void) > > static struct kobject *mempolicy_kobj; > > > > mempolicy_kobj = kzalloc(sizeof(*mempolicy_kobj), GFP_KERNEL); > > - if (!mempolicy_kobj) { > > - err = -ENOMEM; > > - goto err_out; > > - } > > - > > - node_attrs = kcalloc(nr_node_ids, sizeof(struct iw_node_attr *), > > - GFP_KERNEL); > > - if (!node_attrs) { > > - err = -ENOMEM; > > - goto mempol_out; > > - } > > + if (!mempolicy_kobj) > > + return -ENOMEM; > > > > err = kobject_init_and_add(mempolicy_kobj, &mempolicy_ktype, mm_kobj, > > "mempolicy"); > > if (err) > > - goto node_out; > > + goto err_out; > goto err_put_kobj; > or something like that. > > > > > err = add_weighted_interleave_group(mempolicy_kobj); > > - if (err) { > > - pr_err("mempolicy sysfs structure failed to initialize\n"); > > - kobject_put(mempolicy_kobj); > > - return err; > > - } > > + if (err) > > + goto err_del; > > > > - return err; > > -node_out: > > - kfree(node_attrs); > > -mempol_out: > > - kfree(mempolicy_kobj); > > + return 0; > > + > > +err_del: > > + kobject_del(mempolicy_kobj); > > err_out: > > - pr_err("failed to add mempolicy kobject to the system\n"); > > + kobject_put(mempolicy_kobj); > If we keep an err_out, usual expectation is all it does is return > + maybe print a message. We'd not expect a put. Lastly, I agree with your point about `err_out`. I will revise it to use `err_del_kobj` and `err_put_kobj` as needed. Thanks again for the detailed review. Rakie > > > return err; > > } > > >