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 A3224C28B30 for ; Thu, 20 Mar 2025 16:45:38 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 63709280004; Thu, 20 Mar 2025 12:45:37 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 5E7E7280001; Thu, 20 Mar 2025 12:45:37 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 46201280004; Thu, 20 Mar 2025 12:45:37 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 2A494280001 for ; Thu, 20 Mar 2025 12:45:37 -0400 (EDT) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 85940BA976 for ; Thu, 20 Mar 2025 16:45:37 +0000 (UTC) X-FDA: 83242505514.08.ECE5094 Received: from mail-yw1-f180.google.com (mail-yw1-f180.google.com [209.85.128.180]) by imf15.hostedemail.com (Postfix) with ESMTP id CAD6DA0006 for ; Thu, 20 Mar 2025 16:45:35 +0000 (UTC) Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=HtSU6bo3; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf15.hostedemail.com: domain of joshua.hahnjy@gmail.com designates 209.85.128.180 as permitted sender) smtp.mailfrom=joshua.hahnjy@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1742489135; a=rsa-sha256; cv=none; b=UYcufQdE3pwso6gBClnsMOzSjSoR442JWk9eCNWx1/DYlf4DMXDFyuAuxiKJwG4DVFR1YR KHyHy1VfnYjI23xsJkFSv+kbxxVdcss1hYD2YyEp94BWNp3TAqLR87sCwqyQGjJAFhS95n w0lpiHoFVM25LmIvbqy5cfN+U43qKt8= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=HtSU6bo3; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf15.hostedemail.com: domain of joshua.hahnjy@gmail.com designates 209.85.128.180 as permitted sender) smtp.mailfrom=joshua.hahnjy@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1742489135; 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:dkim-signature; bh=2KezL3PAx4Yqh0KckvimzJ7vzPDGdKm/LH84hRf1qio=; b=GvIqESAnMqxyAYPJP2h1u8Qqft3SnWIcdUFdMGZqFxk/f8Oi5tQut7b/XKtKL9/BJilaq6 szTvyNWEbwCEwlygdsDJWn8JbhY9MRv8G/+xJdfKZqoSBxiiM2LE0JzadvBtLoaqbyNe+8 Kv+UCtxgxm0ycuaexYJfoHBUsjFj2sw= Received: by mail-yw1-f180.google.com with SMTP id 00721157ae682-6fd80f30ba5so7090687b3.3 for ; Thu, 20 Mar 2025 09:45:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1742489135; x=1743093935; darn=kvack.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=2KezL3PAx4Yqh0KckvimzJ7vzPDGdKm/LH84hRf1qio=; b=HtSU6bo3n5oCx6bKEX7WBkexHe2L+RNlIW7m3tZtV7xIIAiDnbvZe/lfMuQIPMdD7T O/4e2aikCtBpmm3uJx+xZEVLcGCs3UWV/a8jfe0HhpjB25r4eKB9c6gl1N7CwPVpALN5 fgG7unoi2nINoWMNJC0qjT/3UTzrzSd5D7xbz8dI1FqdPz3/mZkhdf9T1fhER73AKKam n3k2xgKtWYjn7cIkCpSlEU8tkvDdYmkiehKX3bF2z+qHmv03nf2vjAbEnDEB+GcGW04p AQvAKShfGP9iK/UV82bmg2MJxjudPI7NsEQwl3Qf0ei9fP8oHhWVstTSVJ8XGTWvsiUM 3W2w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1742489135; x=1743093935; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=2KezL3PAx4Yqh0KckvimzJ7vzPDGdKm/LH84hRf1qio=; b=MbSrhP3kA0whz6Mt/Vvhk7plKvbKuWgiL2gaLJExNe81PCzSUhHF5AXlPGBihuVzL0 Mdfu4bfyZc+M/QbbqVpQEwQQzLQDz/8SQKJlQxYs8LGoU+yfC+mVFckhJztAv6t0w4E8 QosxrM51r9jrbzMwdN10+M9HI2eqXKJtioK2K/nHxPtDH/wQT9Ze3j0vq6l/mRoQ/nsU 1AYvf72kWXtMbvVEdMscFgbdWN5Nt1FZ18yGEc6WsL3AEuka4S383S+S32nuf8BLBqvR ThX8FwaViKYHJ9vl7hGhf9kNqvlB6ar72xIDySVv6ecgPF0u3wlK5Yyrbd/UBXGTd8q7 o/+Q== X-Forwarded-Encrypted: i=1; AJvYcCVez/5V1zcilxc1hF9xZ1s2jv/iyFcsS9QSFQZ70sQzCVgLPZ8nkuXbzZb9it12bxKEYX7wgg1MAw==@kvack.org X-Gm-Message-State: AOJu0Yw/D0kKb+9OioWVPM0VKXgyANzXmkHC/tuVd5Ulxqi0d5eDhTPM G2VtJlZyrcGVo+X3x8Gf/z5/IvXoatbb3ELEK9sA0dEQE7ucxd1o X-Gm-Gg: ASbGncuDMpS5TdA2xCgWYi9sBxjxr7fCKMFi8RhZdwayCS0mapKoUkticnljzQK2X3x hWe0PkI3Zv5oyE67GKn18iZ/RjE9eCUf++UebtUqV4o7rOXO/26Os+FCLs0beXPLp4JlDhmxypg QcfprkyMfEn96qiqSgilA+rlBvjRR7EQ02GeIuzRGpc+C/ivK6Up+cwYERhze5r/ZyiMgU5WucZ /Kmw1WxguKN3DuuepfphIkpPuINqrRAi5WQtYBeLS+uNKo26Dr4g5iNRBPs9C+FIQZHQ5su3Zgr Wd1koXen1CrnmuGv6x06LJS7NjvPCYMPkob+yG98Fko+ X-Google-Smtp-Source: AGHT+IEHB9i2wGUB7JoCCEDiuzQ+VHkFY61m+oAR66RJyCZK95REKh6ZfA0CzpEWMiphX0glIYrNPg== X-Received: by 2002:a05:690c:4809:b0:6fb:9702:f1ca with SMTP id 00721157ae682-700ac5fe1c2mr57390707b3.20.1742489134456; Thu, 20 Mar 2025 09:45:34 -0700 (PDT) Received: from localhost ([2a03:2880:25ff:70::]) by smtp.gmail.com with ESMTPSA id 00721157ae682-700ba8565a4sm81687b3.86.2025.03.20.09.45.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 20 Mar 2025 09:45:33 -0700 (PDT) From: Joshua Hahn To: Rakie Kim Cc: gourry@gourry.net, akpm@linux-foundation.org, 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, Jonathan.Cameron@huawei.com, kernel_team@skhynix.com, honggyu.kim@sk.com, yunjeong.mun@sk.com Subject: Re: [PATCH v3 1/3] mm/mempolicy: Fix memory leaks in weighted interleave sysfs Date: Thu, 20 Mar 2025 09:45:31 -0700 Message-ID: <20250320164532.1313581-1-joshua.hahnjy@gmail.com> X-Mailer: git-send-email 2.47.1 In-Reply-To: <20250320041749.881-2-rakie.kim@sk.com> References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Rspam-User: X-Rspamd-Queue-Id: CAD6DA0006 X-Stat-Signature: cbz4u9mbbmrf7go74n3axsu1r875xgic X-Rspamd-Server: rspam06 X-HE-Tag: 1742489135-342058 X-HE-Meta: U2FsdGVkX182vDwtZmi2nUajZ9aOceiNfosg9WlkHu3oJziZ2CmbEf8JB3RKYZlEh7FAxRwxtSBanARFrvdrLcVLa+y/4B6MiKw4aP4HHtmH5tYJV7alaTesw+9vEBVG1uzbR1csvH0MAVWqw9+Xy89Yce4vptet4wMx1UYsgdfsMFfSy3WS/blk+Xmr4cHNeTrbpCgOvI0rECi9v8PDbAjsJvTdrrzzu6v7cobUd3Ks5qurXLemv9P72zWyxPuaZ6VvGzwUgJd5F2JMGzwqVWPtA/4ggBPKdqE8wl1cBeAfCQtgAZcsbGxrkzu2U5O/PjPOMhQ4BsCoxgu4GV1JXeuywWBFh/634MbR8kxXu29JG0fSXfAOLC6KSuZhaiyaXc8rSRD0cUnJuvYjdGbkHOoG1pQkumDgmdUq+v/yCNTVclAd/ilcUozjJCVMxaJPvOnI+2z6E1uXSD62tp6INqhc2gWRdATYDnkiqSAOza1fC0e9lHJAPepD360SdW19Bt3CUV3IwAC7JC64UkcDhNnUguD4HyDLX7MvtmyyrHyBHJB1YHxSXXgIBkirdimvBgrcKphN2AD6m4cCrQMnM2eWQy9akYxiDoOZjkdeEP3un/LnBPtqgY8Z117KiuV8+2bKCPQse5Q23pTcKbOAlG8KAQhvt+foyUIbBk3iouAoRuuebur5AKn0v8P46/GoA5loQTKu6/Akjlc9AbvUromqpSrO+L7fBRj7zHnCDfVRvqSZM2NrxVZ1iyMGIgqPz/bQ2w72inB2HrzzTcrB1T+aWbLcru3YxQTOsEai5pzpDEGGoEjcWWh5lfHLTS4z0U+M5a7po49HootxNYiYBSdverE74ls3Y0VksVoe3aIG609ojeBtrwNFzdIvwfP2yW6NyAGmMBSuURKD74UV0YaQ0IPJ18a1RFTT2+og4IbLYan82veaFifd6ZMs3bX65ov029SE2UMoJfsRNRk zy4XQceb FeP0MzW4f4rnhTnMM5yczrfGgWiCOLv0cvc+yzDyNyEUPBo2NwFpkvY32PQme5uFP3ri01uceOi/xBKuxsUuqHciKjoku3yTHJ00NPLK6wJ9XXMIkQ6pR8BvflTVI4yfuckPfk9vha1KvkQF1xf6iFWWrnG7/dtQVBAz4XSYdzj8oMrU= 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: Hi Rakie, thank you for the new version! I have just a few questions / nits about this patch. On Thu, 20 Mar 2025 13:17:46 +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 `kobject_put()`, ensuring proper cleanup and preventing > memory leaks. > > By correctly using `kobject_put()`, the release function now > properly deallocates memory without causing resource leaks, > thereby improving system stability. > > Fixes: dce41f5ae253 ("mm/mempolicy: implement the sysfs-based weighted_interleave interface") > Signed-off-by: Rakie Kim > --- > mm/mempolicy.c | 61 +++++++++++++++++++++++++------------------------- > 1 file changed, 31 insertions(+), 30 deletions(-) > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > index bbaadbeeb291..5950d5d5b85e 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); > } I think the intent here is to make mempolicy_sysfs_init call kobject_put, which will then call sysfs_wi_release when the refcount is 0. So I think replacing kobject_put with kfree makes a lot of sense here. However, I think it is a bit confusing based on the commit message, which states that you are doing the opposite (replacing kfree with kobject_put). Perhaps it makes more sense to say that you are moving kfree() from sysfs_init to the release function, so that the struct and the node_attrs struct is freed together by the last reference holder. > 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; It's also not obvious to me why the allocation for node_attrs was moved to add_weighted_interleave_group. Maybe this refactoring belongs in patch 2, whose described intent is to consolidate the two objects into one (I expand on this idea below) > + wi_kobj = kzalloc(sizeof(struct kobject), GFP_KERNEL); > + if (!wi_kobj) { > + err = -ENOMEM; > + goto node_out; > + } > + > 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,17 @@ static int add_weighted_interleave_group(struct kobject *root_kobj) > break; > } > } > - if (err) > + if (err) { > kobject_put(wi_kobj); > + goto err_out; > + } > + > return 0; > + > +node_out: > + kfree(node_attrs); > +err_out: NIT: Is there a reason why we have a single line goto statement? Maybe it is more readable to replace all `goto err_out` with `return err` and save a few jumps : -) > + return err; > } > > static void mempolicy_kobj_release(struct kobject *kobj) > @@ -3528,7 +3545,6 @@ static void mempolicy_kobj_release(struct kobject *kobj) > mutex_unlock(&iw_table_lock); > synchronize_rcu(); > kfree(old); > - kfree(node_attrs); I think the intent of this patch is slightly confusing. Viewing this patch alone, it is not entirely obvious why the kfree for node_attrs is now being moved from the release of mempolicy_kobj to wi_kobj. Of course, we know that it is actually because this patch serves a secondary purpose of moving the allocations / freeing of nattrs and wi_kobj together, so that in the next patch they can be combined into a single struct. I think one way to make this patch more readable and maintainable is to separate it into (1) fixes, (as the Fixes: tag in your commit message suggests) and (2) refactoring that prepares for the next patch. Please let me know what you think -- these were just some thoughts that I had while I was reading the patch. Thank you again for this new version! Have a great day : -) Joshua Sent using hkml (https://github.com/sjp38/hackermail)