Message ID | 20221101032125.27337-1-shangxiaojing@huawei.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp2707934wru; Mon, 31 Oct 2022 20:45:55 -0700 (PDT) X-Google-Smtp-Source: AMsMyM4sptLZRBiYbl3XLRT2MesSdudB7t38ORHM/7vCeUljG9ygWUr0czTs28acPxi9ZEmvHIks X-Received: by 2002:a17:907:948f:b0:7a7:5fc8:909 with SMTP id dm15-20020a170907948f00b007a75fc80909mr16787053ejc.658.1667274355706; Mon, 31 Oct 2022 20:45:55 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1667274355; cv=none; d=google.com; s=arc-20160816; b=XFHlaqrCiX5gBMFJF6Hh5jZLSqEn2AOycV0Auwo4+jNxX6EKAX1JElvOAWQHhs9C8F krhzI41dXEdpJTWuj9+vRWf3SGdb4RYUGaOK+WvOn0l8QnrNdIOXP/2bS8FRcI+8A9uk HkZD5Sey9WiQXObw4vg13j51B2EBwIGv1WDnJ97XwEzmfxeWeflQnMIYLu0sSbRkb++7 lzzg6SVeMjjdNJDfOdZqL7EESk1xIzTO7ZJEH3JFgxKaTKxdDFrkqyfHh9g/vWozI/C8 QsO0OrtygHA/WDOPDB5NBWlGWxniN7/5hmTlisQu4UJhpWTQE3g+i3MnPGhHZG4uAOiU e0fw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:mime-version:message-id:date:subject:cc:to:from; bh=RFQUYziucMEb9cSvjkAIaACbfmRxGWw/kSM57VQOGL8=; b=S0LRKZKddp3ZcnPPhVS2BPADqKDcSAet7te85R2XZLDw5npOAY1KhCs1notxsCC1Hm nL37U4dEt5KuET27btcaj9Mbj7psK8MYdurCElM4Fv1frpGMQZOsiIHDzDCcd5ue3DfE yEVnsRrNnHQESad6BmjvSDazltzLfhpLpu3SYwOvO2xm8CgQauv+YCWgVqxYCKWszdlW BmiErDtkqo8Fz5CBTx8wSKSm52tHXeRFQy2jVMhoiiLv2+QSph0AzEfwX6Xhm+QGzFSh nWiqX1vHZlzlz423wyNsbuH/aJ2GYZozaXO8YHGsx32k1baGBnr8XvTOAZLo7W+R5A5+ Ux7w== 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 qf37-20020a1709077f2500b0078de83a052csi11886580ejc.483.2022.10.31.20.45.31; Mon, 31 Oct 2022 20:45:55 -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 S229851AbiKADWn (ORCPT <rfc822;kartikey406@gmail.com> + 99 others); Mon, 31 Oct 2022 23:22:43 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60750 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229835AbiKADWi (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 31 Oct 2022 23:22:38 -0400 Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4EFD4BCB3 for <linux-kernel@vger.kernel.org>; Mon, 31 Oct 2022 20:22:34 -0700 (PDT) Received: from kwepemi500016.china.huawei.com (unknown [172.30.72.53]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4N1b1L5NdCz15M2t; Tue, 1 Nov 2022 11:22:30 +0800 (CST) Received: from huawei.com (10.175.100.227) by kwepemi500016.china.huawei.com (7.221.188.220) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Tue, 1 Nov 2022 11:22:31 +0800 From: Shang XiaoJing <shangxiaojing@huawei.com> To: <abbotti@mev.co.uk>, <hsweeten@visionengravers.com>, <gregkh@linuxfoundation.org>, <zhangxuezhi1@coolpad.com>, <fmhess@users.sourceforge.net>, <linux-kernel@vger.kernel.org> CC: <shangxiaojing@huawei.com> Subject: [PATCH] comedi: Fix potential memory leak in comedi_init() Date: Tue, 1 Nov 2022 11:21:25 +0800 Message-ID: <20221101032125.27337-1-shangxiaojing@huawei.com> X-Mailer: git-send-email 2.17.1 MIME-Version: 1.0 Content-Type: text/plain X-Originating-IP: [10.175.100.227] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) To kwepemi500016.china.huawei.com (7.221.188.220) 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?1748263874493924737?= X-GMAIL-MSGID: =?utf-8?q?1748263874493924737?= |
Series |
comedi: Fix potential memory leak in comedi_init()
|
|
Commit Message
Shang XiaoJing
Nov. 1, 2022, 3:21 a.m. UTC
comedi_init() will goto out_unregister_chrdev_region if cdev_add()
failed, which won't free the resource alloced in kobject_set_name().
Call kfree_const() to free the leaked name before goto
out_unregister_chrdev_region.
unreferenced object 0xffff8881000fa8c0 (size 8):
comm "modprobe", pid 239, jiffies 4294905173 (age 51.308s)
hex dump (first 8 bytes):
63 6f 6d 65 64 69 00 ff comedi..
backtrace:
[<000000005f9878f7>] __kmalloc_node_track_caller+0x4c/0x1c0
[<000000000fd70302>] kstrdup+0x3f/0x70
[<000000009428bc33>] kstrdup_const+0x46/0x60
[<00000000ed50d9de>] kvasprintf_const+0xdb/0xf0
[<00000000b2766964>] kobject_set_name_vargs+0x3c/0xe0
[<00000000f2424ef7>] kobject_set_name+0x62/0x90
[<000000005d5a125b>] 0xffffffffa0013098
[<00000000f331e663>] do_one_initcall+0x7a/0x380
[<00000000aa7bac96>] do_init_module+0x5c/0x230
[<000000005fd72335>] load_module+0x227d/0x2420
[<00000000ad550cf1>] __do_sys_finit_module+0xd5/0x140
[<00000000069a60c5>] do_syscall_64+0x3f/0x90
[<00000000c5e0d521>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
Fixes: ed9eccbe8970 ("Staging: add comedi core")
Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com>
---
drivers/comedi/comedi_fops.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Comments
On Tue, Nov 01, 2022 at 11:21:25AM +0800, Shang XiaoJing wrote: > comedi_init() will goto out_unregister_chrdev_region if cdev_add() > failed, which won't free the resource alloced in kobject_set_name(). > Call kfree_const() to free the leaked name before goto > out_unregister_chrdev_region. > > unreferenced object 0xffff8881000fa8c0 (size 8): > comm "modprobe", pid 239, jiffies 4294905173 (age 51.308s) > hex dump (first 8 bytes): > 63 6f 6d 65 64 69 00 ff comedi.. > backtrace: > [<000000005f9878f7>] __kmalloc_node_track_caller+0x4c/0x1c0 > [<000000000fd70302>] kstrdup+0x3f/0x70 > [<000000009428bc33>] kstrdup_const+0x46/0x60 > [<00000000ed50d9de>] kvasprintf_const+0xdb/0xf0 > [<00000000b2766964>] kobject_set_name_vargs+0x3c/0xe0 > [<00000000f2424ef7>] kobject_set_name+0x62/0x90 > [<000000005d5a125b>] 0xffffffffa0013098 > [<00000000f331e663>] do_one_initcall+0x7a/0x380 > [<00000000aa7bac96>] do_init_module+0x5c/0x230 > [<000000005fd72335>] load_module+0x227d/0x2420 > [<00000000ad550cf1>] __do_sys_finit_module+0xd5/0x140 > [<00000000069a60c5>] do_syscall_64+0x3f/0x90 > [<00000000c5e0d521>] entry_SYSCALL_64_after_hwframe+0x63/0xcd > > Fixes: ed9eccbe8970 ("Staging: add comedi core") > Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com> > --- > drivers/comedi/comedi_fops.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/comedi/comedi_fops.c b/drivers/comedi/comedi_fops.c > index e2114bcf815a..2c508c2cf6f6 100644 > --- a/drivers/comedi/comedi_fops.c > +++ b/drivers/comedi/comedi_fops.c > @@ -3379,8 +3379,11 @@ static int __init comedi_init(void) > > retval = cdev_add(&comedi_cdev, MKDEV(COMEDI_MAJOR, 0), > COMEDI_NUM_MINORS); > - if (retval) > + if (retval) { > + kfree_const(comedi_cdev.kobj.name); > + comedi_cdev.kobj.name = NULL; > goto out_unregister_chrdev_region; > + } A driver should never have to poke around in the internals of a cdev object like this. Please fix the cdev core to not need this if cdev_add() fails. thanks, greg k-h
On 2022/11/1 12:45, Greg KH wrote: > On Tue, Nov 01, 2022 at 11:21:25AM +0800, Shang XiaoJing wrote: >> comedi_init() will goto out_unregister_chrdev_region if cdev_add() >> failed, which won't free the resource alloced in kobject_set_name(). >> Call kfree_const() to free the leaked name before goto >> out_unregister_chrdev_region. >> >> unreferenced object 0xffff8881000fa8c0 (size 8): >> comm "modprobe", pid 239, jiffies 4294905173 (age 51.308s) >> hex dump (first 8 bytes): >> 63 6f 6d 65 64 69 00 ff comedi.. >> backtrace: >> [<000000005f9878f7>] __kmalloc_node_track_caller+0x4c/0x1c0 >> [<000000000fd70302>] kstrdup+0x3f/0x70 >> [<000000009428bc33>] kstrdup_const+0x46/0x60 >> [<00000000ed50d9de>] kvasprintf_const+0xdb/0xf0 >> [<00000000b2766964>] kobject_set_name_vargs+0x3c/0xe0 >> [<00000000f2424ef7>] kobject_set_name+0x62/0x90 >> [<000000005d5a125b>] 0xffffffffa0013098 >> [<00000000f331e663>] do_one_initcall+0x7a/0x380 >> [<00000000aa7bac96>] do_init_module+0x5c/0x230 >> [<000000005fd72335>] load_module+0x227d/0x2420 >> [<00000000ad550cf1>] __do_sys_finit_module+0xd5/0x140 >> [<00000000069a60c5>] do_syscall_64+0x3f/0x90 >> [<00000000c5e0d521>] entry_SYSCALL_64_after_hwframe+0x63/0xcd >> >> Fixes: ed9eccbe8970 ("Staging: add comedi core") >> Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com> >> --- >> drivers/comedi/comedi_fops.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/comedi/comedi_fops.c b/drivers/comedi/comedi_fops.c >> index e2114bcf815a..2c508c2cf6f6 100644 >> --- a/drivers/comedi/comedi_fops.c >> +++ b/drivers/comedi/comedi_fops.c >> @@ -3379,8 +3379,11 @@ static int __init comedi_init(void) >> >> retval = cdev_add(&comedi_cdev, MKDEV(COMEDI_MAJOR, 0), >> COMEDI_NUM_MINORS); >> - if (retval) >> + if (retval) { >> + kfree_const(comedi_cdev.kobj.name); >> + comedi_cdev.kobj.name = NULL; >> goto out_unregister_chrdev_region; >> + } > > A driver should never have to poke around in the internals of a cdev > object like this. Please fix the cdev core to not need this if > cdev_add() fails. > Hi, I had checked all 67 places that used cdev_add(), and only the following 5 functions set the name before the cdev_add(): 1. comedi_init(), will memleak and will be fixed by this patch. 2. hfi1_cdev_init(), won't memleak. 3. qib_cdev_init(), won't memleak. 4. uio_major_init(), won't memleak. 5. __register_chrdev(), won't memleak. As the result, I just fix the code in comedi_init(). Should I still fix the cdev core code, which is free the name in cdev_add() fail path? Thanks,
On 01/11/2022 06:16, shangxiaojing wrote: > > > On 2022/11/1 12:45, Greg KH wrote: >> On Tue, Nov 01, 2022 at 11:21:25AM +0800, Shang XiaoJing wrote: >>> comedi_init() will goto out_unregister_chrdev_region if cdev_add() >>> failed, which won't free the resource alloced in kobject_set_name(). >>> Call kfree_const() to free the leaked name before goto >>> out_unregister_chrdev_region. >>> >>> unreferenced object 0xffff8881000fa8c0 (size 8): >>> comm "modprobe", pid 239, jiffies 4294905173 (age 51.308s) >>> hex dump (first 8 bytes): >>> 63 6f 6d 65 64 69 00 ff comedi.. >>> backtrace: >>> [<000000005f9878f7>] __kmalloc_node_track_caller+0x4c/0x1c0 >>> [<000000000fd70302>] kstrdup+0x3f/0x70 >>> [<000000009428bc33>] kstrdup_const+0x46/0x60 >>> [<00000000ed50d9de>] kvasprintf_const+0xdb/0xf0 >>> [<00000000b2766964>] kobject_set_name_vargs+0x3c/0xe0 >>> [<00000000f2424ef7>] kobject_set_name+0x62/0x90 >>> [<000000005d5a125b>] 0xffffffffa0013098 >>> [<00000000f331e663>] do_one_initcall+0x7a/0x380 >>> [<00000000aa7bac96>] do_init_module+0x5c/0x230 >>> [<000000005fd72335>] load_module+0x227d/0x2420 >>> [<00000000ad550cf1>] __do_sys_finit_module+0xd5/0x140 >>> [<00000000069a60c5>] do_syscall_64+0x3f/0x90 >>> [<00000000c5e0d521>] entry_SYSCALL_64_after_hwframe+0x63/0xcd >>> >>> Fixes: ed9eccbe8970 ("Staging: add comedi core") >>> Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com> >>> --- >>> drivers/comedi/comedi_fops.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/comedi/comedi_fops.c b/drivers/comedi/comedi_fops.c >>> index e2114bcf815a..2c508c2cf6f6 100644 >>> --- a/drivers/comedi/comedi_fops.c >>> +++ b/drivers/comedi/comedi_fops.c >>> @@ -3379,8 +3379,11 @@ static int __init comedi_init(void) >>> retval = cdev_add(&comedi_cdev, MKDEV(COMEDI_MAJOR, 0), >>> COMEDI_NUM_MINORS); >>> - if (retval) >>> + if (retval) { >>> + kfree_const(comedi_cdev.kobj.name); >>> + comedi_cdev.kobj.name = NULL; >>> goto out_unregister_chrdev_region; >>> + } >> >> A driver should never have to poke around in the internals of a cdev >> object like this. Please fix the cdev core to not need this if >> cdev_add() fails. Or at least there should be a function that can be called to undo the allocations of kobject_set_name(). >> > Hi, > > I had checked all 67 places that used cdev_add(), and only the following > 5 functions set the name before the cdev_add(): > > 1. comedi_init(), will memleak and will be fixed by this patch. > 2. hfi1_cdev_init(), won't memleak. > 3. qib_cdev_init(), won't memleak. > 4. uio_major_init(), won't memleak. > 5. __register_chrdev(), won't memleak. > > As the result, I just fix the code in comedi_init(). Should I still fix > the cdev core code, which is free the name in cdev_add() fail path? I'm stuck trying to work out why hfi1_cdev_init() doesn't have the same problem when cdev_add() returns an error.
On 2022/11/1 19:40, Ian Abbott wrote: > On 01/11/2022 06:16, shangxiaojing wrote: >> >> >> On 2022/11/1 12:45, Greg KH wrote: >>> On Tue, Nov 01, 2022 at 11:21:25AM +0800, Shang XiaoJing wrote: >>>> comedi_init() will goto out_unregister_chrdev_region if cdev_add() >>>> failed, which won't free the resource alloced in kobject_set_name(). >>>> Call kfree_const() to free the leaked name before goto >>>> out_unregister_chrdev_region. >>>> >>>> unreferenced object 0xffff8881000fa8c0 (size 8): >>>> comm "modprobe", pid 239, jiffies 4294905173 (age 51.308s) >>>> hex dump (first 8 bytes): >>>> 63 6f 6d 65 64 69 00 ff comedi.. >>>> backtrace: >>>> [<000000005f9878f7>] __kmalloc_node_track_caller+0x4c/0x1c0 >>>> [<000000000fd70302>] kstrdup+0x3f/0x70 >>>> [<000000009428bc33>] kstrdup_const+0x46/0x60 >>>> [<00000000ed50d9de>] kvasprintf_const+0xdb/0xf0 >>>> [<00000000b2766964>] kobject_set_name_vargs+0x3c/0xe0 >>>> [<00000000f2424ef7>] kobject_set_name+0x62/0x90 >>>> [<000000005d5a125b>] 0xffffffffa0013098 >>>> [<00000000f331e663>] do_one_initcall+0x7a/0x380 >>>> [<00000000aa7bac96>] do_init_module+0x5c/0x230 >>>> [<000000005fd72335>] load_module+0x227d/0x2420 >>>> [<00000000ad550cf1>] __do_sys_finit_module+0xd5/0x140 >>>> [<00000000069a60c5>] do_syscall_64+0x3f/0x90 >>>> [<00000000c5e0d521>] entry_SYSCALL_64_after_hwframe+0x63/0xcd >>>> >>>> Fixes: ed9eccbe8970 ("Staging: add comedi core") >>>> Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com> >>>> --- >>>> drivers/comedi/comedi_fops.c | 5 ++++- >>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/comedi/comedi_fops.c >>>> b/drivers/comedi/comedi_fops.c >>>> index e2114bcf815a..2c508c2cf6f6 100644 >>>> --- a/drivers/comedi/comedi_fops.c >>>> +++ b/drivers/comedi/comedi_fops.c >>>> @@ -3379,8 +3379,11 @@ static int __init comedi_init(void) >>>> retval = cdev_add(&comedi_cdev, MKDEV(COMEDI_MAJOR, 0), >>>> COMEDI_NUM_MINORS); >>>> - if (retval) >>>> + if (retval) { >>>> + kfree_const(comedi_cdev.kobj.name); >>>> + comedi_cdev.kobj.name = NULL; >>>> goto out_unregister_chrdev_region; >>>> + } >>> >>> A driver should never have to poke around in the internals of a cdev >>> object like this. Please fix the cdev core to not need this if >>> cdev_add() fails. > > Or at least there should be a function that can be called to undo the > allocations of kobject_set_name(). It's ok to add a pair function, but maybe only one place where used cdev_add() need free name. Your mean all "kfree_const(name);" should be replaced to the new function? > >>> >> Hi, >> >> I had checked all 67 places that used cdev_add(), and only the >> following 5 functions set the name before the cdev_add(): >> >> 1. comedi_init(), will memleak and will be fixed by this patch. >> 2. hfi1_cdev_init(), won't memleak. >> 3. qib_cdev_init(), won't memleak. >> 4. uio_major_init(), won't memleak. >> 5. __register_chrdev(), won't memleak. >> >> As the result, I just fix the code in comedi_init(). Should I still >> fix the cdev core code, which is free the name in cdev_add() fail path? > > I'm stuck trying to work out why hfi1_cdev_init() doesn't have the same > problem when cdev_add() returns an error. > Only user_add() calls the hfi1_cdev_init(), and will call user_remove() if hfi1_cdev_init() failed. In user_remove(), hfi1_cdev_cleanup() will call cdev_del(). Thanks,
On 01/11/2022 11:57, shangxiaojing wrote: > > > On 2022/11/1 19:40, Ian Abbott wrote: >> On 01/11/2022 06:16, shangxiaojing wrote: >>> >>> >>> On 2022/11/1 12:45, Greg KH wrote: >>>> On Tue, Nov 01, 2022 at 11:21:25AM +0800, Shang XiaoJing wrote: >>>>> comedi_init() will goto out_unregister_chrdev_region if cdev_add() >>>>> failed, which won't free the resource alloced in kobject_set_name(). >>>>> Call kfree_const() to free the leaked name before goto >>>>> out_unregister_chrdev_region. >>>>> >>>>> unreferenced object 0xffff8881000fa8c0 (size 8): >>>>> comm "modprobe", pid 239, jiffies 4294905173 (age 51.308s) >>>>> hex dump (first 8 bytes): >>>>> 63 6f 6d 65 64 69 00 ff comedi.. >>>>> backtrace: >>>>> [<000000005f9878f7>] __kmalloc_node_track_caller+0x4c/0x1c0 >>>>> [<000000000fd70302>] kstrdup+0x3f/0x70 >>>>> [<000000009428bc33>] kstrdup_const+0x46/0x60 >>>>> [<00000000ed50d9de>] kvasprintf_const+0xdb/0xf0 >>>>> [<00000000b2766964>] kobject_set_name_vargs+0x3c/0xe0 >>>>> [<00000000f2424ef7>] kobject_set_name+0x62/0x90 >>>>> [<000000005d5a125b>] 0xffffffffa0013098 >>>>> [<00000000f331e663>] do_one_initcall+0x7a/0x380 >>>>> [<00000000aa7bac96>] do_init_module+0x5c/0x230 >>>>> [<000000005fd72335>] load_module+0x227d/0x2420 >>>>> [<00000000ad550cf1>] __do_sys_finit_module+0xd5/0x140 >>>>> [<00000000069a60c5>] do_syscall_64+0x3f/0x90 >>>>> [<00000000c5e0d521>] entry_SYSCALL_64_after_hwframe+0x63/0xcd >>>>> >>>>> Fixes: ed9eccbe8970 ("Staging: add comedi core") >>>>> Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com> >>>>> --- >>>>> drivers/comedi/comedi_fops.c | 5 ++++- >>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/comedi/comedi_fops.c >>>>> b/drivers/comedi/comedi_fops.c >>>>> index e2114bcf815a..2c508c2cf6f6 100644 >>>>> --- a/drivers/comedi/comedi_fops.c >>>>> +++ b/drivers/comedi/comedi_fops.c >>>>> @@ -3379,8 +3379,11 @@ static int __init comedi_init(void) >>>>> retval = cdev_add(&comedi_cdev, MKDEV(COMEDI_MAJOR, 0), >>>>> COMEDI_NUM_MINORS); >>>>> - if (retval) >>>>> + if (retval) { >>>>> + kfree_const(comedi_cdev.kobj.name); >>>>> + comedi_cdev.kobj.name = NULL; >>>>> goto out_unregister_chrdev_region; >>>>> + } >>>> >>>> A driver should never have to poke around in the internals of a cdev >>>> object like this. Please fix the cdev core to not need this if >>>> cdev_add() fails. >> >> Or at least there should be a function that can be called to undo the >> allocations of kobject_set_name(). > > It's ok to add a pair function, but maybe only one place where used > cdev_add() need free name. Your mean all "kfree_const(name);" should be > replaced to the new function? > >> >>>> >>> Hi, >>> >>> I had checked all 67 places that used cdev_add(), and only the >>> following 5 functions set the name before the cdev_add(): >>> >>> 1. comedi_init(), will memleak and will be fixed by this patch. >>> 2. hfi1_cdev_init(), won't memleak. >>> 3. qib_cdev_init(), won't memleak. >>> 4. uio_major_init(), won't memleak. >>> 5. __register_chrdev(), won't memleak. >>> >>> As the result, I just fix the code in comedi_init(). Should I still >>> fix the cdev core code, which is free the name in cdev_add() fail path? >> >> I'm stuck trying to work out why hfi1_cdev_init() doesn't have the >> same problem when cdev_add() returns an error. >> > > Only user_add() calls the hfi1_cdev_init(), and will call user_remove() > if hfi1_cdev_init() failed. In user_remove(), hfi1_cdev_cleanup() will > call cdev_del(). No it won't. hfi1_cdev_cleanup() only calls cdev_del() if device is non-null, but device will be null if the call to cdev_add() failed in hfi1_cdev_init().
On 01/11/2022 11:40, Ian Abbott wrote: > On 01/11/2022 06:16, shangxiaojing wrote: >> >> >> On 2022/11/1 12:45, Greg KH wrote: >>> On Tue, Nov 01, 2022 at 11:21:25AM +0800, Shang XiaoJing wrote: >>>> comedi_init() will goto out_unregister_chrdev_region if cdev_add() >>>> failed, which won't free the resource alloced in kobject_set_name(). >>>> Call kfree_const() to free the leaked name before goto >>>> out_unregister_chrdev_region. >>>> >>>> unreferenced object 0xffff8881000fa8c0 (size 8): >>>> comm "modprobe", pid 239, jiffies 4294905173 (age 51.308s) >>>> hex dump (first 8 bytes): >>>> 63 6f 6d 65 64 69 00 ff comedi.. >>>> backtrace: >>>> [<000000005f9878f7>] __kmalloc_node_track_caller+0x4c/0x1c0 >>>> [<000000000fd70302>] kstrdup+0x3f/0x70 >>>> [<000000009428bc33>] kstrdup_const+0x46/0x60 >>>> [<00000000ed50d9de>] kvasprintf_const+0xdb/0xf0 >>>> [<00000000b2766964>] kobject_set_name_vargs+0x3c/0xe0 >>>> [<00000000f2424ef7>] kobject_set_name+0x62/0x90 >>>> [<000000005d5a125b>] 0xffffffffa0013098 >>>> [<00000000f331e663>] do_one_initcall+0x7a/0x380 >>>> [<00000000aa7bac96>] do_init_module+0x5c/0x230 >>>> [<000000005fd72335>] load_module+0x227d/0x2420 >>>> [<00000000ad550cf1>] __do_sys_finit_module+0xd5/0x140 >>>> [<00000000069a60c5>] do_syscall_64+0x3f/0x90 >>>> [<00000000c5e0d521>] entry_SYSCALL_64_after_hwframe+0x63/0xcd >>>> >>>> Fixes: ed9eccbe8970 ("Staging: add comedi core") >>>> Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com> >>>> --- >>>> drivers/comedi/comedi_fops.c | 5 ++++- >>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/comedi/comedi_fops.c >>>> b/drivers/comedi/comedi_fops.c >>>> index e2114bcf815a..2c508c2cf6f6 100644 >>>> --- a/drivers/comedi/comedi_fops.c >>>> +++ b/drivers/comedi/comedi_fops.c >>>> @@ -3379,8 +3379,11 @@ static int __init comedi_init(void) >>>> retval = cdev_add(&comedi_cdev, MKDEV(COMEDI_MAJOR, 0), >>>> COMEDI_NUM_MINORS); >>>> - if (retval) >>>> + if (retval) { >>>> + kfree_const(comedi_cdev.kobj.name); >>>> + comedi_cdev.kobj.name = NULL; >>>> goto out_unregister_chrdev_region; >>>> + } >>> >>> A driver should never have to poke around in the internals of a cdev >>> object like this. Please fix the cdev core to not need this if >>> cdev_add() fails. > > Or at least there should be a function that can be called to undo the > allocations of kobject_set_name(). Looking at it a bit more, cdev_init() calls kobject_init(), and kobject_init() requires a matching call to kobject_put(). Nothing is calling kobject_put() in this situation. Calling cdev_del() will call kobject_put(), so is that the correct way to clean up after cdev_init() if the call to cdev_add() fails?
On 2022/11/1 23:59, Ian Abbott wrote: > On 01/11/2022 11:57, shangxiaojing wrote: >> >> >> On 2022/11/1 19:40, Ian Abbott wrote: >>> On 01/11/2022 06:16, shangxiaojing wrote: >>>> >>>> >>>> On 2022/11/1 12:45, Greg KH wrote: >>>>> On Tue, Nov 01, 2022 at 11:21:25AM +0800, Shang XiaoJing wrote: >>>>>> comedi_init() will goto out_unregister_chrdev_region if cdev_add() >>>>>> failed, which won't free the resource alloced in kobject_set_name(). >>>>>> Call kfree_const() to free the leaked name before goto >>>>>> out_unregister_chrdev_region. >>>>>> >>>>>> unreferenced object 0xffff8881000fa8c0 (size 8): >>>>>> comm "modprobe", pid 239, jiffies 4294905173 (age 51.308s) >>>>>> hex dump (first 8 bytes): >>>>>> 63 6f 6d 65 64 69 00 ff comedi.. >>>>>> backtrace: >>>>>> [<000000005f9878f7>] __kmalloc_node_track_caller+0x4c/0x1c0 >>>>>> [<000000000fd70302>] kstrdup+0x3f/0x70 >>>>>> [<000000009428bc33>] kstrdup_const+0x46/0x60 >>>>>> [<00000000ed50d9de>] kvasprintf_const+0xdb/0xf0 >>>>>> [<00000000b2766964>] kobject_set_name_vargs+0x3c/0xe0 >>>>>> [<00000000f2424ef7>] kobject_set_name+0x62/0x90 >>>>>> [<000000005d5a125b>] 0xffffffffa0013098 >>>>>> [<00000000f331e663>] do_one_initcall+0x7a/0x380 >>>>>> [<00000000aa7bac96>] do_init_module+0x5c/0x230 >>>>>> [<000000005fd72335>] load_module+0x227d/0x2420 >>>>>> [<00000000ad550cf1>] __do_sys_finit_module+0xd5/0x140 >>>>>> [<00000000069a60c5>] do_syscall_64+0x3f/0x90 >>>>>> [<00000000c5e0d521>] entry_SYSCALL_64_after_hwframe+0x63/0xcd >>>>>> >>>>>> Fixes: ed9eccbe8970 ("Staging: add comedi core") >>>>>> Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com> >>>>>> --- >>>>>> drivers/comedi/comedi_fops.c | 5 ++++- >>>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/comedi/comedi_fops.c >>>>>> b/drivers/comedi/comedi_fops.c >>>>>> index e2114bcf815a..2c508c2cf6f6 100644 >>>>>> --- a/drivers/comedi/comedi_fops.c >>>>>> +++ b/drivers/comedi/comedi_fops.c >>>>>> @@ -3379,8 +3379,11 @@ static int __init comedi_init(void) >>>>>> retval = cdev_add(&comedi_cdev, MKDEV(COMEDI_MAJOR, 0), >>>>>> COMEDI_NUM_MINORS); >>>>>> - if (retval) >>>>>> + if (retval) { >>>>>> + kfree_const(comedi_cdev.kobj.name); >>>>>> + comedi_cdev.kobj.name = NULL; >>>>>> goto out_unregister_chrdev_region; >>>>>> + } >>>>> >>>>> A driver should never have to poke around in the internals of a cdev >>>>> object like this. Please fix the cdev core to not need this if >>>>> cdev_add() fails. >>> >>> Or at least there should be a function that can be called to undo the >>> allocations of kobject_set_name(). >> >> It's ok to add a pair function, but maybe only one place where used >> cdev_add() need free name. Your mean all "kfree_const(name);" should >> be replaced to the new function? >> >>> >>>>> >>>> Hi, >>>> >>>> I had checked all 67 places that used cdev_add(), and only the >>>> following 5 functions set the name before the cdev_add(): >>>> >>>> 1. comedi_init(), will memleak and will be fixed by this patch. >>>> 2. hfi1_cdev_init(), won't memleak. >>>> 3. qib_cdev_init(), won't memleak. >>>> 4. uio_major_init(), won't memleak. >>>> 5. __register_chrdev(), won't memleak. >>>> >>>> As the result, I just fix the code in comedi_init(). Should I still >>>> fix the cdev core code, which is free the name in cdev_add() fail path? >>> >>> I'm stuck trying to work out why hfi1_cdev_init() doesn't have the >>> same problem when cdev_add() returns an error. >>> >> >> Only user_add() calls the hfi1_cdev_init(), and will call >> user_remove() if hfi1_cdev_init() failed. In user_remove(), >> hfi1_cdev_cleanup() will call cdev_del(). > > No it won't. hfi1_cdev_cleanup() only calls cdev_del() if device is > non-null, but device will be null if the call to cdev_add() failed in > hfi1_cdev_init(). > Right, device won't be inited, so there has the same potential memleak. Thanks,
On 2022/11/2 0:41, Ian Abbott wrote: > On 01/11/2022 11:40, Ian Abbott wrote: >> On 01/11/2022 06:16, shangxiaojing wrote: >>> >>> >>> On 2022/11/1 12:45, Greg KH wrote: >>>> On Tue, Nov 01, 2022 at 11:21:25AM +0800, Shang XiaoJing wrote: >>>>> comedi_init() will goto out_unregister_chrdev_region if cdev_add() >>>>> failed, which won't free the resource alloced in kobject_set_name(). >>>>> Call kfree_const() to free the leaked name before goto >>>>> out_unregister_chrdev_region. >>>>> >>>>> unreferenced object 0xffff8881000fa8c0 (size 8): >>>>> comm "modprobe", pid 239, jiffies 4294905173 (age 51.308s) >>>>> hex dump (first 8 bytes): >>>>> 63 6f 6d 65 64 69 00 ff comedi.. >>>>> backtrace: >>>>> [<000000005f9878f7>] __kmalloc_node_track_caller+0x4c/0x1c0 >>>>> [<000000000fd70302>] kstrdup+0x3f/0x70 >>>>> [<000000009428bc33>] kstrdup_const+0x46/0x60 >>>>> [<00000000ed50d9de>] kvasprintf_const+0xdb/0xf0 >>>>> [<00000000b2766964>] kobject_set_name_vargs+0x3c/0xe0 >>>>> [<00000000f2424ef7>] kobject_set_name+0x62/0x90 >>>>> [<000000005d5a125b>] 0xffffffffa0013098 >>>>> [<00000000f331e663>] do_one_initcall+0x7a/0x380 >>>>> [<00000000aa7bac96>] do_init_module+0x5c/0x230 >>>>> [<000000005fd72335>] load_module+0x227d/0x2420 >>>>> [<00000000ad550cf1>] __do_sys_finit_module+0xd5/0x140 >>>>> [<00000000069a60c5>] do_syscall_64+0x3f/0x90 >>>>> [<00000000c5e0d521>] entry_SYSCALL_64_after_hwframe+0x63/0xcd >>>>> >>>>> Fixes: ed9eccbe8970 ("Staging: add comedi core") >>>>> Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com> >>>>> --- >>>>> drivers/comedi/comedi_fops.c | 5 ++++- >>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/comedi/comedi_fops.c >>>>> b/drivers/comedi/comedi_fops.c >>>>> index e2114bcf815a..2c508c2cf6f6 100644 >>>>> --- a/drivers/comedi/comedi_fops.c >>>>> +++ b/drivers/comedi/comedi_fops.c >>>>> @@ -3379,8 +3379,11 @@ static int __init comedi_init(void) >>>>> retval = cdev_add(&comedi_cdev, MKDEV(COMEDI_MAJOR, 0), >>>>> COMEDI_NUM_MINORS); >>>>> - if (retval) >>>>> + if (retval) { >>>>> + kfree_const(comedi_cdev.kobj.name); >>>>> + comedi_cdev.kobj.name = NULL; >>>>> goto out_unregister_chrdev_region; >>>>> + } >>>> >>>> A driver should never have to poke around in the internals of a cdev >>>> object like this. Please fix the cdev core to not need this if >>>> cdev_add() fails. >> >> Or at least there should be a function that can be called to undo the >> allocations of kobject_set_name(). > > Looking at it a bit more, cdev_init() calls kobject_init(), and > kobject_init() requires a matching call to kobject_put(). Nothing is > calling kobject_put() in this situation. Calling cdev_del() will call > kobject_put(), so is that the correct way to clean up after cdev_init() > if the call to cdev_add() fails? > Some cdev call cdev_del() when cdev_add() failed (like init_dvbdev()), and at that time the cdev_unmap() in cdev_del() won't do anything due to the failure of cdev_add(), which is calling the kobject_put() when cdev_add() failed. But I'm not sure which one is better. Thanks,
On Wed, Nov 02, 2022 at 10:51:35AM +0800, shangxiaojing wrote: > > > On 2022/11/2 0:41, Ian Abbott wrote: > > On 01/11/2022 11:40, Ian Abbott wrote: > > > On 01/11/2022 06:16, shangxiaojing wrote: > > > > > > > > > > > > On 2022/11/1 12:45, Greg KH wrote: > > > > > On Tue, Nov 01, 2022 at 11:21:25AM +0800, Shang XiaoJing wrote: > > > > > > comedi_init() will goto out_unregister_chrdev_region if cdev_add() > > > > > > failed, which won't free the resource alloced in kobject_set_name(). > > > > > > Call kfree_const() to free the leaked name before goto > > > > > > out_unregister_chrdev_region. > > > > > > > > > > > > unreferenced object 0xffff8881000fa8c0 (size 8): > > > > > > comm "modprobe", pid 239, jiffies 4294905173 (age 51.308s) > > > > > > hex dump (first 8 bytes): > > > > > > 63 6f 6d 65 64 69 00 ff comedi.. > > > > > > backtrace: > > > > > > [<000000005f9878f7>] __kmalloc_node_track_caller+0x4c/0x1c0 > > > > > > [<000000000fd70302>] kstrdup+0x3f/0x70 > > > > > > [<000000009428bc33>] kstrdup_const+0x46/0x60 > > > > > > [<00000000ed50d9de>] kvasprintf_const+0xdb/0xf0 > > > > > > [<00000000b2766964>] kobject_set_name_vargs+0x3c/0xe0 > > > > > > [<00000000f2424ef7>] kobject_set_name+0x62/0x90 > > > > > > [<000000005d5a125b>] 0xffffffffa0013098 > > > > > > [<00000000f331e663>] do_one_initcall+0x7a/0x380 > > > > > > [<00000000aa7bac96>] do_init_module+0x5c/0x230 > > > > > > [<000000005fd72335>] load_module+0x227d/0x2420 > > > > > > [<00000000ad550cf1>] __do_sys_finit_module+0xd5/0x140 > > > > > > [<00000000069a60c5>] do_syscall_64+0x3f/0x90 > > > > > > [<00000000c5e0d521>] entry_SYSCALL_64_after_hwframe+0x63/0xcd > > > > > > > > > > > > Fixes: ed9eccbe8970 ("Staging: add comedi core") > > > > > > Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com> > > > > > > --- > > > > > > drivers/comedi/comedi_fops.c | 5 ++++- > > > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/comedi/comedi_fops.c > > > > > > b/drivers/comedi/comedi_fops.c > > > > > > index e2114bcf815a..2c508c2cf6f6 100644 > > > > > > --- a/drivers/comedi/comedi_fops.c > > > > > > +++ b/drivers/comedi/comedi_fops.c > > > > > > @@ -3379,8 +3379,11 @@ static int __init comedi_init(void) > > > > > > retval = cdev_add(&comedi_cdev, MKDEV(COMEDI_MAJOR, 0), > > > > > > COMEDI_NUM_MINORS); > > > > > > - if (retval) > > > > > > + if (retval) { > > > > > > + kfree_const(comedi_cdev.kobj.name); > > > > > > + comedi_cdev.kobj.name = NULL; > > > > > > goto out_unregister_chrdev_region; > > > > > > + } > > > > > > > > > > A driver should never have to poke around in the internals of a cdev > > > > > object like this. Please fix the cdev core to not need this if > > > > > cdev_add() fails. > > > > > > Or at least there should be a function that can be called to undo > > > the allocations of kobject_set_name(). > > > > Looking at it a bit more, cdev_init() calls kobject_init(), and > > kobject_init() requires a matching call to kobject_put(). Nothing is > > calling kobject_put() in this situation. Calling cdev_del() will call > > kobject_put(), so is that the correct way to clean up after cdev_init() > > if the call to cdev_add() fails? > > > > Some cdev call cdev_del() when cdev_add() failed (like init_dvbdev()), and > at that time the cdev_unmap() in cdev_del() won't do anything due to the > failure of cdev_add(), which is calling the kobject_put() when cdev_add() > failed. But I'm not sure which one is better. My point is that no one calling the cdev_add() call should have to do this type of "cleanup" The cdev code should do it automatically, we don't want to have to audit every user of cdev_add() today, and in the future for forever, to ensure they got it right. Let's fix it up in the cdev code itself please. The kobject in a cdev is a very odd thing, it's not the "normal" type of kobject that you expect, so the cdev code should handle all of that on its own and that should not "leak" into any caller. thanks, greg k-h
On 2022/11/2 10:59, Greg KH wrote: > On Wed, Nov 02, 2022 at 10:51:35AM +0800, shangxiaojing wrote: >> >> >> On 2022/11/2 0:41, Ian Abbott wrote: >>> On 01/11/2022 11:40, Ian Abbott wrote: >>>> On 01/11/2022 06:16, shangxiaojing wrote: >>>>> >>>>> >>>>> On 2022/11/1 12:45, Greg KH wrote: >>>>>> On Tue, Nov 01, 2022 at 11:21:25AM +0800, Shang XiaoJing wrote: >>>>>>> comedi_init() will goto out_unregister_chrdev_region if cdev_add() >>>>>>> failed, which won't free the resource alloced in kobject_set_name(). >>>>>>> Call kfree_const() to free the leaked name before goto >>>>>>> out_unregister_chrdev_region. >>>>>>> >>>>>>> unreferenced object 0xffff8881000fa8c0 (size 8): >>>>>>> comm "modprobe", pid 239, jiffies 4294905173 (age 51.308s) >>>>>>> hex dump (first 8 bytes): >>>>>>> 63 6f 6d 65 64 69 00 ff comedi.. >>>>>>> backtrace: >>>>>>> [<000000005f9878f7>] __kmalloc_node_track_caller+0x4c/0x1c0 >>>>>>> [<000000000fd70302>] kstrdup+0x3f/0x70 >>>>>>> [<000000009428bc33>] kstrdup_const+0x46/0x60 >>>>>>> [<00000000ed50d9de>] kvasprintf_const+0xdb/0xf0 >>>>>>> [<00000000b2766964>] kobject_set_name_vargs+0x3c/0xe0 >>>>>>> [<00000000f2424ef7>] kobject_set_name+0x62/0x90 >>>>>>> [<000000005d5a125b>] 0xffffffffa0013098 >>>>>>> [<00000000f331e663>] do_one_initcall+0x7a/0x380 >>>>>>> [<00000000aa7bac96>] do_init_module+0x5c/0x230 >>>>>>> [<000000005fd72335>] load_module+0x227d/0x2420 >>>>>>> [<00000000ad550cf1>] __do_sys_finit_module+0xd5/0x140 >>>>>>> [<00000000069a60c5>] do_syscall_64+0x3f/0x90 >>>>>>> [<00000000c5e0d521>] entry_SYSCALL_64_after_hwframe+0x63/0xcd >>>>>>> >>>>>>> Fixes: ed9eccbe8970 ("Staging: add comedi core") >>>>>>> Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com> >>>>>>> --- >>>>>>> drivers/comedi/comedi_fops.c | 5 ++++- >>>>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/comedi/comedi_fops.c >>>>>>> b/drivers/comedi/comedi_fops.c >>>>>>> index e2114bcf815a..2c508c2cf6f6 100644 >>>>>>> --- a/drivers/comedi/comedi_fops.c >>>>>>> +++ b/drivers/comedi/comedi_fops.c >>>>>>> @@ -3379,8 +3379,11 @@ static int __init comedi_init(void) >>>>>>> retval = cdev_add(&comedi_cdev, MKDEV(COMEDI_MAJOR, 0), >>>>>>> COMEDI_NUM_MINORS); >>>>>>> - if (retval) >>>>>>> + if (retval) { >>>>>>> + kfree_const(comedi_cdev.kobj.name); >>>>>>> + comedi_cdev.kobj.name = NULL; >>>>>>> goto out_unregister_chrdev_region; >>>>>>> + } >>>>>> >>>>>> A driver should never have to poke around in the internals of a cdev >>>>>> object like this. Please fix the cdev core to not need this if >>>>>> cdev_add() fails. >>>> >>>> Or at least there should be a function that can be called to undo >>>> the allocations of kobject_set_name(). >>> >>> Looking at it a bit more, cdev_init() calls kobject_init(), and >>> kobject_init() requires a matching call to kobject_put(). Nothing is >>> calling kobject_put() in this situation. Calling cdev_del() will call >>> kobject_put(), so is that the correct way to clean up after cdev_init() >>> if the call to cdev_add() fails? >>> >> >> Some cdev call cdev_del() when cdev_add() failed (like init_dvbdev()), and >> at that time the cdev_unmap() in cdev_del() won't do anything due to the >> failure of cdev_add(), which is calling the kobject_put() when cdev_add() >> failed. But I'm not sure which one is better. > > My point is that no one calling the cdev_add() call should have to do > this type of "cleanup" The cdev code should do it automatically, we > don't want to have to audit every user of cdev_add() today, and in the > future for forever, to ensure they got it right. > > Let's fix it up in the cdev code itself please. The kobject in a cdev > is a very odd thing, it's not the "normal" type of kobject that you > expect, so the cdev code should handle all of that on its own and that > should not "leak" into any caller. > ok, I'll fix in cdev code and send v2. Thanks,
diff --git a/drivers/comedi/comedi_fops.c b/drivers/comedi/comedi_fops.c index e2114bcf815a..2c508c2cf6f6 100644 --- a/drivers/comedi/comedi_fops.c +++ b/drivers/comedi/comedi_fops.c @@ -3379,8 +3379,11 @@ static int __init comedi_init(void) retval = cdev_add(&comedi_cdev, MKDEV(COMEDI_MAJOR, 0), COMEDI_NUM_MINORS); - if (retval) + if (retval) { + kfree_const(comedi_cdev.kobj.name); + comedi_cdev.kobj.name = NULL; goto out_unregister_chrdev_region; + } comedi_class = class_create(THIS_MODULE, "comedi"); if (IS_ERR(comedi_class)) {