Re: [PATCH net 1/2] net: macb: reprogram TBQP after shuffling the TX ring on link-up

[sashiko-bot]

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

Pre-existing issues:
- [Critical] macb_hresp_error_task() reinitializes all TX and RX rings without holding any locks and without synchronizing with NAPI.
- [High] macb_tx_error_task() accesses and modifies TX queue state without holding queue->tx_ptr_lock, causing data races with concurrent macb_start_xmit() and gem_shuffle_tx_rings() operations.
--

commit f087c1d0ffc5f4dd5ef2d3cbe0d1c4868c7a17b2
Author: Christian Taedcke <christian.taedcke@xxxxxxxxxxxxxxx>

net: macb: reprogram TBQP after shuffling the TX ring on link-up

gem_shuffle_tx_one_ring() rotates the software TX ring so that the
tail sits at index 0 and resets queue->tx_tail to 0, but it never
reprograms the hardware transmit buffer queue pointer (TBQP). Other
paths that reset tx_tail to the ring base (macb_init_buffers() and
macb_tx_error_task()) also reprogram TBQP to queue->tx_ring_dma; this
path does not, leaving TBQP pointing at a stale descriptor.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index fd282a1700fb9..b11cb8f068b73 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -820,7 +820,7 @@ static void gem_shuffle_tx_one_ring(struct macb_queue *queue)
>  	if (!count) {
>  		queue->tx_head = 0;
>  		queue->tx_tail = 0;
> -		goto unlock;
> +		goto reset_hw_ptr;
>  	}

[Severity: High]
This is a pre-existing issue, but does macb_tx_error_task() modify TX queue
state without holding queue->tx_ptr_lock?

Looking at macb_tx_error_task(), it resets queue->tx_head and queue->tx_tail
to 0 without holding queue->tx_ptr_lock (it only takes bp->lock):

drivers/net/ethernet/cadence/macb_main.c:macb_tx_error_task() {
    ...
    queue->tx_head = 0;
    queue->tx_tail = 0;
    ...
}

Concurrently, macb_start_xmit() can be executing, which holds
queue->tx_ptr_lock but not bp->lock.

Also, macb_mac_link_up() releases bp->lock before calling
gem_shuffle_tx_rings(), enabling a race.

Although netif_tx_stop_all_queues() is called, it only sets a state flag and
does not wait for a currently executing macb_start_xmit() to finish.

Could this lead to data corruption or double-frees?

[ ... ]

> @@ -869,6 +869,13 @@ static void gem_shuffle_tx_one_ring(struct macb_queue *queue)
>  	/* Make descriptor updates visible to hardware */
>  	wmb();
>  
> +reset_hw_ptr:
> +	/* tx_tail was reset to the ring base, so TBQP must be reprogrammed
> +	 * to match; otherwise it keeps pointing at a stale descriptor. Safe
> +	 * to write directly here as TX is still disabled (called from
> +	 * macb_mac_link_up() before TE is set).
> +	 */
> +	queue_writel(queue, TBQP, lower_32_bits(queue->tx_ring_dma));
>  unlock:
>  	spin_unlock_irqrestore(&queue->tx_ptr_lock, flags);
>  }

[Severity: Critical]
This is a pre-existing issue, but does macb_hresp_error_task() reinitialize
all TX and RX rings without holding any locks or synchronizing with NAPI?

Looking at macb_hresp_error_task(), it calls mog_init_rings() which
destructively resets tx_head, tx_tail, and wipes descriptors across all
queues back to 0:

drivers/net/ethernet/cadence/macb_main.c:macb_hresp_error_task() {
    ...
    bp->macbgem_ops.mog_init_rings(bp);
    ...
}

However, macb_hresp_error_task() does not acquire bp->lock or
queue->tx_ptr_lock, and it fails to call napi_disable() to synchronize with
RX/TX softirq paths.

Concurrently, NAPI polls (macb_tx_poll, macb_rx_poll) or macb_start_xmit()
might still be operating on the ring state.

Could this cause memory corruption or use-after-frees?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706-upstreaming-macb-irq-storm-v1-0-ab3115b5a13a@xxxxxxxxxxxxxxx?part=1