diff mbox series

[net,v2] net: qcom/emac: Fix use after free bug in emac_remove due to race condition

Message ID 20230318080526.785457-1-zyytlz.wz@163.com
State New
Headers
Series [net,v2] net: qcom/emac: Fix use after free bug in emac_remove due to race condition |

Commit Message

Zheng Wang March 18, 2023, 8:05 a.m. UTC 
  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 <zyytlz.wz@163.com>
---
v2:
- cancel the work after unregister_netdev suggested by Jakub
---
 drivers/net/ethernet/qualcomm/emac/emac.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

patchwork-bot+netdevbpf@kernel.org March 20, 2023, 9:20 a.m. UTC | #1 
Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

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!
Jakub Kicinski March 20, 2023, 7:22 p.m. UTC | #2 
On Mon, 20 Mar 2023 09:20:17 +0000 patchwork-bot+netdevbpf@kernel.org
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.
Zheng Hacker March 23, 2023, 3:49 a.m. UTC | #3 
Jakub Kicinski <kuba@kernel.org> 于2023年3月21日周二 03:23写道：
>
> On Mon, 20 Mar 2023 09:20:17 +0000 patchwork-bot+netdevbpf@kernel.org
> 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
diff mbox series

Patch

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