[PATCH 1/2] can: m_can: m_can_plat_probe(): free can_net device in case probe fails

[PATCH 1/2] can: m_can: m_can_plat_probe(): free can_net device in case probe fails

[Xulin Sun]

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(-)

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

[PATCH 2/2] can: m_can: m_can_class_allocate_dev(): remove impossible error return judgment

[Xulin Sun]

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 <xulin.sun@windriver.com>
---
 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

Re: [PATCH 2/2] can: m_can: m_can_class_allocate_dev(): remove impossible error return judgment

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 856 bytes --]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] can: m_can: m_can_class_allocate_dev(): remove impossible error return judgment

[Xulin Sun]

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
>

Re: [PATCH 2/2] can: m_can: m_can_class_allocate_dev(): remove impossible error return judgment

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1576 bytes --]

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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] can: m_can: m_can_plat_probe(): free can_net device in case probe fails

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2137 bytes --]

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 <xulin.sun@windriver.com>

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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]