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 D666DC36010 for ; Fri, 4 Apr 2025 12:59:15 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 080216B0024; Fri, 4 Apr 2025 08:59:14 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 02E966B0026; Fri, 4 Apr 2025 08:59:13 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E38D16B0027; Fri, 4 Apr 2025 08:59:13 -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 C3E646B0024 for ; Fri, 4 Apr 2025 08:59:13 -0400 (EDT) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id BFF6DAE304 for ; Fri, 4 Apr 2025 12:59:14 +0000 (UTC) X-FDA: 83296367028.08.B745D57 Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) by imf24.hostedemail.com (Postfix) with ESMTP id BEC6E18000A for ; Fri, 4 Apr 2025 12:59:11 +0000 (UTC) Authentication-Results: imf24.hostedemail.com; dkim=none; spf=pass (imf24.hostedemail.com: domain of jonathan.cameron@huawei.com designates 185.176.79.56 as permitted sender) smtp.mailfrom=jonathan.cameron@huawei.com; dmarc=pass (policy=quarantine) header.from=huawei.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1743771552; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=1BxxwrDGMwQsjdfPWhNUbHLM6UoUkgQfeRL3aJj3OF0=; b=RgOYdQ0WJ4WcsmDmzMpeV2NuzmIy+v5UXBbj+92RM9oFA+BAsFFVtk7Emtm6n7WeBAOaZo o/j14rPA9OUuUvYtUyMnPYe0g72MMvmKeUTCSLwSi49AtKVC+c/tLTQUdymOirTscqg0Wr Z79fTZmZpVJcpLcKtoJVQd57kOqLz2w= ARC-Authentication-Results: i=1; imf24.hostedemail.com; dkim=none; spf=pass (imf24.hostedemail.com: domain of jonathan.cameron@huawei.com designates 185.176.79.56 as permitted sender) smtp.mailfrom=jonathan.cameron@huawei.com; dmarc=pass (policy=quarantine) header.from=huawei.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1743771552; a=rsa-sha256; cv=none; b=aBD1U66CzJ97YQOJXQrIKLq00wOUDgDze+fjV8wzciXcukC+RmEC/6Mkd760v2ceupf3Vi GLnG3oUF6UnBMCt72sE/Gr2DKJuHwTFuHOunXvoyJT+Uq8oWjWf9oymBNr8n7HndkK5w0P pwPBJNYjqgTjxnsztlIbhZGjFPzauls= Received: from mail.maildlp.com (unknown [172.18.186.231]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4ZTdr02jL4z6K9DN; Fri, 4 Apr 2025 20:55:28 +0800 (CST) Received: from frapeml500008.china.huawei.com (unknown [7.182.85.71]) by mail.maildlp.com (Postfix) with ESMTPS id EC187140557; Fri, 4 Apr 2025 20:59:08 +0800 (CST) Received: from localhost (10.203.177.66) by frapeml500008.china.huawei.com (7.182.85.71) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.39; Fri, 4 Apr 2025 14:59:08 +0200 Date: Fri, 4 Apr 2025 13:59:06 +0100 From: Jonathan Cameron To: Rakie Kim CC: , , , , , , , , , , , , Subject: Re: [PATCH v6 1/3] mm/mempolicy: Fix memory leaks in weighted interleave sysfs Message-ID: <20250404135906.0000308e@huawei.com> In-Reply-To: <20250404074623.1179-2-rakie.kim@sk.com> References: <20250404074623.1179-1-rakie.kim@sk.com> <20250404074623.1179-2-rakie.kim@sk.com> X-Mailer: Claws Mail 4.3.0 (GTK 3.24.42; x86_64-w64-mingw32) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.203.177.66] X-ClientProxiedBy: lhrpeml500005.china.huawei.com (7.191.163.240) To frapeml500008.china.huawei.com (7.182.85.71) X-Rspamd-Queue-Id: BEC6E18000A X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: ouzhgk7bfrm34axsxfgzsjtpyq54shkt X-HE-Tag: 1743771551-603041 X-HE-Meta: U2FsdGVkX19p6u0RXkLPcDf3BdIskKZ3RZWl0S22ex0pw4HzBvtTUrLjk8RPn1/tIKTKaYR8vVlmpHQetKL0FYe3+4WhiiqNMiZzWHAX8wwCMdteb71wKTmf4z7MfZq4/CzJgM+P9l+/e0FG+dO78QLVHsQ5FeUwXzkxVd06Vwu59GL/xMEBdZ86E5ouHX8r9k2/hmJKDYoVQbs8fD++E+E+ClNJ4fBfvsrCjSQS5SXIBMlkiAwjpAsMTRwuxUHxTAQcENLylp5JDx9wanm41cAGJ/QCI3lDKgYm4kDtba3VsSM9E03toBDZioNz+5TOQds88SWd7uOv0Ij9gmwWeSmM6yIwthjJz4zw0Ot1FiFpdKy7F1Tyi+GmUq+xpL5Igol9BNMV0To98CyxPPs+nxYZjpoS+XMw6ikyqEj8EfEvCYrlRwYWzhtN6Fa9tP1cxDWxgzU0bToGL7/FxDdmw/XneGIVCK1Wp49G7qOXHSZj92bqCgU+k5pqaN/gPf0Ky2D6Ge+ppkzfKvTOtRW/xD6q3/6MdlzJPBOvbWBet4qEemnBrbhE/gYWvqd75VdhAh1SMro9qAQmiSpGEKqqszAmJcZCRTxN2lPi+McPsqVHG1RKHGX6wRegZuuCdpH9wWrSM2JUvM/Jl47A7PwXCnbjJHRGlj3SxwEcKAa+Y3EJSlCer+ENbMtYYszemHc9oZPM/lhpmIZ130aeASi+59LTWebCF9LOkruAQ9ZJkeCwdf2WnmAX0dFs5JohpjSBxWhSck7QTLNUTojqYkmhOZTCRSmJRwnKD8fG/r63sbjfOCrxrylJLQIkoQooO/2mpCyE4XgpmiAUGwx3js+otbauJQITdnN2DgXLKcMoapHJyJ6g/L6HwnIQjtcSNug206M536x1SeV5PEMDsSHVq2SgVhsMkQQsM28FPcFGLdIOQcadv8cRY9A865YKc+Mr7HcfhoJCN+s2RRtWYfZ X01ztF3z EUToKkX5jvUV2mF8/x9NNgMTTSNgo2OBTe31647NmYeOO/CXUUAX5xcEVYFYn2ssAByiA0/GZ+mNEPWjbDOcS63qLxyQ9ZkMB5ECEAja6WkpP0vVXsadmJkiVa1YDoCWn2LOazB9gLMcZGxqzlnlpcnkqMPcJG582T5NwK044LMs0zXa0jDPGTk3iBYcpBclSpcp85SSNFlLHTjwX3TiTVf8ESLEpVSNjgyJzgkgGvchP7zbg3IEdRWak96A+8qNGtPK1OS23ZQ65R0CFOEEUXnnLBvOYkxsUrDyM6maHT6S31GmerrFF+CsAQm99MTRzfpK7e1xUJgY6isQzbg4dFH2zMyMDo8uJWs+PBrWdC9B0+4E= 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 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. > --- > 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. > + } > + > 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. > + 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. > return err; > } >