Re: [PATCH net v2 1/3] net: fealnx: fix teardown order in remove

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 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).

- [High] The patch makes fealnx_remove_one() free both DMA rings, unmap
  np->mem and free the netdev right after unregister_netdev().
- [Medium] The patch's safety argument is that dev_close() stops the
  Tx/Rx engines before fealnx_remove_one() frees the descriptor rings.

--

net: fealnx: fix teardown order in remove

This commit moves unregister_netdev() ahead of the dma_free_coherent()
calls in fealnx_remove_one(). The goal is for dev_close() to stop the
Tx/Rx engines, delete the timers and free the IRQ before the DMA rings
are freed.

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

[Severity: High]

Does netdev_close() in drivers/net/ethernet/fealnx.c really guarantee
what the commit message relies on? It runs this sequence without taking
np->lock:

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

intr_handler() is registered with IRQF_SHARED. It can already be running
on another CPU while holding np->lock. Its loop checks the software
np->imrvalue, so the IMR=0 write above does not stop it.

Suppose the handler reaches the too-much-work path after
timer_delete_sync(&np->reset_timer) has returned:

intr_handler() {
    ...
		if (--boguscnt < 0) {
			...
			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 it does not cancel the
timer the handler armed. Can reset_timer still be pending when
unregister_netdev() returns?

If it can, fealnx_remove_one() then frees the rings, unmaps np->mem and
calls free_netdev(). That frees the timer_list embedded in np while it
is still queued.

When reset_timer() later fires, it takes np->lock in freed memory and
calls enable_rxtx(). enable_rxtx() touches rx_ring through
reset_rx_descriptors(). It then writes the freed tx_ring_dma and
rx_ring_dma into TXLBA/RXLBA through the unmapped np->mem.

Three other paths write np->crvalue, with RXEN/TXEN still set, back to
TCRRCR after close's stop_nic_rxtx():

- the RBU branch in intr_handler()
- the rx error branch in netdev_rx()
- the media change branch in netdev_timer(), which can run before
  timer_delete_sync(&np->timer)

The handler also restores IMR on exit:

	iowrite32(np->imrvalue, ioaddr + IMR);

Could any of these turn the engines and interrupts back on before the
rings are freed? Remove does no BCR soft reset and does not clear bus
mastering before dma_free_coherent(), so a re-enabled chip could DMA
into the freed ring pages.

Later patches in the series leave netdev_close() unchanged. They add
pci_disable_device() only after the rings are freed.

Would netdev_close() need to clear the enable bits under np->lock, and
sync the IRQ before a final timer_shutdown_sync()? Would the chip also
need a reset before the rings are freed?

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

[Severity: Medium]

Are the engines guaranteed to have stopped before these rings are freed?
The only hardware quiesce step in netdev_close() is stop_nic_rxtx(), and
it does not report failure:

drivers/net/ethernet/fealnx.c:stop_nic_rxtx() {
	int delay = 0x1000;
	iowrite32(crvalue & ~(CR_W_RXEN+CR_W_TXEN), ioaddr + TCRRCR);
	while (--delay) {
		if ( (ioread32(ioaddr + TCRRCR) & (CR_R_RXSTOP+CR_R_TXSTOP))
					    == (CR_R_RXSTOP+CR_R_TXSTOP) )
			break;
	}
}

The poll has no delay between reads. To netdev_close(), running out of
the budget looks the same as a successful stop.

A Tx frame might still be in collision backoff on a 10 Mb/s half-duplex
link, or the chip might be wedged, for example after tx_timeout. In
either case, could the chip still write descriptor status into tx_ring
or rx_ring after dma_free_coherent()?

The driver already has reset_and_disable_rxtx(), which does the stop,
IMR=0 and a BCR soft reset. Neither close nor remove uses it, and bus
mastering is not cleared before the rings are freed.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002140954.261779-1-r.zhambakiev%40prosoftsystems.ru