[PATCH net 2/6] net: stmmac: drop split frames on AF_XDP zero-copy RX
Lorenzo Bianconi
lorenzo.bianconi at oss.qualcomm.com
Tue Oct 27 03:17:30 PDT 2026
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 (2) · Medium: 1 (2) · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] The commit message does not describe one behaviour change the
> patch makes, and nothing explains the second Fixes tag.
>
> Pre-existing issues:
> - [High] On GMAC4/XGMAC, stmmac_rx() in native (non-ZC) XDP mode still
> handles split RX frames wrongly at this commit.
> - [High] On AF_XDP zero-copy XDP_PASS, stmmac_dispatch_skb_zc() ignores
> the csum_none bit from the descriptor status.
> - [Medium] In stmmac_rx(), if napi_build_skb() fails on the first
> descriptor of a multi-descriptor frame, the code does `rx_dropped++;…
> - [Medium] priv->xstats.rx_dropped and priv->xstats.rx_errors are device-
> wide `unsigned long` counters.
>
> --
>
> net: stmmac: drop split frames on AF_XDP zero-copy RX
>
> When stmmac_rx_zc() drops a frame that spans more than one RX descriptor,
> it now sets error, so the remaining descriptors are freed and the tail is
> not handled as a new frame. The dwmac100/dwmac1000 descriptor callbacks now
> return rx_not_ls together with discard_frame for non-last descriptors. The
> discard_frame checks in stmmac_rx_zc() and stmmac_rx() change from == to &.
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index fba21599a5e15..3fa2c15222d88 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -5710,18 +5710,21 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
> > if (priv->extend_desc)
> > stmmac_rx_extended_status(priv, &priv->xstats,
> > rx_q->dma_erx + entry);
> > - if (unlikely(status == discard_frame)) {
> > - xsk_buff_free(buf->xdp);
> > - buf->xdp = NULL;
> > - dirty++;
> > +
> > + if (unlikely(!error && (status & discard_frame))) {
> > error = 1;
> > if (!priv->hwts_rx_en)
> > rx_errors++;
> > }
>
> [Severity: High]
> This is a pre-existing issue, but with this test, a dwmac4 frame that only
> has csum_none set still reaches the XDP program. That covers an L4 payload
> checksum error without RDES3 ERROR_SUMMARY. On XDP_PASS the frame goes to
> stmmac_dispatch_skb_zc(), and that function never receives status:
This issue has not been introduced in this patch and it needs to be fixed with
a dedicated patch.
>
> stmmac_dispatch_skb_zc() {
> ...
> if (unlikely(!coe) || !stmmac_has_ip_ethertype(skb))
> skb_checksum_none_assert(skb);
> else
> skb->ip_summed = CHECKSUM_UNNECESSARY;
> ...
> }
>
> dwmac4_dma_rx_chan_op_mode() always sets MTL_OP_MODE_DIS_TCP_EF, so the
> hardware does not drop these frames. dwmac4_wrback_get_rx_status() reports
> them with no discard_frame:
>
> if (rdes1 & RDES1_IP_PAYLOAD_ERROR) {
> x->ip_payload_err++;
> ret |= csum_none;
> }
>
> Can a packet with a bad TCP/UDP checksum get marked CHECKSUM_UNNECESSARY
> on the AF_XDP zero-copy XDP_PASS path and be delivered to sockets?
>
> stmmac_rx() checks (status & csum_none) before it sets
> CHECKSUM_UNNECESSARY. Should stmmac_dispatch_skb_zc() do the same check?
> The gap is still there at the end of the series.
>
> [ ... ]
>
> > @@ -5730,8 +5733,9 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
> > if (likely(status & rx_not_ls)) {
> > xsk_buff_free(buf->xdp);
> > buf->xdp = NULL;
> > + rx_dropped++;
> > + error = 1;
> > dirty++;
> > - count++;
> > goto read_again;
> > }
>
> [Severity: Medium]
> This is a pre-existing issue, but the rx_dropped count added here is added
> to the device-wide counter at the end of stmmac_rx_zc():
>
> priv->xstats.rx_dropped += rx_dropped;
> priv->xstats.rx_errors += rx_errors;
>
> stmmac_rx() does the same thing, and stmmac_dispatch_skb_zc() does
> priv->xstats.rx_dropped++. These are plain unsigned long read-modify-writes
> with no lock, no atomic and no per-queue split.
>
> NAPI polls for different RX queues can finish at the same time on
> different CPUs. Can they lose increments here, so that stmmac_get_stats64()
> under-reports rx_dropped and rx_errors?
This issue has not been introduced in this patch and it needs to be fixed with
a dedicated patch.
>
> [ ... ]
>
> > @@ -5898,7 +5902,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> >
> > if (priv->extend_desc)
> > stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx + entry);
> > - if (unlikely(status == discard_frame)) {
> > +
> > + if (unlikely(status & discard_frame)) {
> > page_pool_put_page(rx_q->page_pool, buf->page, 0, true);
> > buf->page = NULL;
> > error = 1;
>
> [Severity: Medium]
> The commit message does not seem to describe this change or explain the
> second Fixes tag:
>
> Fixes: 644b8437ccef9e ("net: stmmac: dwmac4: report Rx checksum errors in status")
>
> Is this change from == to & the actual fix for 644b8437ccef9e?
> dwmac4_wrback_get_rx_status() sets ret = discard_frame on
> RDES3_ERROR_SUMMARY. It then ORs in csum_none on RDES1_IP_HDR_ERROR or
> RDES1_IP_PAYLOAD_ERROR, so it can return discard_frame | csum_none.
>
> The old status == discard_frame test missed that value. Frames with CRC,
> overflow, watchdog or giant errors that also failed the IP checksum were
> passed to the stack in stmmac_rx(), or to XDP/XSK in stmmac_rx_zc().
>
> The fix looks correct. However, the body only covers the AF_XDP zero-copy
> 1:1 drop and the dwmac100/dwmac1000 descriptor change, and this hunk
> changes the regular non-XDP RX path on every GMAC4/XGMAC system.
>
> Could the commit message describe this change and the 644b8437ccef9e
> regression it fixes?
>
> [Severity: High]
> This is a pre-existing issue, but does stmmac_rx() in native XDP mode
> still mishandle split frames on GMAC4/XGMAC? For a non-last descriptor,
> dwmac4_wrback_get_rx_status() returns rx_not_ls without discard_frame:
>
> if (likely(!(rdes3 & RDES3_LAST_DESCRIPTOR)))
> return rx_not_ls;
>
> So the discard_frame drain above never starts, and the XDP program sees
> the first fragment as a complete frame. On XDP_DROP:
>
> if (xdp_res & STMMAC_XDP_CONSUMED) {
> ...
> skb = NULL;
>
> if (unlikely((status & rx_not_ls)))
> goto read_again;
>
> skb is NULL and error is still 0. The tail then goes through XDP again
> and can be passed to the stack as a new packet.
>
> On XDP_TX or XDP_REDIRECT the code does goto next, which clears
> in_progress. The next iteration resets len to 0, so stmmac_rx_buf1_len()
> reports the full dma_buf_sz for the tail. Can that expose stale page_pool
> contents that DMA did not write for this frame?
>
> This is the same kind of bug the patch fixes for stmmac_rx_zc(), but here
> stmmac_rx() is only covered for dwmac100/dwmac1000. The next patch in the
> series, "net: stmmac: drop multi-buffer frames in XDP mode", appears to
> fix it. That patch sets error on rx_not_ls in XDP mode, drops frames that
> use buf2_len, and makes the XDP_CONSUMED path goto next.
I will fix the commit message in v2.
>
> [Severity: Medium]
> This is a pre-existing issue, but stmmac_rx() has a similar problem when
> napi_build_skb() fails on the first descriptor of a multi-descriptor
> frame:
>
> skb = napi_build_skb(page_address(buf->page),
> rx_q->napi_skb_frag_size);
> if (!skb) {
> page_pool_recycle_direct(rx_q->page_pool,
> buf->page);
> buf->page = NULL;
> rx_dropped++;
> count++;
> goto drain_data;
> }
>
> error is not set, so drain_data jumps to read_again with skb == NULL and
> error == 0. If allocation succeeds on the next descriptor of the same
> frame, the if (!skb) branch builds a new skb from only the tail fragment.
>
> At the last descriptor, pskb_trim(skb, len) does nothing. len still
> includes the dropped first fragment, so it is larger than skb->len.
>
> Does this pass the tail to the stack as a standalone packet with its FCS
> still attached, and count the frame against the budget twice? Should this
> path set error so the rest of the frame is drained?
>
> At the end of the series this only happens in non-XDP mode, because XDP
> mode now sets error on rx_not_ls.
This issue has not been introduced in this patch and it needs to be fixed with
a dedicated patch.
Regards,
Lorenzo
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023-stmmac-xdp-fixes-v1-0-efb37e19f5bd%40oss.qualcomm.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20261027/8fd7af30/attachment-0001.sig>
More information about the linux-arm-kernel
mailing list