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

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

[-- Attachment #1: Type: text/plain, Size: 6524 bytes --]

> 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

I will address sashiko's comments in v2.

Regards,
Lorenzo

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]