Re: [PATCH net-next v6 4/7] net: macb: make macb_tx_skb generic

[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: make macb_tx_skb generic

This commit renames the macb_tx_skb structure to macb_tx_buff with no
functional changes. This is done as a preparatory step for adding XDP
xmit support.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c

[ ... ]

> @@ -2404,12 +2404,12 @@ static unsigned int macb_tx_map(struct macb *bp,
>  	}
>  
>  	/* Should never happen */
> -	if (unlikely(!tx_skb)) {
> +	if (unlikely(!tx_buff)) {
>  		netdev_err(bp->dev, "BUG! empty skb!\n");
>  		return 0;
>  	}

Does returning 0 here cause a memory leak? 

If macb_tx_map() returns 0 for an empty skb, the caller macb_start_xmit()
will assume the packet was successfully enqueued and will not free it. Should
this return an error code like -EINVAL instead to trigger the 
dev_kfree_skb_any() error path?

[ ... ]

> @@ -2808,8 +2808,8 @@ static void macb_free_consistent(struct macb *bp)
>  	dma_free_coherent(dev, size, bp->queues[0].rx_ring, bp->queues[0].rx_ring_dma);
>  
>  	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> -		kfree(queue->tx_skb);
> -		queue->tx_skb = NULL;
> +		kfree(queue->tx_buff);
> +		queue->tx_buff = NULL;

Is queue->tx_error_task guaranteed to be canceled before we free
queue->tx_buff here?

Since macb_close() calls macb_free_consistent() without explicitly calling
cancel_work_sync(&queue->tx_error_task), could a pending background error
task execute and access the freed array, resulting in a use-after-free?

[ ... ]

> @@ -2887,9 +2887,9 @@ static int macb_alloc_consistent(struct macb *bp)
>  		queue->rx_ring = rx + macb_rx_ring_size_per_queue(bp) * q;
>  		queue->rx_ring_dma = rx_dma + macb_rx_ring_size_per_queue(bp) * q;
>  
> -		size = bp->tx_ring_size * sizeof(struct macb_tx_skb);
> -		queue->tx_skb = kmalloc(size, GFP_KERNEL);
> -		if (!queue->tx_skb)
> +		size = bp->tx_ring_size * sizeof(struct macb_tx_buff);
> +		queue->tx_buff = kmalloc(size, GFP_KERNEL);
> +		if (!queue->tx_buff)
>  			goto out_err;
>  	}

This isn't a bug, but would it be better to use kcalloc() or kmalloc_array()
here instead of manual size multiplication?