Message ID | 20221021022102.2231464-2-yangyingliang@huawei.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4242:0:0:0:0:0 with SMTP id s2csp437922wrr; Thu, 20 Oct 2022 19:26:00 -0700 (PDT) X-Google-Smtp-Source: AMsMyM7HIKhKjsKYSBrIptISDI9VDLAmB4ETRN0FtYQ24Gw44XtF5CpuvPZuTt+Drtungiiy8NKV X-Received: by 2002:a63:1608:0:b0:45a:355a:9420 with SMTP id w8-20020a631608000000b0045a355a9420mr14314820pgl.354.1666319160458; Thu, 20 Oct 2022 19:26:00 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666319160; cv=none; d=google.com; s=arc-20160816; b=JuEmEmlOrhJG+ozdxUkJO13MMDrn+x8vZJaL+8jTFJBdqt2gmMRs1Xiox/8RAXjOEw tPiAyqhdL+jKyRRNKht+Us/+fRI7J9iz6EnROlSAWIal/20TjZ4gvsOd3A7pHv+gU8Wx AH01+zIGtX4dDl5cSZ5nkLHdppA2KXjfLLlomdnt9J/K3vATWvCHgFiffbBWxzd8vjyO 1A6A91iXkhBuaRIyJ8owH51CiPv00Qs7JmcgiYuZHxB3NLDDv6/LlfukI+95OQqtrt7/ Ptf2wIeeDEjnJ/2ISMz6IwV/nlNxWh/0X05ljihMcD9TBvxMQv5s2L6RsnSO2mxZwqL/ bqcw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from; bh=dyGVpi/cxfdKsPC0auDFM0qgMAIU47NidVfdyPK5ykE=; b=IM/aZ5Czw/QvfoM/Z6k73kWhW0Wec1XojDbp/FQ6nzqVS4u6Z7bBS+CG7kpa57k6Qi 1ulb3Dcg5e6S39DkRgre+b+R91GqLjDtXFvq7V1O73TaGT7gtD0PDM2IVe9UVRdaF8N6 6EFgUdxmHBg24EAV+6Ul2I3sv4Bp5t5d2fa9bMSICMyfsEViqAUxHBr4TPeD6UnID+uX wGQrISiledlEUPn8jAm68Si7VUCoYES3XX+taFNOwso4BNDQsaz+1HakSGcOd3+1wgU6 A/7TlcTMXxDf7rxQSo3cqx6825pTOY5V62GDqB64WvjLjnPZDZUyEzyDWHR58qIPJG+d jBHA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id 21-20020a630515000000b004599c5f7d63si23725779pgf.857.2022.10.20.19.25.47; Thu, 20 Oct 2022 19:26:00 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230018AbiJUCXJ (ORCPT <rfc822;pwkd43@gmail.com> + 99 others); Thu, 20 Oct 2022 22:23:09 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37596 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229980AbiJUCWz (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 20 Oct 2022 22:22:55 -0400 Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7A72A232E54 for <linux-kernel@vger.kernel.org>; Thu, 20 Oct 2022 19:22:51 -0700 (PDT) Received: from dggpemm500022.china.huawei.com (unknown [172.30.72.57]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4MtpCN0WmtzHvCB; Fri, 21 Oct 2022 10:22:40 +0800 (CST) Received: from dggpemm500007.china.huawei.com (7.185.36.183) by dggpemm500022.china.huawei.com (7.185.36.162) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Fri, 21 Oct 2022 10:22:40 +0800 Received: from huawei.com (10.175.103.91) by dggpemm500007.china.huawei.com (7.185.36.183) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Fri, 21 Oct 2022 10:22:36 +0800 From: Yang Yingliang <yangyingliang@huawei.com> To: <linux-kernel@vger.kernel.org>, <qemu-devel@nongnu.org>, <linux-f2fs-devel@lists.sourceforge.net>, <linux-erofs@lists.ozlabs.org>, <ocfs2-devel@oss.oracle.com>, <linux-mtd@lists.infradead.org>, <amd-gfx@lists.freedesktop.org> CC: <gregkh@linuxfoundation.org>, <rafael@kernel.org>, <somlo@cmu.edu>, <mst@redhat.com>, <jaegeuk@kernel.org>, <chao@kernel.org>, <hsiangkao@linux.alibaba.com>, <huangjianan@oppo.com>, <mark@fasheh.com>, <jlbec@evilplan.org>, <joseph.qi@linux.alibaba.com>, <akpm@linux-foundation.org>, <alexander.deucher@amd.com>, <luben.tuikov@amd.com>, <richard@nod.at>, <liushixin2@huawei.com> Subject: [PATCH 01/11] kset: fix documentation for kset_register() Date: Fri, 21 Oct 2022 10:20:52 +0800 Message-ID: <20221021022102.2231464-2-yangyingliang@huawei.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20221021022102.2231464-1-yangyingliang@huawei.com> References: <20221021022102.2231464-1-yangyingliang@huawei.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7BIT Content-Type: text/plain; charset=US-ASCII X-Originating-IP: [10.175.103.91] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) To dggpemm500007.china.huawei.com (7.185.36.183) X-CFilter-Loop: Reflected X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1747262279852004794?= X-GMAIL-MSGID: =?utf-8?q?1747262279852004794?= |
Series |
fix memory leak while kset_register() fails
|
|
Commit Message
Yang Yingliang
Oct. 21, 2022, 2:20 a.m. UTC
kset_register() is currently used in some places without calling
kset_put() in error path, because the callers think it should be
kset internal thing to do, but the driver core can not know what
caller doing with that memory at times. The memory could be freed
both in kset_put() and error path of caller, if it is called in
kset_register().
So make the function documentation more explicit about calling
kset_put() in the error path of caller.
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
lib/kobject.c | 3 +++
1 file changed, 3 insertions(+)
Comments
On 2022-10-20 22:20, Yang Yingliang wrote: > kset_register() is currently used in some places without calling > kset_put() in error path, because the callers think it should be > kset internal thing to do, but the driver core can not know what > caller doing with that memory at times. The memory could be freed > both in kset_put() and error path of caller, if it is called in > kset_register(). > > So make the function documentation more explicit about calling > kset_put() in the error path of caller. > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > --- > lib/kobject.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/lib/kobject.c b/lib/kobject.c > index a0b2dbfcfa23..6da04353d974 100644 > --- a/lib/kobject.c > +++ b/lib/kobject.c > @@ -834,6 +834,9 @@ EXPORT_SYMBOL_GPL(kobj_sysfs_ops); > /** > * kset_register() - Initialize and add a kset. > * @k: kset. > + * > + * If this function returns an error, kset_put() must be called to > + * properly clean up the memory associated with the object. > */ And I'd continue the sentence, with " ... with the object, for instance the memory for the kset.kobj.name when kobj_set_name(&kset.kobj, format, ...) was called before calling kset_register()." This makes it clear what we want to make sure is freed, in case of an early error from kset_register(). Regards, Luben > int kset_register(struct kset *k) > {
On 2022/10/21 13:34, Luben Tuikov wrote: > On 2022-10-20 22:20, Yang Yingliang wrote: >> kset_register() is currently used in some places without calling >> kset_put() in error path, because the callers think it should be >> kset internal thing to do, but the driver core can not know what >> caller doing with that memory at times. The memory could be freed >> both in kset_put() and error path of caller, if it is called in >> kset_register(). >> >> So make the function documentation more explicit about calling >> kset_put() in the error path of caller. >> >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> >> --- >> lib/kobject.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/lib/kobject.c b/lib/kobject.c >> index a0b2dbfcfa23..6da04353d974 100644 >> --- a/lib/kobject.c >> +++ b/lib/kobject.c >> @@ -834,6 +834,9 @@ EXPORT_SYMBOL_GPL(kobj_sysfs_ops); >> /** >> * kset_register() - Initialize and add a kset. >> * @k: kset. >> + * >> + * If this function returns an error, kset_put() must be called to >> + * properly clean up the memory associated with the object. >> */ > And I'd continue the sentence, with " ... with the object, > for instance the memory for the kset.kobj.name when kobj_set_name(&kset.kobj, format, ...) > was called before calling kset_register()." kobject_cleanup() not only frees name, but aslo calls ->release() to free another resources. > > This makes it clear what we want to make sure is freed, in case of an early error > from kset_register(). How about like this: If this function returns an error, kset_put() must be called to clean up the name of kset object and other memory associated with the object. > > Regards, > Luben > >> int kset_register(struct kset *k) >> { > .
On Fri, Oct 21, 2022 at 04:05:18PM +0800, Yang Yingliang wrote: > > On 2022/10/21 13:34, Luben Tuikov wrote: > > On 2022-10-20 22:20, Yang Yingliang wrote: > > > kset_register() is currently used in some places without calling > > > kset_put() in error path, because the callers think it should be > > > kset internal thing to do, but the driver core can not know what > > > caller doing with that memory at times. The memory could be freed > > > both in kset_put() and error path of caller, if it is called in > > > kset_register(). > > > > > > So make the function documentation more explicit about calling > > > kset_put() in the error path of caller. > > > > > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > > > --- > > > lib/kobject.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/lib/kobject.c b/lib/kobject.c > > > index a0b2dbfcfa23..6da04353d974 100644 > > > --- a/lib/kobject.c > > > +++ b/lib/kobject.c > > > @@ -834,6 +834,9 @@ EXPORT_SYMBOL_GPL(kobj_sysfs_ops); > > > /** > > > * kset_register() - Initialize and add a kset. > > > * @k: kset. > > > + * > > > + * If this function returns an error, kset_put() must be called to > > > + * properly clean up the memory associated with the object. > > > */ > > And I'd continue the sentence, with " ... with the object, > > for instance the memory for the kset.kobj.name when kobj_set_name(&kset.kobj, format, ...) > > was called before calling kset_register()." > kobject_cleanup() not only frees name, but aslo calls ->release() to free > another resources. Yes, but it's the kobject of the kset, which does need to have it's name cleaned up, but that kobject should NOT be freeing any larger structures that the kset might be embedded in, right? > > This makes it clear what we want to make sure is freed, in case of an early error > > from kset_register(). > > How about like this: > > If this function returns an error, kset_put() must be called to clean up the name of > kset object and other memory associated with the object. Again, I think we can fix this up to not be needed. thanks, greg k-h
On 2022-10-21 04:05, Yang Yingliang wrote: > > On 2022/10/21 13:34, Luben Tuikov wrote: >> On 2022-10-20 22:20, Yang Yingliang wrote: >>> kset_register() is currently used in some places without calling >>> kset_put() in error path, because the callers think it should be >>> kset internal thing to do, but the driver core can not know what >>> caller doing with that memory at times. The memory could be freed >>> both in kset_put() and error path of caller, if it is called in >>> kset_register(). >>> >>> So make the function documentation more explicit about calling >>> kset_put() in the error path of caller. >>> >>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> >>> --- >>> lib/kobject.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/lib/kobject.c b/lib/kobject.c >>> index a0b2dbfcfa23..6da04353d974 100644 >>> --- a/lib/kobject.c >>> +++ b/lib/kobject.c >>> @@ -834,6 +834,9 @@ EXPORT_SYMBOL_GPL(kobj_sysfs_ops); >>> /** >>> * kset_register() - Initialize and add a kset. >>> * @k: kset. >>> + * >>> + * If this function returns an error, kset_put() must be called to >>> + * properly clean up the memory associated with the object. >>> */ >> And I'd continue the sentence, with " ... with the object, >> for instance the memory for the kset.kobj.name when kobj_set_name(&kset.kobj, format, ...) >> was called before calling kset_register()." > kobject_cleanup() not only frees name, but aslo calls ->release() to > free another resources. Yes, it does. For this reason I said "for instance..." I didn't want to include this in case in the future if the code changes, the comment would be wrong. IOW, I wanted to add the minimalist comment possible. >> >> This makes it clear what we want to make sure is freed, in case of an early error >> from kset_register(). > > How about like this: > > If this function returns an error, kset_put() must be called to clean up the name of > kset object and other memory associated with the object. It's bit too wordy and redundant with what else it does--this can be gleaned from the code. I'd say: On error, kset_put() should be called to clean up at least kset.kobj.name allocated by kobj_set_name(&kset.kobj, format, ...). This tells the reader the symmetry of the calls: kobj_set_name() --> kset_register() --> kset_put(); Because if the code evolves to use other means of allocation, or if the the user allocates a name by different means, then they'll understand what to watch out for. Regards, Luben > >> >> Regards, >> Luben >> >>> int kset_register(struct kset *k) >>> { >> .
diff --git a/lib/kobject.c b/lib/kobject.c index a0b2dbfcfa23..6da04353d974 100644 --- a/lib/kobject.c +++ b/lib/kobject.c @@ -834,6 +834,9 @@ EXPORT_SYMBOL_GPL(kobj_sysfs_ops); /** * kset_register() - Initialize and add a kset. * @k: kset. + * + * If this function returns an error, kset_put() must be called to + * properly clean up the memory associated with the object. */ int kset_register(struct kset *k) {