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 <[email protected]>
---
drivers/net/can/m_can/m_can_platform.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
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;
}
--
2.17.1
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.
Signed-off-by: Xulin Sun <[email protected]>
---
drivers/net/can/m_can/m_can.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 02c5795b7393..042940088d41 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1797,11 +1797,6 @@ struct m_can_classdev *m_can_class_allocate_dev(struct device *dev)
}
class_dev = netdev_priv(net_dev);
- if (!class_dev) {
- dev_err(dev, "Failed to init netdev cdevate");
- goto out;
- }
-
class_dev->net = net_dev;
class_dev->dev = dev;
SET_NETDEV_DEV(net_dev, dev);
--
2.17.1
On 05.02.2021 15:25:58, Xulin Sun wrote:
> 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 <[email protected]>
This patch doesn't apply to net/master, since v5.10 there is a
similar fix:
85816aba460c can: m_can: Fix freeing of can device from peripherials
Please update to latest v5.10.x. If you're on a kernel that's still
supported, and you're using the latest stable of that kernel, and it
doesn't have that patch applied, ask on linux-stable to pick up that
patch.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
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 <[email protected]>
Applied to linux-can-next/testing.
Thanks,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
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 <[email protected]>
> Applied to linux-can-next/testing.
>
> Thanks,
> Marc
>
On 05.02.2021 16:46:16, Xulin Sun wrote:
> 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
>
> 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.
No - as you said in the original patch description. The return value
of netdev_priv() cannot be NULL, as net_dev is not NULL.
> Now directly remove the impossible error return judgment to resolve
> the above possible issue.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |