| Message ID | 20210205072559.13241-1-xulin.sun@windriver.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] can: m_can: m_can_plat_probe(): free can_net device in case probe fails | expand |
On 05.02.2021 15:25:59, Xulin Sun wrote: > If the previous can_net device has been successfully allocated, its > private data structure is impossible to be empty, remove this redundant > error return judgment. Otherwise, memory leaks for alloc_candev() will > be triggered. Your analysis is correct, the netdev_priv() will never fail. But how will this trigger a mem leak on alloc_candev()? I've removed that statement. I'll add it back, if I've missed something. > Signed-off-by: Xulin Sun <xulin.sun@windriver.com> Applied to linux-can-next/testing. Thanks, Marc
On 2021/2/5 下午4:19, Marc Kleine-Budde wrote: > On 05.02.2021 15:25:59, Xulin Sun wrote: >> If the previous can_net device has been successfully allocated, its >> private data structure is impossible to be empty, remove this redundant >> error return judgment. Otherwise, memory leaks for alloc_candev() will >> be triggered. > Your analysis is correct, the netdev_priv() will never fail. But how > will this trigger a mem leak on alloc_candev()? I've removed that Hi Marc, The previous code judges the netdev_priv is empty, and then goto out. The correct approach should add free_candev(net_dev) before goto. The code Like: class_dev = netdev_priv(net_dev); if (!class_dev) { dev_err(dev, "Failed to init netdev cdevate"); + free_candev(net_dev); goto out; } Otherwise, memory leaks for alloc_candev() will be triggered. Now directly remove the impossible error return judgment to resolve the above possible issue. Thanks Xulin > statement. I'll add it back, if I've missed something. > >> Signed-off-by: Xulin Sun <xulin.sun@windriver.com> > Applied to linux-can-next/testing. > > Thanks, > Marc >
diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c index 38ea5e600fb8..0a2655c94018 100644 --- a/drivers/net/can/m_can/m_can_platform.c +++ b/drivers/net/can/m_can/m_can_platform.c @@ -67,8 +67,10 @@ static int m_can_plat_probe(struct platform_device *pdev) return -ENOMEM; priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); - if (!priv) - return -ENOMEM; + if (!priv) { + ret = -ENOMEM; + goto failed_ret; + } mcan_class->device_data = priv; @@ -113,7 +115,11 @@ static int m_can_plat_probe(struct platform_device *pdev) ret = m_can_class_register(mcan_class); + return ret; + failed_ret: + free_candev(mcan_class->net); + return ret; }
The can_net device is allocated through kvzalloc(), if the subsequent probe cases fail to initialize, it should free the can_net device that has been successfully allocated before. To fix below memory leaks call trace: unreferenced object 0xfffffc08418b0000 (size 32768): comm "kworker/0:1", pid 22, jiffies 4294893966 (age 931.976s) hex dump (first 32 bytes): 63 61 6e 25 64 00 00 00 00 00 00 00 00 00 00 00 can%d........... 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [<000000003faec9cc>] __kmalloc+0x1a4/0x3e0 [<00000000560b1cad>] kvmalloc_node+0xa0/0xb0 [<0000000093bada32>] alloc_netdev_mqs+0x60/0x380 [<0000000041ddbb53>] alloc_candev_mqs+0x6c/0x14c [<00000000d08c7529>] m_can_class_allocate_dev+0x64/0x18c [<000000009fef1617>] m_can_plat_probe+0x2c/0x1f4 [<000000006fdcc497>] platform_drv_probe+0x5c/0xb0 [<00000000fd0f0726>] really_probe+0xec/0x41c [<000000003ffa5158>] driver_probe_device+0x60/0xf0 [<000000005986c77e>] __device_attach_driver+0xb0/0x100 [<00000000757823bc>] bus_for_each_drv+0x8c/0xe0 [<0000000059253919>] __device_attach+0xdc/0x180 [<0000000035c2b9f1>] device_initial_probe+0x28/0x34 [<0000000082e2c85c>] bus_probe_device+0xa4/0xb0 [<00000000cc6181c3>] deferred_probe_work_func+0x7c/0xb0 [<0000000001b85f22>] process_one_work+0x1ec/0x480 Signed-off-by: Xulin Sun <xulin.sun@windriver.com> --- drivers/net/can/m_can/m_can_platform.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)