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 12A7EC282EC for ; Fri, 14 Mar 2025 10:55:09 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 511A3280006; Fri, 14 Mar 2025 06:55:06 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 49C7E280004; Fri, 14 Mar 2025 06:55:06 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 36373280006; Fri, 14 Mar 2025 06:55:06 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 19118280004 for ; Fri, 14 Mar 2025 06:55:06 -0400 (EDT) Received: from smtpin25.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 07E2556456 for ; Fri, 14 Mar 2025 10:55:08 +0000 (UTC) X-FDA: 83219849496.25.70C981E Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) by imf10.hostedemail.com (Postfix) with ESMTP id C7C03C000B for ; Fri, 14 Mar 2025 10:55:05 +0000 (UTC) Authentication-Results: imf10.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf10.hostedemail.com: domain of jonathan.cameron@huawei.com designates 185.176.79.56 as permitted sender) smtp.mailfrom=jonathan.cameron@huawei.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1741949706; a=rsa-sha256; cv=none; b=i8WEJec54kTXyXSi278rGE/GPNwc4wBUrBjuuJ1nz9mxaJfmm2KBAeQvdbbc+mxGcb2goW gNU4a7rECgxC+qXMZhx+g+f7dZmtEWN+PZgGL4xv5+3gUN9QHvZ91hSxeLsylbZKOU8n+7 NCQiH2pxT7XztnQrM9OygZPZ7Kqz6u0= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf10.hostedemail.com: domain of jonathan.cameron@huawei.com designates 185.176.79.56 as permitted sender) smtp.mailfrom=jonathan.cameron@huawei.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1741949706; 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=akNdDDZaWq467cb8N7VrhObK0AqcbINIgjT5Kbu/XdI=; b=fCSC6bqaMOeTzTfn0L2bj2kWC0ABA6G6LYC5iB4obh5GGlIzr8nFBb2ilPS6wL+mxSvKK5 Y8FnegEG+r1nzNvlGzrlziopR6cxCLlgSHIweCW4RYAcWca+JIZWDvEoJjlB+l+0w0mwAl 2DN/JICX5KFXafQp68x9rrzPI1SZsuE= Received: from mail.maildlp.com (unknown [172.18.186.231]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4ZDh3S0VHKz67D9Y; Fri, 14 Mar 2025 18:50:28 +0800 (CST) Received: from frapeml500008.china.huawei.com (unknown [7.182.85.71]) by mail.maildlp.com (Postfix) with ESMTPS id BC6391400CA; Fri, 14 Mar 2025 18:55:02 +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, 14 Mar 2025 11:55:02 +0100 Date: Fri, 14 Mar 2025 10:55:00 +0000 From: Jonathan Cameron To: Gregory Price CC: Rakie Kim , , , , , , , , , , Subject: Re: [PATCH v2 1/4] mm/mempolicy: Fix memory leaks in mempolicy_sysfs_init() Message-ID: <20250314105500.00000157@huawei.com> In-Reply-To: References: <20250313063247.681-1-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: lhrpeml100002.china.huawei.com (7.191.160.241) To frapeml500008.china.huawei.com (7.182.85.71) X-Rspam-User: X-Rspamd-Queue-Id: C7C03C000B X-Stat-Signature: b6hskbwtz99qp37aaqqfx5s1tjjfeq58 X-Rspamd-Server: rspam06 X-HE-Tag: 1741949705-542262 X-HE-Meta: U2FsdGVkX19ih8f8qOIGVsEkMozcyuhZ8CD5pUQ2iih9zk2bbqALhQEVOGnV/r73lQx6s8aqpnxmRT6mGmEAJtISX45lhFssmSRIQg8sHGjs2KdBlABWacN7e+MuyNuDMZxsZTrC8I6Dr6LdRtWEnz7UTmXJy9nRnC4mz+KaQdw7qNglKtRNZ4f2X7QqSF1OKhpF/kJc+IDQoiPAdQjmfpADxx1I7z0PJ2cJWXCCFamQyqErL1f2Ay7gNaPlrueiJdZN10fNJKxyV/CRBWYy5Ac7Yr7bxRBXg3X9+y/2ek26nIGvLOeXdo79WFLrSgsi5F3ACVwrIgQ3Li5M3j5uYJb1aqbzDi4u/brPLC4qnxTMnsNeJvKJPclqyv1K7nT0cW5Qqv0w+cwl8M/vP1cFHxAJK3YXh35Op33tfpUdjANH8s5qdH0qqgmkLawf+E6kChWzsbn1FsLb5KQFuv/Re5FQYgDNxlNzxPfRrdIZfDV19TPBDIVdHdQbGGlbmLjbbn5LMdaY5hW40o+0yfR6MXThzNrQx7zIKg9ooLpNXCEVvwKvTrinxChkOWSm/Yxq4h+JtWVMheip+CHvTSt4AJqbweUEP+gXYENgjmC6m9QiMDoUyN69eINR7w+ht9lWiRFyzAnwQh8pDLcolwH2/P7+btQoHnkdZ9IYI9aJwxRVZg6dNKQxGMEmREE7ZOU7c93CHMasu6aHLu1KV7qIrb+XQIAeKcZ7j29wazpzU7+4OoWv1+ofIigThalt6WGibKDdDhWFi/Iu/gt0vgxJzXl5Lkk7EgdFHyaVkdXMKlWya+HYy8GmqZjsVrPzy9ijS3hlPAFr449HRvy38N6BHPAb1ASAv6kM3Pkem04oOQZEIc92JQ4eMl+FWqMsQ4TnmvhVwz5GGNZ/q/Q3bGii4gJ+GRVavid/GjToX6N/xj8TmSu9zhLhcjjE3FlM5+tBJm5nQr0nO4N/cQ7yDCB 5I5aohlm limSKh8qPewU5RZr6Is28svlr51Cvl/O9wRXIuzBN8469JdC3/h/C5j9iU7QJABiJAa99ra9U3ZP7/Dl+VkI5QOsuKntmzjDVK6hBFUrOuzcieoqjtwO7w9nz81tZCvrr1F+JRgVuoDxp/CimL6B7lcwqu+NsK0DsxweTbhBmnu+LGW4CdndtFZtMz9cxpc25fPY5PHAwlJG1OB30KKdbX7vviyGvFY8u/3JQmBOARmq9a7uEWU92nFDVsQxPuKLhSglkc6P1x1E5C7+BeLxdK/tkpMdCEe1IGpKrLQsJiXlT/ASw8/UhmqVk86axs0u9zgeAaZzxVM1iiZpbpyC21hwOXk6i1ZjXDWyu 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 Thu, 13 Mar 2025 11:52:18 -0400 Gregory Price wrote: > On Thu, Mar 13, 2025 at 03:31:38PM +0900, Rakie Kim wrote: > > > Is this correct? If kobject_init_and_add fails, from other examples we > > > need only free the mempolicy_kobj - because it failed to initialize and > > > therefore should not have any references. I think this causes an > > > underflow. > > > > Regarding the reordering of mempolicy_kobj allocation: > > 1) In kobject_init_and_add(), kobject_init() is always called, which > > Quite right, mea culpa. > > > > > 2) The release function for mempolicy_kobj is responsible for freeing > > associated memory: > > > > static void mempolicy_kobj_release(struct kobject *kobj) > > { > > ... > > kfree(ngrp->nattrs); > > kfree(ngrp); > > kfree(kobj); > > } > > > > I see what you're trying to do now after looking at the free-ordering > at little closer. > > Lets do the following: > > 1) allocate node_attrs and mempolicy_kobj up front and keep your > reordering, this lets us clean up allocations on failure before > kobject_init is called > > 2) after this remove all the other code and just let > mempolicy_kobj_release clean up node_attrs > > 3) Add a (%d) to the error message to differentiate failures Given how unlikely (and noisy) a memory allocation failure is, maybe just drop the printing at all in those paths - allowing early returns. The lifetime rules around node_attrs in here are making readability poor. It is implicitly owned by the mempolicy_kobj, but no direct association. Maybe just encapsulating the kobject in a structure that contains this as a [] array at the end. Then we end up with single allocation of stuff that is effectively one thing. > > This is a little bit cleaner and is a bit less code. (Not built or > tested, just a recommendation). > > I'd recommend submitting this patch by itself to mm-stable, since the > remainder of the patch line changes functionality and this fixes a bug > in LTS kernels. > > ~Gregory > > --- > > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > index 530e71fe9147..05a410db08b4 100644 > --- a/mm/mempolicy.c > +++ b/mm/mempolicy.c > @@ -3541,38 +3541,34 @@ static int __init mempolicy_sysfs_init(void) > int err; > static struct kobject *mempolicy_kobj; > > - mempolicy_kobj = kzalloc(sizeof(*mempolicy_kobj), GFP_KERNEL); > - if (!mempolicy_kobj) { > + node_attrs = kcalloc(nr_node_ids, sizeof(struct iw_node_attr *), > + GFP_KERNEL); > + if (!node_attrs) { > err = -ENOMEM; > goto err_out; > } > > - node_attrs = kcalloc(nr_node_ids, sizeof(struct iw_node_attr *), > - GFP_KERNEL); > - if (!node_attrs) { > + mempolicy_kobj = kzalloc(sizeof(*mempolicy_kobj), GFP_KERNEL); > + if (!mempolicy_kobj) { > err = -ENOMEM; > - goto mempol_out; > + kfree(node_attrs); > + goto err_out; > } > > err = kobject_init_and_add(mempolicy_kobj, &mempolicy_ktype, mm_kobj, > "mempolicy"); > if (err) > - goto node_out; > + goto mempol_out; > > 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 mempol_out; > > - return err; > -node_out: > - kfree(node_attrs); > + return 0; > mempol_out: > - kfree(mempolicy_kobj); > + kobject_put(mempolicy_kobj); > err_out: > - pr_err("failed to add mempolicy kobject to the system\n"); > + pr_err("mempolicy sysfs structure failed to initialize (%d)\n", err); > return err; > } > >