Re: [PATCH] net: ns83820: drain RX tasklet when stopping the device

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (1) · Medium: 0 (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 fix drains only one of the two deferred RX contexts.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but rx_irq(),
  which the drained rx_tasklet runs, unmaps completed RX buffers with…
- [Medium] This problem wasn't introduced by this patch, but
  ns83820_cleanup_rx() frees every posted RX skb without…

--

net: ns83820: drain RX tasklet when stopping the device

This patch adds a tasklet_kill() of rx_tasklet to ns83820_stop(), after
the device is reset and the IRQ is synchronized. The goal is to make sure
an RX tasklet that is already queued cannot run after the stop path
cleans the RX descriptors, or after device removal frees the rings and
the netdev.

> diff --git a/drivers/net/ethernet/natsemi/ns83820.c b/drivers/net/ethernet/natsemi/ns83820.c
> index cdbf82affa7be..f31c15cb2f517 100644
> --- a/drivers/net/ethernet/natsemi/ns83820.c
> +++ b/drivers/net/ethernet/natsemi/ns83820.c
> @@ -1536,6 +1536,7 @@ static int ns83820_stop(struct net_device *ndev)
>  	ns83820_do_reset(dev, CR_RST);
>  
>  	synchronize_irq(dev->pci_dev->irq);
> +	tasklet_kill(&dev->rx_tasklet);

[Severity: High]
Does this drain only one of the two deferred RX contexts? The tasklet
being killed here can itself queue dev->tq_refill. The hard IRQ path can
queue it as well:

rx_action()->ns83820_rx_kick()
ns83820_do_isr()->ns83820_rx_kick()

drivers/net/ethernet/natsemi/ns83820.c:ns83820_rx_kick() {
    ...
	if (dev->rx_info.up && nr_rx_empty(dev) > NR_RX_DESC*3/4)
		schedule_work(&dev->tq_refill);
    ...
}

This happens when the GFP_ATOMIC refill fails under memory pressure.
tasklet_kill() waits for the tasklet but not for work the tasklet has
already queued. The driver has no cancel_work_sync() or flush_work() for
tq_refill anywhere.

queue_refill() calls rx_refill() without checking rx_info.up. Only the
kick_rx() call is gated:

drivers/net/ethernet/natsemi/ns83820.c:queue_refill() {
	struct ns83820 *dev = container_of(work, struct ns83820, tq_refill);
	struct net_device *ndev = dev->ndev;

	rx_refill(ndev, GFP_KERNEL);
	if (dev->rx_info.up)
		kick_rx(ndev);
}

The GFP_KERNEL refill can sleep in reclaim. It then allocates skbs,
DMA-maps them and stores them in rx_info.skbs[] through
ns83820_add_rx_skb(). ns83820_cleanup_rx() frees rx_info.skbs[] without
holding rx_info.lock, and it does not reset next_empty or next_rx.

If the work overlaps with or follows ns83820_cleanup_rx() during ifdown,
could skbs be left in the ring? On the next open, ns83820_setup_rx()
resets next_empty to 0. rx_refill() would then hit this check in
ns83820_add_rx_skb():

	BUG_ON(NULL != dev->rx_info.skbs[next_empty]);

On removal, unregister_netdev()->ns83820_stop() can return while the
work is still pending or running. ns83820_remove_one() then does:

	iounmap(dev->base);
	...
	dma_free_coherent(&dev->pci_dev->dev, 4 * DESC_SIZE * NR_RX_DESC,
			  dev->rx_info.descs, dev->rx_info.phy_descs);
	pci_disable_device(dev->pci_dev);
	free_netdev(ndev);

Wouldn't queue_refill() then use the freed private area, write to the
freed RX descriptor ring, and reach kick_rx() on an unmapped dev->base?

This is the same kind of problem the commit message describes: deferred
RX work queued from the interrupt path, which synchronize_irq() does not
drain. Should ns83820_stop() also call cancel_work_sync(&dev->tq_refill)
after the tasklet is killed and before ns83820_cleanup_rx()?

>  
>  	spin_lock_irq(&dev->misc_lock);
>  	dev->IMR_cache &= ~(ISR_TXURN | ISR_TXIDLE | ISR_TXERR | ISR_TXDESC | ISR_TXOK);

[Severity: Medium]
This is a pre-existing issue, and this patch did not introduce it. The
stop sequence here ends in ns83820_cleanup_rx(). That function frees each
posted RX skb without unmapping it first:

drivers/net/ethernet/natsemi/ns83820.c:ns83820_cleanup_rx() {
    ...
	for (i=0; i<NR_RX_DESC; i++) {
		struct sk_buff *skb = dev->rx_info.skbs[i];
		dev->rx_info.skbs[i] = NULL;
		clear_rx_desc(dev, i);
		kfree_skb(skb);
	}
}

ns83820_add_rx_skb() maps each buffer with
dma_map_single(..., REAL_RX_BUF_SIZE, DMA_FROM_DEVICE). The bus address
is stored only in the descriptor's BUFPTR. clear_rx_desc() overwrites
BUFPTR before anything reads it. The only RX unmap is in rx_irq(), and
it only covers completed descriptors.

Does each ifdown, and the stop during removal, leak up to NR_RX_DESC
streaming DMA mappings? Those are swiotlb slots or IOMMU IOVAs that stay
mapped to freed pages.

[Severity: High]
This is also a pre-existing issue that this patch did not introduce. It
is in rx_irq(), which the drained rx_tasklet runs. Completed RX buffers
are unmapped with a different size than they were mapped with:

drivers/net/ethernet/natsemi/ns83820.c:ns83820_add_rx_skb() {
    ...
	buf = dma_map_single(&dev->pci_dev->dev, skb->data, REAL_RX_BUF_SIZE,
			     DMA_FROM_DEVICE);
    ...
}

drivers/net/ethernet/natsemi/ns83820.c:rx_irq() {
    ...
		dma_unmap_single(&dev->pci_dev->dev, bufptr, RX_BUF_SIZE,
				 DMA_FROM_DEVICE);
    ...
}

with:

#define RX_BUF_SIZE	1500	/* 8192 */
#define REAL_RX_BUF_SIZE (RX_BUF_SIZE + 14)	/* rx/tx mac addr + type */

With swiotlb bouncing, only the unmap size is copied back. On
non-coherent architectures, only 1500 bytes are synced for the CPU.
rx_irq() then calls skb_put() with a len of up to 1514 bytes. It can
also set CHECKSUM_UNNECESSARY when the hardware reports a good checksum.

Can the last 14 bytes of a full-size frame be delivered to the stack
with stale buffer contents, and skip software checksum verification?
DMA_API_DEBUG would also report the size mismatch on every packet.

Should the unmap in rx_irq() use REAL_RX_BUF_SIZE to match the mapping?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018040708.534335-1-yaojiale02%40163.com