Re: [PATCH net-next] net: stmmac: rework stmmac_rx to support XDP rx multi-buff

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] stmmac: loop state leak on skb build failure causes panic and memory corruption
- [High] stmmac: uninitialized xdp_frags_truesize corrupts socket memory accounting
- [High] stmmac: silently truncated frames bypass checksum errors and cause data corruption
- [Medium] stmmac: unconditional rx state clear when NAPI budget is zero leaks memory
--

commit 342011b18b585ad2ea2189dab3bfe7efcf08625c
Author: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

net: stmmac: rework stmmac_rx to support XDP rx multi-buff
    
Build the xdp_buff by accumulating all the descriptors that make up a
frame, so the XDP program runs on the full (possibly fragmented) packet
instead of just the first buffer. When the frame is not consumed by the
program, assemble the skb from the head buffer and the collected
fragments via napi_build_skb()/xdp_update_skb_frags_info().

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index bf9e7e4cb1c31..1de8d9626c640 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5735,6 +5737,82 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
>  	return failure ? limit : (int)count;
>  }
>  
[ ... ]
> +static void stmmac_build_xdp_frags(struct stmmac_priv *priv,
> +				   struct stmmac_rx_queue *rx_q,
> +				   unsigned int len, struct page *page,
> +				   unsigned int offset,
> +				   enum dma_data_direction dma_dir,
> +				   struct xdp_buff *xdp)
> +{
> +	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
> +	dma_addr_t dma_addr = page_pool_get_dma_addr(page) + offset;
> +
> +	dma_sync_single_for_cpu(priv->device, dma_addr, len, dma_dir);
> +
> +	if (!xdp_buff_has_frags(xdp)) {
> +		xdp_buff_set_frags_flag(xdp);
> +		sinfo->xdp_frags_size = 0;
> +		sinfo->nr_frags = 0;
> +	}

[Severity: High]
Does this code fail to initialize sinfo->xdp_frags_truesize, leaving it with
uninitialized memory?

If the XDP program redirects the packet, generic XDP handlers like
xdp_build_skb_from_buff() use tsize = sinfo->xdp_frags_truesize ? : ...

Could this random non-zero garbage be added to skb->truesize, causing massive
socket memory accounting (sk_rmem_alloc) corruption and subsequent drops?

> +
> +	if (sinfo->nr_frags < MAX_SKB_FRAGS) {
> +		skb_frag_t *frag = &sinfo->frags[sinfo->nr_frags++];
> +
> +		skb_frag_fill_page_desc(frag, page, offset, len);
> +		sinfo->xdp_frags_size += len;
> +
> +		if (page_is_pfmemalloc(page))
> +			xdp_buff_set_frag_pfmemalloc(xdp);
> +	} else {
> +		page_pool_put_full_page(rx_q->page_pool, page, true);
> +	}

[Severity: High]
If a packet exceeds MAX_SKB_FRAGS, does returning the excess pages to the pool
here silently truncate the payload without marking the frame as errored?

Because the hardware perfectly validated the original full frame's checksum,
could the truncated frame be passed up to the network stack where stmmac_rx()
will set skb->ip_summed = CHECKSUM_UNNECESSARY? 

While the IPv4/v6 stack might drop it due to length mismatch, could bridged
Layer-2 traffic or XDP redirection forward the truncated payload with a valid
checksum, leading to silent data corruption at the destination?

> +}
> +
>  /**
>   * stmmac_rx - manage the receive process
>   * @priv: driver private structure
[ ... ]
> @@ -5831,6 +5911,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  		if (unlikely(error && (status & rx_not_ls)))
>  			goto read_again;
>  		if (unlikely(error)) {
> +			if (!first_desc) {
> +				stmmac_xdp_put_buff(rx_q, &ctx.xdp, -1);
> +				first_desc = true;
> +			}
>  			dev_kfree_skb(skb);

[Severity: High]
Is it possible for dev_kfree_skb(skb) to cause a kernel panic here by
dereferencing an error pointer?

If stmmac_build_skb() fails later in this loop, it jumps to read_again and
skb will contain an ERR_PTR(-ENOMEM). If the next frame has an RX error,
this path will be taken with that error pointer.

>  			skb = NULL;
>  			count++;
[ ... ]
> @@ -5914,51 +6014,13 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  			}
>  		}
>  
> -		if (!skb) {
> -			unsigned int head_pad_len;
> -
[ ... ]
> -drain_data:
> -		if (likely(status & rx_not_ls))
> +		skb = stmmac_build_skb(&ctx.xdp);
> +		if (IS_ERR(skb)) {
> +			stmmac_xdp_put_buff(rx_q, &ctx.xdp, -1);
> +			rx_dropped++;
> +			count++;
>  			goto read_again;

[Severity: High]
Does this jump skip the loop header where skb = NULL and len = 0 reset the
state?

As noted above, this leaves skb as an ERR_PTR for the next iteration.
Additionally, since len is not reset, could stmmac_rx_buf2_len() later
underflow, producing a massive unsigned length for DMA syncs and leading to
memory corruption?

> -		if (!skb)
> -			continue;
> +		}
>  
>  		/* Got entire packet into SKB. Finish it. */
>  
[ ... ]
> @@ -5991,9 +6053,9 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  		count++;
>  	}
>  
> -	if (status & rx_not_ls || skb) {
> -		rx_q->state_saved = true;
> -		rx_q->state.skb = skb;
> +	rx_q->state_saved = !first_desc;
> +	if (!first_desc) {
> +		rx_q->state.xdp = ctx.xdp;
>  		rx_q->state.error = error;
>  		rx_q->state.len = len;
>  	}

[Severity: Medium]
Will this unconditionally drop previously saved state if limit == 0?

If stmmac_rx() is invoked with limit == 0 (e.g., via netpoll), the loop
while (count < limit) is bypassed entirely, and first_desc remains true.

Does setting rx_q->state_saved = !first_desc unconditionally overwrite the
saved state to false, discarding any partially accumulated xdp_buff from a
previous poll without freeing the associated fragments and causing a memory
leak?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918-stmmac-rx-mb-v1-1-0b4517d404af@oss.qualcomm.com?part=1