* Re: [PATCH net] net: fealnx: Fix potential use-after-free during removal
2026-11-11 12:23 [PATCH net] net: fealnx: Fix potential use-after-free during removal Guangshuo Li
@ 2026-11-11 12:26 ` netdev-bot+sinfo
2026-11-15 12:37 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sinfo @ 2026-11-11 12:26 UTC (permalink / raw)
To: Guangshuo Li
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, linux-kernel
Hi!
This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:
- What hardware the change was tested on. For driver fixes please
mention the device (and if relevant firmware version) used for
testing, or say that the change was not tested on real hardware.
Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.
The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net] net: fealnx: Fix potential use-after-free during removal
2026-11-11 12:23 [PATCH net] net: fealnx: Fix potential use-after-free during removal Guangshuo Li
2026-11-11 12:26 ` netdev-bot+sinfo
@ 2026-11-15 12:37 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sashiko @ 2026-11-15 12:37 UTC (permalink / raw)
To: liguangshuo
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, netdev, linux-kernel
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 0 (1) · Medium: 1 · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] The patch's Fixes: tag points at the wrong commit.
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but netdev_close()
does not stop the interrupt handler and timers before it stops the…
--
net: fealnx: Fix potential use-after-free during removal
This patch moves unregister_netdev() ahead of the dma_free_coherent() calls
in fealnx_remove_one(). With that order, netdev_close() no longer walks
the tx and rx descriptor rings after they have been freed.
> Fixes: 8f1482080104 ("eth: fealnx: bring back this old driver")
[Severity: Medium]
Is this the right commit for the Fixes: tag?
Commit 8f1482080104 is a straight revert of d5e2d038dbec ("eth: fealnx:
delete the driver for Myson MTD-800"), and it only re-added the file.
Before the deletion, fealnx_remove_one() already used the same ordering:
dma_free_coherent(&pdev->dev, TX_TOTAL_SIZE, np->tx_ring,
np->tx_ring_dma);
dma_free_coherent(&pdev->dev, RX_TOTAL_SIZE, np->rx_ring,
np->rx_ring_dma);
unregister_netdev(dev);
git blame at d5e2d038dbec^ attributes the function body and the
unregister_netdev() call to 1da177e4c3f4 ("Linux-2.6.12-rc2"). The
dma_free_coherent() lines come from the 2020 pci_free_consistent
conversion, 405e30e23cb3.
The driver was only deleted for v6.2. That means 6.1.y, 5.15.y and 5.10.y
still carry it, with the same use-after-free on removal. None of those
branches contain 8f1482080104, so wouldn't Fixes-based stable selection
skip them?
Would Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2"), plus
Cc: stable@vger.kernel.org, be more accurate?
> diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c
> index bdc38aac58509..68194c9ef3326 100644
> --- a/drivers/net/ethernet/fealnx.c
> +++ b/drivers/net/ethernet/fealnx.c
> @@ -682,11 +682,11 @@ static void fealnx_remove_one(struct pci_dev *pdev)
> if (dev) {
> struct netdev_private *np = netdev_priv(dev);
>
> + unregister_netdev(dev);
> dma_free_coherent(&pdev->dev, TX_TOTAL_SIZE, np->tx_ring,
> np->tx_ring_dma);
> dma_free_coherent(&pdev->dev, RX_TOTAL_SIZE, np->rx_ring,
> np->rx_ring_dma);
> - unregister_netdev(dev);
> pci_iounmap(pdev, np->mem);
> free_netdev(dev);
[Severity: High]
This is a pre-existing issue and was not introduced by this patch. Can
removal still end in a use-after-free when netdev_close() races with
intr_handler()?
netdev_close() never takes np->lock. It also calls free_irq() only after
it has stopped the chip and deleted the timers:
drivers/net/ethernet/fealnx.c:netdev_close() {
...
iowrite32(0x0000, ioaddr + IMR);
stop_nic_rxtx(ioaddr, 0);
timer_delete_sync(&np->timer);
timer_delete_sync(&np->reset_timer);
free_irq(np->pci_dev->irq, dev);
...
}
The IRQ is requested with IRQF_SHARED, so intr_handler() can run on
another CPU during this whole sequence. If the handler runs out of
boguscnt after timer_delete_sync(&np->reset_timer), it re-arms the timer:
drivers/net/ethernet/fealnx.c:intr_handler() {
...
if (!np->reset_timer_armed) {
np->reset_timer_armed = 1;
np->reset_timer.expires = RUN_AT(HZ/2);
add_timer(&np->reset_timer);
...
}
free_irq() waits for the handler to finish, but netdev_close() then
returns with reset_timer still pending. fealnx_remove_one() goes on to
call pci_iounmap() and free_netdev(), which frees netdev_private and the
timer_list embedded in it.
About HZ/2 later, reset_timer() would take np->lock in freed memory and
do MMIO through the unmapped np->mem.
The chip can also be restarted after the stop_nic_rxtx() call in close:
- The RBU branch in intr_handler() and the netdev_rx() error path write
np->crvalue back to TCRRCR. netdev_close() never clears
CR_W_RXEN/CR_W_TXEN from np->crvalue. intr_handler() also writes
np->imrvalue back to IMR on exit.
- If reset_timer() fires between stop_nic_rxtx() and
timer_delete_sync(&np->reset_timer), enable_rxtx() rewrites the ring
base addresses, turns RX/TX back on through __set_rx_mode() and
unmasks IMR.
- On a link-up media change, netdev_timer() can write crvalue, with the
enable bits set, back to TCRRCR before
timer_delete_sync(&np->timer) runs.
In addition, stop_nic_rxtx() gives up silently after 0x1000 polls. The
driver never does a BCR soft reset and never calls pci_disable_device()
or pci_clear_master().
With this patch, the coherent rings are freed right after
unregister_netdev() returns. Could a chip that is still bus mastering
then DMA into the freed rings and RX skbs?
One possible order:
- call free_irq() first, or mask IMR under np->lock and then call
synchronize_irq()
- delete the timers so that reset_timer cannot be re-armed
- do the final hardware stop
- confirm the stop and reset the chip before the rings are released
Would that close these windows?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-e5624093d6-net-fealnx%40mail.sdu.edu.cn
^ permalink raw reply [flat|nested] 3+ messages in thread