Re: [PATCH net] net: hns: fix possible memory leak in hnae_ae_register()

[Leon Romanovsky <leon@kernel.org>]

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>
> > .