| Message ID | 20250123004520.806855-5-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | eth: fix calling napi_enable() in atomic context | expand |
Jakub Kicinski <kuba@kernel.org> : > napi_enable() may sleep now, take netdev_lock() before tp->lock and > tp->rx_lock. [...] > diff --git a/drivers/net/ethernet/realtek/8139too.c b/drivers/net/ethernet/realtek/8139too.c > index 9ce0e8a64ba8..a73dcaffa8c5 100644 > --- a/drivers/net/ethernet/realtek/8139too.c > +++ b/drivers/net/ethernet/realtek/8139too.c > @@ -1684,6 +1684,7 @@ static void rtl8139_tx_timeout_task (struct work_struct *work) > if (tmp8 & CmdTxEnb) > RTL_W8 (ChipCmd, CmdRxEnb); > > + netdev_lock(dev); > spin_lock_bh(&tp->rx_lock); > /* Disable interrupts by clearing the interrupt mask. */ > RTL_W16 (IntrMask, 0x0000); > @@ -1694,11 +1695,12 @@ static void rtl8139_tx_timeout_task (struct work_struct *work) > spin_unlock_irq(&tp->lock); > > /* ...and finally, reset everything */ > - napi_enable(&tp->napi); > + napi_enable_locked(&tp->napi); > rtl8139_hw_start(dev); > netif_wake_queue(dev); > > spin_unlock_bh(&tp->rx_lock); > + netdev_unlock(dev); > } I wonder why the old-style napi_enable could not be moved right before spin_lock_bh(&tp->rx_lock) above. /me checks... The napi poll handler of the driver only does Rx processing. Tx completion is performed in the IRQ handler [*]. rtl8139_tx_timeout_task is only triggered after Tx timeout. The bh locked section above provides exclusion against the whole content of the napi poll handler If a request for the napi poll handler is instantly racing with the timeout task right before spin_lock_bh(&tp->rx_lock) ... CmdTxEnb may be enabled early in rtl8139_rx_err. :o( RTL_W8(ChipCmd, CmdRxEnb) above is PCI posted and thus already racy wrt CmdTxEnb but simply moving napi_enable can't be formally claimed to be safe. Too bad. Acked-by: Francois Romieu <romieu@fr.zoreil.com> [*] Not that uncommon pre subprime crisis napi design style btw.
diff --git a/drivers/net/ethernet/realtek/8139too.c b/drivers/net/ethernet/realtek/8139too.c index 9ce0e8a64ba8..a73dcaffa8c5 100644 --- a/drivers/net/ethernet/realtek/8139too.c +++ b/drivers/net/ethernet/realtek/8139too.c @@ -1684,6 +1684,7 @@ static void rtl8139_tx_timeout_task (struct work_struct *work) if (tmp8 & CmdTxEnb) RTL_W8 (ChipCmd, CmdRxEnb); + netdev_lock(dev); spin_lock_bh(&tp->rx_lock); /* Disable interrupts by clearing the interrupt mask. */ RTL_W16 (IntrMask, 0x0000); @@ -1694,11 +1695,12 @@ static void rtl8139_tx_timeout_task (struct work_struct *work) spin_unlock_irq(&tp->lock); /* ...and finally, reset everything */ - napi_enable(&tp->napi); + napi_enable_locked(&tp->napi); rtl8139_hw_start(dev); netif_wake_queue(dev); spin_unlock_bh(&tp->rx_lock); + netdev_unlock(dev); } static void rtl8139_tx_timeout(struct net_device *dev, unsigned int txqueue)
napi_enable() may sleep now, take netdev_lock() before tp->lock and tp->rx_lock. Fixes: 413f0271f396 ("net: protect NAPI enablement with netdev_lock()") Reported-by: Dan Carpenter <dan.carpenter@linaro.org> Link: https://lore.kernel.org/dcfd56bc-de32-4b11-9e19-d8bd1543745d@stanley.mountain Reviewed-by: Eric Dumazet <edumazet@google.com> Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- CC: romieu@fr.zoreil.com CC: kuniyu@amazon.com --- drivers/net/ethernet/realtek/8139too.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)