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

[Paolo Valerio <pvalerio@redhat.com>]

On 23 Feb 2026 at 04:09:00 PM, Jakub Kicinski <kuba@kernel.org> wrote:

> 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?
>

The branch on MACB_TYPE_SKB is a bit redundant, so I guess we
can either remove that or initialize skb at declaration to improve
clarity. There should be no potential NULL dereference anyways
(for the note below).

Next patch initializes the local variable and should be fine as well.

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