Re: [PATCH net 05/11] net: au1000: serialize TX completion with packet submission

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 (1) · High: 0 · Medium: 2 · 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] au1000_tx() in drivers/net/ethernet/amd/au1000_eth.c now frees
  the skb with hardware IRQs disabled.
- [Medium] After this patch, au1000_tx()'s inline reclaim path leaves
  aup->tx_tail permanently out of step with the hardware.

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but
  au1000_tx() copies skb->len bytes into a fixed MAX_BUF_SIZE…

--

net: au1000: serialize TX completion with packet submission

This commit takes the existing IRQ-safe aup->lock in both au1000_tx_ack()
and au1000_tx(). The goal is to serialize TX descriptor reclamation in hard
IRQ context with packet submission in ndo_start_xmit. It also closes a
missed-wakeup window between netif_stop_queue() and setting tx_full.

> diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
> index a507a524370fd..90887dac26bc9 100644
> --- a/drivers/net/ethernet/amd/au1000_eth.c
> +++ b/drivers/net/ethernet/amd/au1000_eth.c

[ ... ]

> @@ -973,17 +977,20 @@ static netdev_tx_t au1000_tx(struct sk_buff *skb, struct net_device *dev)
>  	u32 buff_stat;
>  	struct db_dest *pDB;
>  	int i;
> +	unsigned long flags;
>  
>  	netif_dbg(aup, tx_queued, dev, "tx: aup %x len=%d, data=%p, head %d\n",
>  				(unsigned)aup, skb->len,
>  				skb->data, aup->tx_head);
>  
> +	spin_lock_irqsave(&aup->lock, flags);
>  	ptxd = aup->tx_dma_ring[aup->tx_head];
>  	buff_stat = ptxd->buff_stat;
>  	if (buff_stat & TX_DMA_ENABLE) {
>  		/* We've wrapped around and the transmitter is still busy */
>  		netif_stop_queue(dev);
>  		aup->tx_full = 1;
> +		spin_unlock_irqrestore(&aup->lock, flags);
>  		return NETDEV_TX_BUSY;
>  	} else if (buff_stat & TX_T_DONE) {
>  		au1000_update_tx_stats(dev, ptxd->status);

[Severity: Medium]

Now that the lock is held here, can this inline reclaim leave aup->tx_tail
permanently out of step with the hardware?

If desc[tx_head] has TX_T_DONE set at this point, the ring is full
(tx_head == tx_tail) and the oldest buffer has just completed. This branch
updates stats and zeroes len. Later in au1000_tx() the code re-arms the
descriptor, which clears the done bit, and advances tx_head:

	ptxd->buff_stat = lower_32_bits(pDB->dma_addr) | TX_DMA_ENABLE;
	...
	aup->tx_head = (aup->tx_head + 1) & (NUM_TX_DMA - 1);

tx_tail is never advanced on this path.

Before this patch, on the uniprocessor Au1x00, the pending MAC DMA
interrupt could run au1000_tx_ack() before the re-arm and move tx_tail
forward. Now the ISR is held off until spin_unlock_irqrestore(). At that
point au1000_tx_ack() finds desc[tx_tail] re-armed with no done bit:

	while (ptxd->buff_stat & TX_T_DONE) {

So it returns without moving tx_tail, which now points at the newest
submitted descriptor.

au1000_init() sets tx_head from the hardware's own buffer index, so the
hardware completes descriptors in index order and that newest descriptor
completes last. Wouldn't that leave the TX_T_DONE bits of older completed
descriptors unacknowledged until then?

It would also delay netif_wake_queue() in au1000_tx_ack() until the whole
4-entry ring drains, rather than after one completion. The offset would
then persist until au1000_init() resets head and tail.

The same thing can happen when au1000_tx() is called with IRQs already
disabled, for example from netpoll.

The MAC DMA interrupt is IRQ_TYPE_LEVEL_HIGH, and au1000_interrupt() always
returns IRQ_RETVAL(1). If unacknowledged done bits keep the line asserted,
could this become an interrupt storm? That part depends on hardware
behaviour that has not been confirmed.

[ ... ]

[Severity: Critical]

This is a pre-existing issue, not one introduced by this patch.
au1000_tx() copies skb->len bytes into a fixed MAX_BUF_SIZE (2048 byte)
bounce buffer without checking the length:

	pDB = aup->tx_db_inuse[aup->tx_head];
	skb_copy_from_linear_data(skb, (void *)pDB->vaddr, skb->len);

Can skb->len be larger than 2048 here? When po->vnet_hdr_sz is nonzero,
tpacket_snd() in net/packet/af_packet.c skips both MTU checks:

	if ((size_max > dev->mtu + reserve + VLAN_HLEN) && !vnet_hdr_sz)
		size_max = dev->mtu + reserve + VLAN_HLEN;
	...
		if (likely(tp_len >= 0) &&
		    tp_len > dev->mtu + reserve &&
		    !vnet_hdr_sz &&
		    !packet_extra_vlan_len_allowed(dev, skb))

An all-zero virtio_net_hdr (GSO_NONE, no NEEDS_CSUM) is accepted without
making the skb GSO. validate_xmit_skb() then only linearizes it, because
au1000 does not advertise NETIF_F_SG, and applies no MTU check.

With CAP_NET_RAW, this path could deliver a multi-kilobyte skb to
au1000_tx():

socket(AF_PACKET, SOCK_RAW)
  setsockopt(PACKET_VNET_HDR)
  setsockopt(PACKET_TX_RING) with tp_frame_size > 2048
  tpacket_snd()
    packet_xmit()
      validate_xmit_skb()
        au1000_tx()
          skb_copy_from_linear_data(..., skb->len)

au1000_probe() allocates a single 16 KiB region with dma_alloc_coherent().
The TX buffers are at offsets 0-8K and the RX buffers at 8-16K. Would an
oversized frame overwrite the neighbouring TX/RX DMA buffers? Beyond about
10 KiB, would it write past the end of the coherent allocation?

Should au1000_tx() drop frames larger than MAX_BUF_SIZE?

> @@ -1012,6 +1019,7 @@ static netdev_tx_t au1000_tx(struct sk_buff *skb, struct net_device *dev)
>  	wmb(); /* drain writebuffer */
>  	dev_kfree_skb(skb);
>  	aup->tx_head = (aup->tx_head + 1) & (NUM_TX_DMA - 1);
> +	spin_unlock_irqrestore(&aup->lock, flags);
>  	return NETDEV_TX_OK;
>  }

[Severity: Medium]

With the new spin_lock_irqsave() in au1000_tx(), dev_kfree_skb(skb) now
runs with hard IRQs disabled. dev_kfree_skb() is just consume_skb():

include/linux/skbuff.h:
#define dev_kfree_skb(a)	consume_skb(a)

include/linux/netdevice.h says:

 * It is not allowed to call kfree_skb() or consume_skb() from hardware
 * interrupt context or with hardware interrupts being disabled.
 * (in_hardirq() || irqs_disabled())

Does this mean every transmitted packet now runs its skb destructors with
IRQs off? That includes sock_wfree()/tcp_wfree(), zerocopy ubuf callbacks,
and dst and conntrack puts.

If any of those reach local_bh_enable(), lockdep_assert_irqs_enabled()
would fire. With CONFIG_TRACE_IRQFLAGS, __local_bh_enable_ip() could also
turn IRQs back on inside the aup->lock section, which would reopen the
race this patch is closing.

The data has already been copied into pDB->vaddr. Could the free move
after spin_unlock_irqrestore(), or use dev_consume_skb_any() instead?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027223140.3774570-1-f.fainelli%40gmail.com