Re: [net-next,v2,7/8] net: macb: make tx path skb agnostic

[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.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: macb: make tx path skb agnostic

This commit renames the skb member of macb_tx_buff to ptr and introduces
macb_tx_buff_type to identify the buffer type. This is preparatory work
for XDP transmit support.

> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 4e3cc0e9ea87..67fef88af2c6 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h

[ ... ]

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index f65f976123fd..50646ee90672 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c

[ ... ]

> @@ -1204,7 +1205,9 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
>  		/* Process all buffers of the current transmitted frame */
>  		for (;; tail++) {
>  			tx_buff = macb_tx_buff(queue, tail);
> -			skb = tx_buff->skb;
> +
> +			if (tx_buff->type == MACB_TYPE_SKB)
> +				skb = tx_buff->ptr;
>
>  			/* First, update TX stats if needed */
>  			if (skb) {

In macb_tx_complete(), the local variable skb is declared without
initialization at the start of the outer for loop. The patch changes the
unconditional assignment to a conditional one based on tx_buff->type.

When tx_buff->type is not MACB_TYPE_SKB, skb remains uninitialized with
whatever garbage value was on the stack. The subsequent "if (skb)" check
will likely evaluate as true on this garbage pointer, causing dereferences
of skb->len, skb->data, and skb_shinfo(skb) on invalid memory.

This could lead to a crash or memory corruption. Should skb be initialized
to NULL at declaration?

Note: While all buffers are MACB_TYPE_SKB in this commit, the commit message
states this is the "last preparatory step for xdp xmit support," suggesting
the next commit will introduce non-SKB buffer types that would trigger this
issue.