Message ID | 20221018122451.1749171-1-yangyingliang@huawei.com (mailing list archive) |
---|---|
State | Accepted |
Commit | ff2f5ec5d009844ec28f171123f9e58750cef4bf |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | [net] net: hns: fix possible memory leak in hnae_ae_register() | expand |
On Tue, Oct 18, 2022 at 08:24:51PM +0800, Yang Yingliang wrote: > Inject fault while probing module, if device_register() fails, > but the refcount of kobject is not decreased to 0, the name > allocated in dev_set_name() is leaked. Fix this by calling > put_device(), so that name can be freed in callback function > kobject_cleanup(). > > unreferenced object 0xffff00c01aba2100 (size 128): > comm "systemd-udevd", pid 1259, jiffies 4294903284 (age 294.152s) > hex dump (first 32 bytes): > 68 6e 61 65 30 00 00 00 18 21 ba 1a c0 00 ff ff hnae0....!...... > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace: > [<0000000034783f26>] slab_post_alloc_hook+0xa0/0x3e0 > [<00000000748188f2>] __kmem_cache_alloc_node+0x164/0x2b0 > [<00000000ab0743e8>] __kmalloc_node_track_caller+0x6c/0x390 > [<000000006c0ffb13>] kvasprintf+0x8c/0x118 > [<00000000fa27bfe1>] kvasprintf_const+0x60/0xc8 > [<0000000083e10ed7>] kobject_set_name_vargs+0x3c/0xc0 > [<000000000b87affc>] dev_set_name+0x7c/0xa0 > [<000000003fd8fe26>] hnae_ae_register+0xcc/0x190 [hnae] > [<00000000fe97edc9>] hns_dsaf_ae_init+0x9c/0x108 [hns_dsaf] > [<00000000c36ff1eb>] hns_dsaf_probe+0x548/0x748 [hns_dsaf] > > Fixes: 6fe6611ff275 ("net: add Hisilicon Network Subsystem hnae framework support") > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > --- > drivers/net/ethernet/hisilicon/hns/hnae.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/hisilicon/hns/hnae.c b/drivers/net/ethernet/hisilicon/hns/hnae.c > index 00fafc0f8512..430eccea8e5e 100644 > --- a/drivers/net/ethernet/hisilicon/hns/hnae.c > +++ b/drivers/net/ethernet/hisilicon/hns/hnae.c > @@ -419,8 +419,10 @@ int hnae_ae_register(struct hnae_ae_dev *hdev, struct module *owner) > hdev->cls_dev.release = hnae_release; > (void)dev_set_name(&hdev->cls_dev, "hnae%d", hdev->id); ^^^^^^^^^^^^ this is unexpected in netdev code. > ret = device_register(&hdev->cls_dev); > - if (ret) > + if (ret) { > + put_device(&hdev->cls_dev); > return ret; > + } > > __module_get(THIS_MODULE); ^^^^^^^^ I'm curious why? I don't see why you need this. The change itself is ok. Thanks, Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
On Tue, 18 Oct 2022 15:58:38 +0300 Leon Romanovsky wrote: > The change itself is ok. Also the .release function is empty which is another bad smell? > Reviewed-by: Leon Romanovsky <leonro@nvidia.com> Thanks!
Hello: This patch was applied to netdev/net.git (master) by Jakub Kicinski <kuba@kernel.org>: On Tue, 18 Oct 2022 20:24:51 +0800 you wrote: > Inject fault while probing module, if device_register() fails, > but the refcount of kobject is not decreased to 0, the name > allocated in dev_set_name() is leaked. Fix this by calling > put_device(), so that name can be freed in callback function > kobject_cleanup(). > > unreferenced object 0xffff00c01aba2100 (size 128): > comm "systemd-udevd", pid 1259, jiffies 4294903284 (age 294.152s) > hex dump (first 32 bytes): > 68 6e 61 65 30 00 00 00 18 21 ba 1a c0 00 ff ff hnae0....!...... > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace: > [<0000000034783f26>] slab_post_alloc_hook+0xa0/0x3e0 > [<00000000748188f2>] __kmem_cache_alloc_node+0x164/0x2b0 > [<00000000ab0743e8>] __kmalloc_node_track_caller+0x6c/0x390 > [<000000006c0ffb13>] kvasprintf+0x8c/0x118 > [<00000000fa27bfe1>] kvasprintf_const+0x60/0xc8 > [<0000000083e10ed7>] kobject_set_name_vargs+0x3c/0xc0 > [<000000000b87affc>] dev_set_name+0x7c/0xa0 > [<000000003fd8fe26>] hnae_ae_register+0xcc/0x190 [hnae] > [<00000000fe97edc9>] hns_dsaf_ae_init+0x9c/0x108 [hns_dsaf] > [<00000000c36ff1eb>] hns_dsaf_probe+0x548/0x748 [hns_dsaf] > > [...] Here is the summary with links: - [net] net: hns: fix possible memory leak in hnae_ae_register() https://git.kernel.org/netdev/net/c/ff2f5ec5d009 You are awesome, thank you!
On 2022/10/18 20:58, Leon Romanovsky wrote: > On Tue, Oct 18, 2022 at 08:24:51PM +0800, Yang Yingliang wrote: >> Inject fault while probing module, if device_register() fails, >> but the refcount of kobject is not decreased to 0, the name >> allocated in dev_set_name() is leaked. Fix this by calling >> put_device(), so that name can be freed in callback function >> kobject_cleanup(). >> >> unreferenced object 0xffff00c01aba2100 (size 128): >> comm "systemd-udevd", pid 1259, jiffies 4294903284 (age 294.152s) >> hex dump (first 32 bytes): >> 68 6e 61 65 30 00 00 00 18 21 ba 1a c0 00 ff ff hnae0....!...... >> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ >> backtrace: >> [<0000000034783f26>] slab_post_alloc_hook+0xa0/0x3e0 >> [<00000000748188f2>] __kmem_cache_alloc_node+0x164/0x2b0 >> [<00000000ab0743e8>] __kmalloc_node_track_caller+0x6c/0x390 >> [<000000006c0ffb13>] kvasprintf+0x8c/0x118 >> [<00000000fa27bfe1>] kvasprintf_const+0x60/0xc8 >> [<0000000083e10ed7>] kobject_set_name_vargs+0x3c/0xc0 >> [<000000000b87affc>] dev_set_name+0x7c/0xa0 >> [<000000003fd8fe26>] hnae_ae_register+0xcc/0x190 [hnae] >> [<00000000fe97edc9>] hns_dsaf_ae_init+0x9c/0x108 [hns_dsaf] >> [<00000000c36ff1eb>] hns_dsaf_probe+0x548/0x748 [hns_dsaf] >> >> Fixes: 6fe6611ff275 ("net: add Hisilicon Network Subsystem hnae framework support") >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> >> --- >> drivers/net/ethernet/hisilicon/hns/hnae.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/hisilicon/hns/hnae.c b/drivers/net/ethernet/hisilicon/hns/hnae.c >> index 00fafc0f8512..430eccea8e5e 100644 >> --- a/drivers/net/ethernet/hisilicon/hns/hnae.c >> +++ b/drivers/net/ethernet/hisilicon/hns/hnae.c >> @@ -419,8 +419,10 @@ int hnae_ae_register(struct hnae_ae_dev *hdev, struct module *owner) >> hdev->cls_dev.release = hnae_release; >> (void)dev_set_name(&hdev->cls_dev, "hnae%d", hdev->id); > ^^^^^^^^^^^^ this is unexpected in netdev code. Did you mean the 'void' can be removed ? > >> ret = device_register(&hdev->cls_dev); >> - if (ret) >> + if (ret) { >> + put_device(&hdev->cls_dev); >> return ret; >> + } >> >> __module_get(THIS_MODULE); > ^^^^^^^^ I'm curious why? I don't see why you need this. hnae_ae_register() is called from hns_dsaf.ko(hns_dsaf_probe()), the refcount of module hnae is already be got while loading hns_dsaf.ko in resolve_symbol(), so I think this can be removed. Thanks, Yang > > The change itself is ok. > > Thanks, > Reviewed-by: Leon Romanovsky <leonro@nvidia.com> > .
On 2022/10/20 8:28, Jakub Kicinski wrote: > On Tue, 18 Oct 2022 15:58:38 +0300 Leon Romanovsky wrote: >> The change itself is ok. > Also the .release function is empty which is another bad smell? The upper device (struct dsaf_device *dsaf_dev) is allocated by devm_kzalloc(), so it's no need to free it in ->release(). Thanks, Yang > >> Reviewed-by: Leon Romanovsky <leonro@nvidia.com> > Thanks! > .
On Thu, 20 Oct 2022 15:48:38 +0800 Yang Yingliang wrote: > On 2022/10/20 8:28, Jakub Kicinski wrote: > > On Tue, 18 Oct 2022 15:58:38 +0300 Leon Romanovsky wrote: > >> The change itself is ok. > > > > Also the .release function is empty which is another bad smell? > > The upper device (struct dsaf_device *dsaf_dev) is allocated by > devm_kzalloc(), so it's no need to free it in ->release(). Nah ah. devm_* is just for objects which tie their lifetime naturally to the lifetime of the driver instance, IOW the device ->priv. struct device allocated by the driver is not tied to that, it's a properly referenced object. I don't think that just because the driver that allocated it got ->remove()d you're safe to free allocated struct devices.
On 2022/10/21 3:43, Jakub Kicinski wrote: > On Thu, 20 Oct 2022 15:48:38 +0800 Yang Yingliang wrote: >> On 2022/10/20 8:28, Jakub Kicinski wrote: >>> On Tue, 18 Oct 2022 15:58:38 +0300 Leon Romanovsky wrote: >>>> The change itself is ok. >>> Also the .release function is empty which is another bad smell? >> The upper device (struct dsaf_device *dsaf_dev) is allocated by >> devm_kzalloc(), so it's no need to free it in ->release(). > Nah ah. devm_* is just for objects which tie their lifetime naturally > to the lifetime of the driver instance, IOW the device ->priv. > > struct device allocated by the driver is not tied to that, it's > a properly referenced object. I don't think that just because > the driver that allocated it got ->remove()d you're safe to free > allocated struct devices. In this driver, I see the 'cls_dev' is used as driver data and it unregistered before got removed to free the device memory, I think it's safe for now. Thanks, Yang > .
On Thu, Oct 20, 2022 at 03:45:17PM +0800, Yang Yingliang wrote: > > On 2022/10/18 20:58, Leon Romanovsky wrote: > > On Tue, Oct 18, 2022 at 08:24:51PM +0800, Yang Yingliang wrote: > > > Inject fault while probing module, if device_register() fails, > > > but the refcount of kobject is not decreased to 0, the name > > > allocated in dev_set_name() is leaked. Fix this by calling > > > put_device(), so that name can be freed in callback function > > > kobject_cleanup(). > > > > > > unreferenced object 0xffff00c01aba2100 (size 128): > > > comm "systemd-udevd", pid 1259, jiffies 4294903284 (age 294.152s) > > > hex dump (first 32 bytes): > > > 68 6e 61 65 30 00 00 00 18 21 ba 1a c0 00 ff ff hnae0....!...... > > > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > > > backtrace: > > > [<0000000034783f26>] slab_post_alloc_hook+0xa0/0x3e0 > > > [<00000000748188f2>] __kmem_cache_alloc_node+0x164/0x2b0 > > > [<00000000ab0743e8>] __kmalloc_node_track_caller+0x6c/0x390 > > > [<000000006c0ffb13>] kvasprintf+0x8c/0x118 > > > [<00000000fa27bfe1>] kvasprintf_const+0x60/0xc8 > > > [<0000000083e10ed7>] kobject_set_name_vargs+0x3c/0xc0 > > > [<000000000b87affc>] dev_set_name+0x7c/0xa0 > > > [<000000003fd8fe26>] hnae_ae_register+0xcc/0x190 [hnae] > > > [<00000000fe97edc9>] hns_dsaf_ae_init+0x9c/0x108 [hns_dsaf] > > > [<00000000c36ff1eb>] hns_dsaf_probe+0x548/0x748 [hns_dsaf] > > > > > > Fixes: 6fe6611ff275 ("net: add Hisilicon Network Subsystem hnae framework support") > > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > > > --- > > > drivers/net/ethernet/hisilicon/hns/hnae.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/ethernet/hisilicon/hns/hnae.c b/drivers/net/ethernet/hisilicon/hns/hnae.c > > > index 00fafc0f8512..430eccea8e5e 100644 > > > --- a/drivers/net/ethernet/hisilicon/hns/hnae.c > > > +++ b/drivers/net/ethernet/hisilicon/hns/hnae.c > > > @@ -419,8 +419,10 @@ int hnae_ae_register(struct hnae_ae_dev *hdev, struct module *owner) > > > hdev->cls_dev.release = hnae_release; > > > (void)dev_set_name(&hdev->cls_dev, "hnae%d", hdev->id); > > ^^^^^^^^^^^^ this is unexpected in netdev code. > Did you mean the 'void' can be removed ? I mean that ethernet names are provided by netdev code and I don't expect to see any dev_set_name() call in drivers/net/ethernet/* folders. > > > > > ret = device_register(&hdev->cls_dev); > > > - if (ret) > > > + if (ret) { > > > + put_device(&hdev->cls_dev); > > > return ret; > > > + } > > > __module_get(THIS_MODULE); > > ^^^^^^^^ I'm curious why? I don't see why you need this. > hnae_ae_register() is called from hns_dsaf.ko(hns_dsaf_probe()), the > refcount of module hnae is already be got while loading hns_dsaf.ko > in resolve_symbol(), so I think this can be removed. Yes. > > Thanks, > Yang > > > > The change itself is ok. > > > > Thanks, > > Reviewed-by: Leon Romanovsky <leonro@nvidia.com> > > .
On Fri, Oct 21, 2022 at 11:01:47AM +0800, Yang Yingliang wrote: > > On 2022/10/21 3:43, Jakub Kicinski wrote: > > On Thu, 20 Oct 2022 15:48:38 +0800 Yang Yingliang wrote: > > > On 2022/10/20 8:28, Jakub Kicinski wrote: > > > > On Tue, 18 Oct 2022 15:58:38 +0300 Leon Romanovsky wrote: > > > > > The change itself is ok. > > > > Also the .release function is empty which is another bad smell? > > > The upper device (struct dsaf_device *dsaf_dev) is allocated by > > > devm_kzalloc(), so it's no need to free it in ->release(). > > Nah ah. devm_* is just for objects which tie their lifetime naturally > > to the lifetime of the driver instance, IOW the device ->priv. > > > > struct device allocated by the driver is not tied to that, it's > > a properly referenced object. I don't think that just because > > the driver that allocated it got ->remove()d you're safe to free > > allocated struct devices. > In this driver, I see the 'cls_dev' is used as driver data and it > unregistered > before got removed to free the device memory, I think it's safe for now. Empty release means reference counting doesn't really count anything. According to your reply, cls_dev is protected from outside and its life time bounded to upper level. The thing is that you was expected to create that cls_dev when you did device_initalization and release it with not-empty release function. Thanks > > Thanks, > Yang > > .
diff --git a/drivers/net/ethernet/hisilicon/hns/hnae.c b/drivers/net/ethernet/hisilicon/hns/hnae.c index 00fafc0f8512..430eccea8e5e 100644 --- a/drivers/net/ethernet/hisilicon/hns/hnae.c +++ b/drivers/net/ethernet/hisilicon/hns/hnae.c @@ -419,8 +419,10 @@ int hnae_ae_register(struct hnae_ae_dev *hdev, struct module *owner) hdev->cls_dev.release = hnae_release; (void)dev_set_name(&hdev->cls_dev, "hnae%d", hdev->id); ret = device_register(&hdev->cls_dev); - if (ret) + if (ret) { + put_device(&hdev->cls_dev); return ret; + } __module_get(THIS_MODULE);
Inject fault while probing module, if device_register() fails, but the refcount of kobject is not decreased to 0, the name allocated in dev_set_name() is leaked. Fix this by calling put_device(), so that name can be freed in callback function kobject_cleanup(). unreferenced object 0xffff00c01aba2100 (size 128): comm "systemd-udevd", pid 1259, jiffies 4294903284 (age 294.152s) hex dump (first 32 bytes): 68 6e 61 65 30 00 00 00 18 21 ba 1a c0 00 ff ff hnae0....!...... 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [<0000000034783f26>] slab_post_alloc_hook+0xa0/0x3e0 [<00000000748188f2>] __kmem_cache_alloc_node+0x164/0x2b0 [<00000000ab0743e8>] __kmalloc_node_track_caller+0x6c/0x390 [<000000006c0ffb13>] kvasprintf+0x8c/0x118 [<00000000fa27bfe1>] kvasprintf_const+0x60/0xc8 [<0000000083e10ed7>] kobject_set_name_vargs+0x3c/0xc0 [<000000000b87affc>] dev_set_name+0x7c/0xa0 [<000000003fd8fe26>] hnae_ae_register+0xcc/0x190 [hnae] [<00000000fe97edc9>] hns_dsaf_ae_init+0x9c/0x108 [hns_dsaf] [<00000000c36ff1eb>] hns_dsaf_probe+0x548/0x748 [hns_dsaf] Fixes: 6fe6611ff275 ("net: add Hisilicon Network Subsystem hnae framework support") Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- drivers/net/ethernet/hisilicon/hns/hnae.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)