Re: [PATCH net-next v6 5/7] net: macb: generalize tx buffer handling

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: macb: generalize tx buffer handling

This commit renames the skb member in struct macb_tx_buff to ptr and
introduces an enum macb_tx_buff_type. This prepares the driver for XDP xmit
support by allowing the buffer to handle both sk_buff and xdp frames.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index a71d36b18170..4c5dd70f003b 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1197,7 +1197,8 @@ static int macb_halt_tx(struct macb *bp)
>  					bp, TSR);
>  }
>  
> -static void macb_tx_unmap(struct macb *bp, struct macb_tx_buff *tx_buff, int budget)
> +static void macb_tx_unmap(struct macb *bp, struct macb_tx_buff *tx_buff,
> +			  int budget)
>  {

[ ... ]

> -	if (tx_buff->skb) {
> -		napi_consume_skb(tx_buff->skb, budget);
> -		tx_buff->skb = NULL;
> +	if (tx_buff->ptr) {
> +		napi_consume_skb(tx_buff->ptr, budget);
> +		tx_buff->ptr = NULL;
>  	}
>  }

Does the teardown path iterate over the uncompleted descriptors to call
macb_tx_unmap()?

When the interface is closed, the hardware is reset and macb_free_consistent()
frees the tx_buff array directly via kfree(queue->tx_buff).

If the teardown path does not iterate from tx_tail to tx_head, will any
packets (and future xdp frames) that are currently in the transmit ring when
the interface is brought down be permanently leaked, along with their
associated DMA mappings?

[ ... ]

> @@ -1298,7 +1299,7 @@ static void macb_tx_error_task(struct work_struct *work)
>  		desc = macb_tx_desc(queue, tail);
>  		ctrl = desc->ctrl;
>  		tx_buff = macb_tx_buff(queue, tail);
> -		skb = tx_buff->skb;
> +		skb = tx_buff->ptr;

Is the tx_error_task workqueue canceled before freeing tx_buff during device
teardown?

The macb_tx_error_task background task accesses the queue->tx_buff array and
the hardware tx_ring. During device teardown, macb_free_consistent() frees
queue->tx_buff using kfree().

If cancel_work_sync(&queue->tx_error_task) is not called in the teardown
path, could a transmit error schedule this workqueue just before the device
is brought down, causing the task to execute after the structures have been
destroyed and resulting in a use-after-free?

[ ... ]

> @@ -2365,7 +2368,8 @@ static unsigned int macb_tx_map(struct macb *bp,
>  			goto dma_error;
>  
>  		/* Save info to properly release resources */
> -		tx_buff->skb = NULL;
> +		tx_buff->ptr = NULL;
> +		tx_buff->type = MACB_TYPE_SKB;

What happens if this function receives an empty skb and fails to find any
fragments?

Looking at the rest of macb_tx_map(), if the packet has no data length, it
appears the function logs an error and returns 0:

	/* Should never happen */
	if (unlikely(!tx_buff)) {
		netdev_err(bp->dev, "BUG! empty skb!\n");
		return 0;
	}

Since the caller treats 0 as success, it would skip the error path that frees
the skb. As no descriptor was actually queued and tx_head wasn't updated, will
the packet never be unmapped or freed by the completion path, causing a
permanent memory leak? Should this return a non-zero error code instead?

[ ... ]

> @@ -5283,8 +5289,9 @@ static netdev_tx_t at91ether_start_xmit(struct sk_buff *skb,
>  		netif_stop_queue(dev);
>  
>  		/* Store packet information (to free when Tx completed) */
> -		lp->rm9200_txq[desc].skb = skb;
> +		lp->rm9200_txq[desc].ptr = skb;
>  		lp->rm9200_txq[desc].size = skb->len;
> +		lp->rm9200_txq[desc].type = MACB_TYPE_SKB;
>  		lp->rm9200_txq[desc].mapping = dma_map_single(&lp->pdev->dev, skb->data,

Can a DMA mapping error here cause a permanent queue hang?

If dma_mapping_error() fails just below this, the driver calls
dev_kfree_skb_any(skb) and returns NETDEV_TX_OK but leaves the queue stopped
via netif_stop_queue(dev).

Because no transmission is started, the hardware will never generate a
completion interrupt, leaving the queue permanently hung.

Additionally, ptr is left pointing to the freed memory, creating a dangling
pointer. Does this need to be cleared when the error occurs?