In emac_probe, &adpt->work_thread is bound with
emac_work_thread. Then it will be started by timeout
handler emac_tx_timeout or a IRQ handler emac_isr.
If we remove the driver which will call emac_remove
to make cleanup, there may be a unfinished work.
The possible sequence is as follows:
Fix it by finishing the work before cleanup in the emac_remove
and disable timeout response.
CPU0 CPU1
|emac_work_thread
emac_remove |
free_netdev |
kfree(netdev); |
|emac_reinit_locked
|emac_mac_down
|//use netdev
Fixes: b9b17debc69d ("net: emac: emac gigabit ethernet controller driver")
Signed-off-by: Zheng Wang <[email protected]>
---
v2:
- cancel the work after unregister_netdev suggested by Jakub
---
drivers/net/ethernet/qualcomm/emac/emac.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c b/drivers/net/ethernet/qualcomm/emac/emac.c
index 3115b2c12898..eaa50050aa0b 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac.c
@@ -724,9 +724,15 @@ static int emac_remove(struct platform_device *pdev)
struct net_device *netdev = dev_get_drvdata(&pdev->dev);
struct emac_adapter *adpt = netdev_priv(netdev);
+ netif_carrier_off(netdev);
+ netif_tx_disable(netdev);
+
unregister_netdev(netdev);
netif_napi_del(&adpt->rx_q.napi);
+ free_irq(adpt->irq.irq, &adpt->irq);
+ cancel_work_sync(&adpt->work_thread);
+
emac_clks_teardown(adpt);
put_device(&adpt->phydev->mdio.dev);
--
2.25.1
Hello:
This patch was applied to netdev/net.git (main)
by David S. Miller <[email protected]>:
On Sat, 18 Mar 2023 16:05:26 +0800 you wrote:
> In emac_probe, &adpt->work_thread is bound with
> emac_work_thread. Then it will be started by timeout
> handler emac_tx_timeout or a IRQ handler emac_isr.
>
> If we remove the driver which will call emac_remove
> to make cleanup, there may be a unfinished work.
>
> [...]
Here is the summary with links:
- [net,v2] net: qcom/emac: Fix use after free bug in emac_remove due to race condition
https://git.kernel.org/netdev/net/c/6b6bc5b8bd2d
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
On Mon, 20 Mar 2023 09:20:17 +0000 [email protected]
wrote:
> Here is the summary with links:
> - [net,v2] net: qcom/emac: Fix use after free bug in emac_remove due to race condition
> https://git.kernel.org/netdev/net/c/6b6bc5b8bd2d
Don't think this is correct FWIW, randomly shutting things down without
holding any locks and before unregister_netdev() is called has got to
be racy. Oh, eh.
Jakub Kicinski <[email protected]> 于2023年3月21日周二 03:23写道：
>
> On Mon, 20 Mar 2023 09:20:17 +0000 [email protected]
> wrote:
> > Here is the summary with links:
> > - [net,v2] net: qcom/emac: Fix use after free bug in emac_remove due to race condition
> > https://git.kernel.org/netdev/net/c/6b6bc5b8bd2d
>
> Don't think this is correct FWIW, randomly shutting things down without
> holding any locks and before unregister_netdev() is called has got to
> be racy. Oh, eh.
Dear Jakubju,
Sorry for my late reply. I had a busy week.
I have taken a look at similar fixes implemented in other drivers, but
I do think your advice is more precious for I'm not familiar with the
driver.
Based on your experience and expertise, what do you think would be the
most effective solution to address the race condition issue that you
have identified in the emac_remove function of the qcom/emac driver? I
appreciate any insights or suggestions that you might have on this
matter.
Thank you for your time and help.
Best regards,
Zheng Wang