| Message ID | 20240522021317.1113689-1-xiaolei.wang@windriver.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [net] net: fec: free fec queue when fec_enet_mii_init() fails | expand |
> -----Original Message----- > From: Xiaolei Wang <xiaolei.wang@windriver.com> > Sent: 2024年5月22日 10:13 > To: Wei Fang <wei.fang@nxp.com>; Shenwei Wang > <shenwei.wang@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com > Cc: imx@lists.linux.dev; netdev@vger.kernel.org; > linux-kernel@vger.kernel.org > Subject: [net PATCH] net: fec: free fec queue when fec_enet_mii_init() fails > > commit 63e3cc2b87c2 ("arm64: dts: imx93-11x11-evk: add > reset gpios for ethernet PHYs") the rese-gpios attribute nit: reset-gpios > is added, but this pcal6524 is loaded later, which causes > fec driver defer, the following memory leak occurs. > > unreferenced object 0xffffff8010350000 (size 8192): > comm "kworker/u8:3", pid 39, jiffies 4294893562 > hex dump (first 32 bytes): > 02 00 00 00 00 00 00 00 00 50 06 8a c0 ff ff ff .........P...... > e0 6f 06 8a c0 ff ff ff 00 50 06 8a c0 ff ff ff .o.......P...... > backtrace (crc f1b8b79f): > [<0000000057d2c6ae>] kmemleak_alloc+0x34/0x40 > [<000000003c413e60>] kmalloc_trace+0x2f8/0x460 > [<00000000663f64e6>] fec_probe+0x1364/0x3a04 > [<0000000024d7e427>] platform_probe+0xc4/0x198 > [<00000000293aa124>] really_probe+0x17c/0x4f0 > [<00000000dfd1e0f3>] __driver_probe_device+0x158/0x2c4 > [<000000004ae0034a>] driver_probe_device+0x60/0x18c > [<00000000fa3ad0e1>] __device_attach_driver+0x168/0x208 > [<00000000394a38d3>] bus_for_each_drv+0x104/0x190 > [<00000000c44e3dea>] __device_attach+0x1f8/0x33c > [<000000004db69c14>] device_initial_probe+0x14/0x20 > [<00000000f4705309>] bus_probe_device+0x128/0x158 > [<00000000f7115919>] deferred_probe_work_func+0x12c/0x1d8 > [<0000000012315b3b>] process_scheduled_works+0x6c0/0x164c > [<0000000089b2b6e1>] worker_thread+0x370/0x95c > [<000000004dbe3d1a>] kthread+0x360/0x420 > > Fixes: 63e3cc2b87c2 ("arm64: dts: imx93-11x11-evk: add reset gpios for > ethernet PHYs") I don't think this commit should be blamed, because this issue should have existed earlier, it is just pcal6524 driver is loaded later than the fec driver, which triggers this issue. The commit 59d0f7465644 ("net: fec: init multi queue date structure") was the first to introduce this issue, commit 619fee9eb13b ("net: fec: fix the potential memory leak in fec_enet_init() ") fixed this, but it does not seem to be completely fixed. We should find the correct blamed commit so that stable maintainers can backport to the correct kernel version. > Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com> > --- > drivers/net/ethernet/freescale/fec_main.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c > b/drivers/net/ethernet/freescale/fec_main.c > index a72d8a2eb0b3..2b3534d434d8 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -4524,6 +4524,7 @@ fec_probe(struct platform_device *pdev) > fec_enet_mii_remove(fep); > failed_mii_init: > failed_irq: > + fec_enet_free_queue(ndev); > failed_init: > fec_ptp_stop(pdev); > failed_reset: > -- > 2.25.1
> The commit 59d0f7465644 ("net: fec: init multi queue date structure") > was the first to introduce this issue, commit 619fee9eb13b > ("net: fec: fix the potential memory leak in fec_enet_init() ") > fixed this, but it does not seem to be completely fixed. This fix is also not great, and i would say the initial design is really the problem. There needs to be a function which is the opposite of fec_enet_init(). It can then be called in the probe cleanup code, and in fec_drv_remove() which also appears to leak the queues. Andrew
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: 2024年5月22日 11:15 > To: Wei Fang <wei.fang@nxp.com> > Cc: Xiaolei Wang <xiaolei.wang@windriver.com>; Shenwei Wang > <shenwei.wang@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com; imx@lists.linux.dev; netdev@vger.kernel.org; > linux-kernel@vger.kernel.org > Subject: Re: [net PATCH] net: fec: free fec queue when fec_enet_mii_init() fails > > > The commit 59d0f7465644 ("net: fec: init multi queue date structure") > > was the first to introduce this issue, commit 619fee9eb13b > > ("net: fec: fix the potential memory leak in fec_enet_init() ") fixed > > this, but it does not seem to be completely fixed. > > This fix is also not great, and i would say the initial design is really the problem. > There needs to be a function which is the opposite of fec_enet_init(). It can > then be called in the probe cleanup code, and in fec_drv_remove() which also > appears to leak the queues. > Yes, this issue also exists when the fec driver is unbound, maybe Xiaolei can help improve it in his patch.
On 5/22/24 6:28 PM, Wei Fang wrote: > CAUTION: This email comes from a non Wind River email account! > Do not click links or open attachments unless you recognize the sender and know the content is safe. > >> -----Original Message----- >> From: Andrew Lunn <andrew@lunn.ch> >> Sent: 2024年5月22日 11:15 >> To: Wei Fang <wei.fang@nxp.com> >> Cc: Xiaolei Wang <xiaolei.wang@windriver.com>; Shenwei Wang >> <shenwei.wang@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; >> davem@davemloft.net; edumazet@google.com; kuba@kernel.org; >> pabeni@redhat.com; imx@lists.linux.dev; netdev@vger.kernel.org; >> linux-kernel@vger.kernel.org >> Subject: Re: [net PATCH] net: fec: free fec queue when fec_enet_mii_init() fails >> >>> The commit 59d0f7465644 ("net: fec: init multi queue date structure") >>> was the first to introduce this issue, commit 619fee9eb13b >>> ("net: fec: fix the potential memory leak in fec_enet_init() ") fixed >>> this, but it does not seem to be completely fixed. >> This fix is also not great, and i would say the initial design is really the problem. >> There needs to be a function which is the opposite of fec_enet_init(). It can >> then be called in the probe cleanup code, and in fec_drv_remove() which also >> appears to leak the queues. >> > Yes, this issue also exists when the fec driver is unbound, maybe Xiaolei can > help improve it in his patch. I will add fec_enet_free_queue() to fec_drv_remove in v2 version thanks xiaolei
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index a72d8a2eb0b3..2b3534d434d8 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -4524,6 +4524,7 @@ fec_probe(struct platform_device *pdev) fec_enet_mii_remove(fep); failed_mii_init: failed_irq: + fec_enet_free_queue(ndev); failed_init: fec_ptp_stop(pdev); failed_reset:
commit 63e3cc2b87c2 ("arm64: dts: imx93-11x11-evk: add reset gpios for ethernet PHYs") the rese-gpios attribute is added, but this pcal6524 is loaded later, which causes fec driver defer, the following memory leak occurs. unreferenced object 0xffffff8010350000 (size 8192): comm "kworker/u8:3", pid 39, jiffies 4294893562 hex dump (first 32 bytes): 02 00 00 00 00 00 00 00 00 50 06 8a c0 ff ff ff .........P...... e0 6f 06 8a c0 ff ff ff 00 50 06 8a c0 ff ff ff .o.......P...... backtrace (crc f1b8b79f): [<0000000057d2c6ae>] kmemleak_alloc+0x34/0x40 [<000000003c413e60>] kmalloc_trace+0x2f8/0x460 [<00000000663f64e6>] fec_probe+0x1364/0x3a04 [<0000000024d7e427>] platform_probe+0xc4/0x198 [<00000000293aa124>] really_probe+0x17c/0x4f0 [<00000000dfd1e0f3>] __driver_probe_device+0x158/0x2c4 [<000000004ae0034a>] driver_probe_device+0x60/0x18c [<00000000fa3ad0e1>] __device_attach_driver+0x168/0x208 [<00000000394a38d3>] bus_for_each_drv+0x104/0x190 [<00000000c44e3dea>] __device_attach+0x1f8/0x33c [<000000004db69c14>] device_initial_probe+0x14/0x20 [<00000000f4705309>] bus_probe_device+0x128/0x158 [<00000000f7115919>] deferred_probe_work_func+0x12c/0x1d8 [<0000000012315b3b>] process_scheduled_works+0x6c0/0x164c [<0000000089b2b6e1>] worker_thread+0x370/0x95c [<000000004dbe3d1a>] kthread+0x360/0x420 Fixes: 63e3cc2b87c2 ("arm64: dts: imx93-11x11-evk: add reset gpios for ethernet PHYs") Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com> --- drivers/net/ethernet/freescale/fec_main.c | 1 + 1 file changed, 1 insertion(+)