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 E8EBBC369BD for ; Thu, 17 Apr 2025 01:50:35 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 757896B012E; Wed, 16 Apr 2025 21:50:34 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 6DFAC6B012F; Wed, 16 Apr 2025 21:50:34 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 559DF28001A; Wed, 16 Apr 2025 21:50:34 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 352326B012E for ; Wed, 16 Apr 2025 21:50:34 -0400 (EDT) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id BFE281411D8 for ; Thu, 17 Apr 2025 01:50:19 +0000 (UTC) X-FDA: 83341855758.04.6E26797 Received: from invmail4.hynix.com (exvmail4.skhynix.com [166.125.252.92]) by imf06.hostedemail.com (Postfix) with ESMTP id 1E519180005 for ; Thu, 17 Apr 2025 01:50:16 +0000 (UTC) Authentication-Results: imf06.hostedemail.com; dkim=none; spf=pass (imf06.hostedemail.com: domain of rakie.kim@sk.com designates 166.125.252.92 as permitted sender) smtp.mailfrom=rakie.kim@sk.com; dmarc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1744854618; 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=lBBYptPBNFBKgJd0I0QuEgqt69um45M2urITfPTylB4=; b=BiAtgy9sJebMPWDG9jUPEOgAqVAEGS1fx5NugfxVPuwB1/Tzqo2D1DNSQT79kHGeDxUHhj girWDdpm3rMbCVvTvzAZH7IE2FsC3VjsWmCdnlkKp8VGiSF8eMmZqno4EUnBXls4g9Ip01 ++cmabvzOijnpAxaUkq+USzOvvR2m+k= ARC-Authentication-Results: i=1; imf06.hostedemail.com; dkim=none; spf=pass (imf06.hostedemail.com: domain of rakie.kim@sk.com designates 166.125.252.92 as permitted sender) smtp.mailfrom=rakie.kim@sk.com; dmarc=none ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1744854618; a=rsa-sha256; cv=none; b=OLjw1tIYzlO0DeeZLrFl6RSeJ0GW0ic/jPNT5S36gBMPqBvO90WIfaUcQSryfjPhpmqitG IeMhCpAyK0sDzeqaNrVuxEzFAb/yA4Tvc5T6SsmrQlVnotQZ490+ba4TMqfpNE2TLy3Zoi 43ldsuhW3HgIk+sQqnaGBTtmwzeB2Y4= X-AuditID: a67dfc5b-681ff7000002311f-18-68005e54eb85 From: Rakie Kim To: Dan Williams Cc: gourry@gourry.net, linux-mm@kvack.org, linux-kernel@vger.kernel.org, linux-cxl@vger.kernel.org, joshua.hahnjy@gmail.com, ying.huang@linux.alibaba.com, david@redhat.com, Jonathan.Cameron@huawei.com, osalvador@suse.de, kernel_team@skhynix.com, honggyu.kim@sk.com, yunjeong.mun@sk.com, rakie.kim@sk.com, akpm@linux-foundation.org Subject: Re: [PATCH v8 1/3] mm/mempolicy: Fix memory leaks in weighted interleave sysfs Date: Thu, 17 Apr 2025 10:49:38 +0900 Message-ID: <20250417015009.650-1-rakie.kim@sk.com> X-Mailer: git-send-email 2.48.1.windows.1 In-Reply-To: <6800270845306_1302d294a4@dwillia2-xfh.jf.intel.com.notmuch> References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrBLMWRmVeSWpSXmKPExsXC9ZZnkW5IHEOGwa4vNhZz1q9hs5g+9QKj xdf1v5gtft49zm6xauE1NovjW+exW5yfdYrF4vKuOWwW99b8Z7U4M63IYvWaDAduj52z7rJ7 dLddZvdoOfKW1WPxnpdMHps+TWL3ODHjN4vHzoeWHu/3XWXz2Hy62uPzJrkArigum5TUnMyy 1CJ9uwSujBer+Qv2ulY0rP/B1sD4w6SLkZNDQsBEYueGaeww9pW3E1i7GDk42ASUJI7tjQEJ iwhoS0ycc5C5i5GLg1ngCZPE3CNL2UASwgLhErte/wazWQRUJZ41NDKC2LwCxhL3Nl+Fmqkp 0XDpHhOIzSngKXFi8WFWEFtIgEfi1Yb9UPWCEidnPmEBsZkF5CWat84GWyYh8J1NovvcLUaI QZISB1fcYJnAyD8LSc8sJD0LGJlWMQpl5pXlJmbmmOhlVOZlVugl5+duYgSG/7LaP9E7GD9d CD7EKMDBqMTDe2LR/3Qh1sSy4srcQ4wSHMxKIrznzP+lC/GmJFZWpRblxxeV5qQWH2KU5mBR Euc1+laeIiSQnliSmp2aWpBaBJNl4uCUamBkeMz/MG61tu2twj9ic+fe3mEof8LonKJfWs6L G56FyjeaYl/PyvVtdy9Yu9tre7Txy0OlqtuclW5VW5QdD9BZ9Pmy4tSZJX0tKu997aJfSNfx zbMzu+hX8fHv98sXI5iydPL45LbfmXNF4ubCxLZytfPJlXO6KlMERJz19r0rqfulG5PMZ6fE UpyRaKjFXFScCAAg+EhBewIAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrELMWRmVeSWpSXmKPExsXCNUNNSzckjiHD4Mc9A4s569ewWUyfeoHR 4uv6X8wWP+8eZ7f4/Ow1s8WqhdfYLI5vncducXjuSVaL87NOsVhc3jWHzeLemv+sFmemFVkc uvac1WL1mgyL39tWsDnwe+ycdZfdo7vtMrtHy5G3rB6L97xk8tj0aRK7x4kZv1k8dj609Hi/ 7yqbx7fbHh6LX3xg8th8utrj8ya5AJ4oLpuU1JzMstQifbsErowXq/kL9rpWNKz/wdbA+MOk i5GTQ0LAROLK2wmsXYwcHGwCShLH9saAhEUEtCUmzjnI3MXIxcEs8IRJYu6RpWwgCWGBcIld r3+D2SwCqhLPGhoZQWxeAWOJe5uvskPM1JRouHSPCcTmFPCUOLH4MCuILSTAI/Fqw36oekGJ kzOfsIDYzALyEs1bZzNPYOSZhSQ1C0lqASPTKkaRzLyy3MTMHFO94uyMyrzMCr3k/NxNjMCQ X1b7Z+IOxi+X3Q8xCnAwKvHwnlj0P12INbGsuDL3EKMEB7OSCO8583/pQrwpiZVVqUX58UWl OanFhxilOViUxHm9wlMThATSE0tSs1NTC1KLYLJMHJxSDYwPzlQGuve/r9z4yd1zVeOcg08X rgzbpZ099+gkwbLneifarKRYrhtPeGGq9kDFP7Rqw+Uz3dLZNg++lB/gXGC7aFfw0cNlXy3E d17gZWL43J3au9H4lZqrt/0iDY6Tvxj8byxv58z57fWmQ9MqXOK5ZNHnL0vzGP5dKvrVWX7z b8XkCGu1/7lKLMUZiYZazEXFiQD6h9JQdQIAAA== X-CFilter-Loop: Reflected X-Stat-Signature: tgjcjuej6itz6gzewdn7cpynxei74g56 X-Rspamd-Queue-Id: 1E519180005 X-Rspam-User: X-Rspamd-Server: rspam05 X-HE-Tag: 1744854616-244886 X-HE-Meta: U2FsdGVkX1/GjR2+GXE/HjTQm3mstNu6KiLGql12UsfAqGJ26VRTvLB6sa15o7mx/Qf4AoyxqtkQQz6X5wfLsps/23Vr47LE5iEd5n02PHRB0ED+hCKVM6xovGCAD5Xc+ERWMkwD+4Jt5UJvFqPr8b0oosqI7p2rMDDNSqNjhpx4uxnle1p6MqAGwYTiLv72asogw/ChbL17PoB/fVcjEqvLg/RScfCRIiXrPdSGE47lIRZdC5zcwUDUHPfe/8oEFFCrz/Nw+RWrzy3zMviOnkU9/R8rP+c+B6e58IvlfBJOLX+xwJRJbWEueCGCIBiZzqcd9D/K/jJC0IAlSyiNS9gy7oojRLI0Bzoe45TPyupT5Et5CluOC9AngM86Gtw4ywPweRUgU9jsV8xUzgQfh8JT0dnOZcZym7nUYEO992RAUR7sRfkBJua6jm6uja3DM/k0a7eKBsMJWpoyctLEctD2ycP+8TpoZf3Zx0ahKWYmUifAGvhPTVVTSCPLg8q4M4EttKlvbJ86OZXRW2F0sSvRoInS3m3SFZAvtyI4nKsF4+i2Ek8VBbWByvd6sGSz3ShR5+vx3hDQ66zzWSzy2MhFIUzWMCWXDqkOSY3fBs1e3/QdKLoY39J1cZpSU5/XtBVngo4ncjwGiT3pN9+yiykXLMPozygQrqY3pecblhRM4XXQpqmnVHhnwdBGYNGJ9lfOB+Or4EuFB9nhITDPDaZ2m1/aTWgABhlsH6vaDOko6f4KpBae3gx/1nZC4VThk99FI5rCX3Qwfu5Xx/AtuLTrp9FWkLB4tcZ5PoJ7HpmRUnFjCncAN1M6Ms1RlgLBfcuHhQmP4Iwq+wbXjnB6Mlaso1xOU7PDgmwkJKrGtAr55XP3s2VtSYMPByi2UhJJtxnOAwAOmyqqb9tjicYLPrAk8e7fOdD2oNTLE/l8sCN2QZmkJor1YbU41QTcaHkEsU7QNKnvTSSKokLfJiC sOekmwKO dCjVHz6nZeydCnWRPESzlCumj4nygmuSw9XrPXuNeGwV4nLifZEHgExGu6VOy9fO7iJBh 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 Wed, 16 Apr 2025 14:54:16 -0700 Dan Williams wrote: > Rakie Kim wrote: > > + > > +static void iw_table_free(void) > > +{ > > + u8 *old; > > + > > + mutex_lock(&iw_table_lock); > > + old = rcu_dereference_protected(iw_table, > > + lockdep_is_held(&iw_table_lock)); > > + if (old) { > > + rcu_assign_pointer(iw_table, NULL); > > + mutex_unlock(&iw_table_lock); > > + > > + synchronize_rcu(); > > + kfree(old); > > + } else > > + mutex_unlock(&iw_table_lock); > > This looks correct. I personally would not have spent the effort to > avoid the synchronize_rcu() because this is an error path that rarely > gets triggered, and kfree(NULL) is already a nop, so no pressing need to be > careful there either: > > mutex_lock(&iw_table_lock); > old = rcu_dereference_protected(iw_table, > lockdep_is_held(&iw_table_lock)); > rcu_assign_pointer(iw_table, NULL); > mutex_unlock(&iw_table_lock); > synchronize_rcu(); > kfree(old); I will modify the structure as you suggested. > > > +} > > + > > +static void wi_kobj_release(struct kobject *wi_kobj) > > +{ > > + iw_table_free(); > > This memory can be freed as soon as node_attrs have been deleted. By > waiting until final wi_kobj release it confuses the lifetime rules. > > > + kfree(node_attrs); > > This memory too can be freed as soon as the attributes are deleted. > > ...the rationale for considering these additional cleanups below: > I created a new function named wi_cleanup(), as you proposed. static void wi_cleanup(struct kobject *wi_kobj) { sysfs_wi_node_delete_all(wi_kobj); iw_table_free(); kfree(node_attrs); } And I changed the error handling code to call this function. static int add_weighted_interleave_group(struct kobject *root_kobj) { ... err_cleanup_kobj: wi_cleanup(wi_kobj); kobject_del(wi_kobj); err_put_kobj: kobject_put(wi_kobj); return err; } However, I have one question. With this change, iw_table and node_attrs will not be freed at system shutdown. Is it acceptable to leave this memory unfreed on shutdown? (As you previously noted, the sysfs files in this patch are also not removed during system shutdown.) > > + kfree(wi_kobj); > > } > > > > static const struct kobj_type wi_ktype = { > > .sysfs_ops = &kobj_sysfs_ops, > > - .release = sysfs_wi_release, > > + .release = wi_kobj_release, > > }; > > > > static int add_weight_node(int nid, struct kobject *wi_kobj) > > @@ -3525,41 +3548,42 @@ static int add_weighted_interleave_group(struct kobject *root_kobj) > > struct kobject *wi_kobj; > > int nid, err; > > > > + 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) > > + if (!wi_kobj) { > > + kfree(node_attrs); > > return -ENOMEM; > > + } > > > > err = kobject_init_and_add(wi_kobj, &wi_ktype, root_kobj, > > "weighted_interleave"); > > If you fix wi_kobj_release() to stop being responsible to free memory > that should have been handled in the delete path (@node_attrs, > iw_table_free()), then you can also drop the wi_ktype and > wi_kobj_release() callback altogether. I understand your suggestion about simplifying the kobject handling. If we only consider Patch1, then replacing kobject_init_and_add with kobject_create_and_add would be the right choice. However, in Patch2, the code changes as follows: struct sysfs_wi_group { struct kobject wi_kobj; struct iw_node_attr *nattrs[]; }; static struct sysfs_wi_group *wi_group; ... static void wi_kobj_release(struct kobject *wi_kobj) { kfree(wi_group); } ... static int __init add_weighted_interleave_group(struct kobject *mempolicy_kobj) { int nid, err; wi_group = kzalloc(struct_size(wi_group, nattrs, nr_node_ids), GFP_KERNEL); In this case, wi_kobj_release() is responsible for freeing the container struct wi_group that includes the kobject. Therefore, it seems more appropriate to use kobject_init_and_add in this context. I would appreciate your thoughts on this. > > I.e. once releasing @wi_kobj is just "kfree(wi_kobj)", then this > sequence: > > wi_kobj = kzalloc(...) > kobject_init_and_add(wi_kob, &wi_ktype, ...) > > Can simply become: > > wi_kobj = kobject_create_and_add("weighted_interleave", root_kobj); > > > - if (err) { > > - kfree(wi_kobj); > > - return err; > > - } > > + 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); > > - break; > > + goto err_cleanup_kobj; > > } > > } > > - if (err) > > - kobject_put(wi_kobj); > > + > > return 0; > > + > > +err_cleanup_kobj: > > + sysfs_wi_node_delete_all(wi_kobj); > > + kobject_del(wi_kobj); > > +err_put_kobj: > > + kobject_put(wi_kobj); > > + return err; > > } > > > > static void mempolicy_kobj_release(struct kobject *kobj) > > { > > - u8 *old; > > - > > - mutex_lock(&iw_table_lock); > > - old = rcu_dereference_protected(iw_table, > > - lockdep_is_held(&iw_table_lock)); > > - rcu_assign_pointer(iw_table, NULL); > > - mutex_unlock(&iw_table_lock); > > - synchronize_rcu(); > > - kfree(old); > > - kfree(node_attrs); > > kfree(kobj); > > } > > > > @@ -3573,37 +3597,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"); > > Similar comment as above, now that mempolicy_kobj_release() is simply > kfree(@kobj), you can use kobject_create_and_add() and make this all > that much simpler. For the mempolicy_kobj, I will update the code as you suggested and use kobject_create_and_add(). With all your recommendations applied, Patch1 would now look like this: @@ -3488,20 +3488,21 @@ static void iw_table_free(void) mutex_lock(&iw_table_lock); old = rcu_dereference_protected(iw_table, lockdep_is_held(&iw_table_lock)); - if (old) { - rcu_assign_pointer(iw_table, NULL); - mutex_unlock(&iw_table_lock); + rcu_assign_pointer(iw_table, NULL); + mutex_unlock(&iw_table_lock); - synchronize_rcu(); - kfree(old); - } else - mutex_unlock(&iw_table_lock); + synchronize_rcu(); + kfree(old); } -static void wi_kobj_release(struct kobject *wi_kobj) -{ +static void wi_cleanup(struct kobject *wi_kobj) { + sysfs_wi_node_delete_all(wi_kobj); iw_table_free(); kfree(node_attrs); +} + +static void wi_kobj_release(struct kobject *wi_kobj) +{ kfree(wi_kobj); } @@ -3575,45 +3576,30 @@ static int add_weighted_interleave_group(struct kobject *root_kobj) return 0; err_cleanup_kobj: - sysfs_wi_node_delete_all(wi_kobj); + wi_cleanup(wi_kobj); kobject_del(wi_kobj); err_put_kobj: kobject_put(wi_kobj); return err; } -static void mempolicy_kobj_release(struct kobject *kobj) -{ - kfree(kobj); -} - -static const struct kobj_type mempolicy_ktype = { - .release = mempolicy_kobj_release -}; - static int __init mempolicy_sysfs_init(void) { int err; static struct kobject *mempolicy_kobj; - mempolicy_kobj = kzalloc(sizeof(*mempolicy_kobj), GFP_KERNEL); + mempolicy_kobj = kobject_create_and_add("mempolicy", mm_kobj); if (!mempolicy_kobj) return -ENOMEM; - err = kobject_init_and_add(mempolicy_kobj, &mempolicy_ktype, mm_kobj, - "mempolicy"); - if (err) - goto err_put_kobj; - err = add_weighted_interleave_group(mempolicy_kobj); if (err) - goto err_del_kobj; + goto err_kobj; return 0; -err_del_kobj: +err_kobj: kobject_del(mempolicy_kobj); -err_put_kobj: kobject_put(mempolicy_kobj); return err; } Rakie > > So the patch looks technically correct as is, but if you make those > final cleanups I will add my review tag.