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