[PATCH net-next] net: stmmac: rework stmmac_rx to support XDP rx multi-buff
netdev-bot+sashiko at kernel.org
netdev-bot+sashiko at kernel.org
Tue Sep 22 08:24:20 PDT 2026
Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.
Critical: 0 · High: 2 · Medium: 3 (1) · Low: 1
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).
- [High] stmmac_rx(): after stmmac_build_skb() fails (napi_build_skb()
returns NULL), the code does `rx_dropped++; count++; goto read_again;`…
- [High] The patch introduces a new invariant — 'rx_q->state_saved ==
true implies rx_q->state.xdp holds a valid in-progress xdp_buff' — but…
- [Medium] stmmac_rx() now ends with an unconditional `rx_q->state_saved
= !first_desc;` (line 6056).
- [Medium] The new RX code can hand the XDP program a fragmented xdp_buff
(stmmac_build_xdp_frags() calls xdp_buff_set_frags_flag() and fills…
- [Medium] The commit subject and message state that the XDP program now
'runs on the full (possibly fragmented) packet instead of just the…
- [Low] Concern (assessed as a false positive): stmmac_build_xdp_frags()
recycles the excess page via page_pool_put_full_page() once…
Pre-existing issues:
- [Medium] Concern (assessed as not reportable): stmmac_rx_buf2_len()
returns the unsigned expression 'plen - len' with no check that plen…
--
net: stmmac: rework stmmac_rx to support XDP rx multi-buff
The RX path now accumulates every descriptor of a frame into a single
xdp_buff so the XDP program sees the whole (possibly fragmented) packet, and
builds the skb from that buffer with napi_build_skb() plus
xdp_update_skb_frags_info(). The in-progress xdp_buff is stored in
rx_q->state instead of a partially built skb so it survives a NAPI poll
boundary.
A few questions below about the new failure paths and the saved state.
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index 4fc96b317d79..69bdbbf4b920 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -132,7 +132,7 @@ struct stmmac_rx_queue {
> dma_addr_t dma_rx_phy;
> unsigned int state_saved;
> struct {
> - struct sk_buff *skb;
> + struct xdp_buff xdp;
> unsigned int len;
> unsigned int error;
> } state;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index bf9e7e4cb1c3..1de8d9626c64 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_xdp_put_buff(struct stmmac_rx_queue *rx_q, struct xdp_buff *xdp,
> + int sync_len)
> +{
> + struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
> + int i;
> +
> + if (likely(!xdp_buff_has_frags(xdp)))
> + goto out;
> +
> + for (i = 0; i < sinfo->nr_frags; i++)
> + page_pool_put_full_page(rx_q->page_pool,
> + skb_frag_page(&sinfo->frags[i]), true);
> +out:
> + page_pool_put_page(rx_q->page_pool, virt_to_head_page(xdp->data),
> + sync_len, true);
> +}
> +
> +static struct sk_buff *stmmac_build_skb(struct xdp_buff *xdp)
> +{
> + struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
> + u32 metasize = xdp->data - xdp->data_meta;
> + struct sk_buff *skb;
> + u8 num_frags;
> +
> + if (unlikely(xdp_buff_has_frags(xdp)))
> + num_frags = sinfo->nr_frags;
> +
> + skb = napi_build_skb(xdp->data_hard_start, xdp->frame_sz);
> + if (!skb)
> + return ERR_PTR(-ENOMEM);
> +
> + skb_mark_for_recycle(skb);
> + skb_reserve(skb, xdp->data - xdp->data_hard_start);
> + skb_put(skb, xdp->data_end - xdp->data);
> + if (metasize)
> + skb_metadata_set(skb, metasize);
> +
> + if (unlikely(xdp_buff_has_frags(xdp)))
> + xdp_update_skb_frags_info(skb, num_frags, sinfo->xdp_frags_size,
> + num_frags * xdp->frame_sz,
> + xdp_buff_get_skb_flags(xdp));
> + return skb;
> +}
> +
> +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);
^^^^
[Severity: Medium]
This marks buffers that are subsequently passed to the XDP program as
multi-buffer, but the advertised feature mask in stmmac_dvr_probe() is
unchanged:
ndev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
NETDEV_XDP_ACT_XSK_ZEROCOPY;
Should NETDEV_XDP_ACT_RX_SG be set here, so core and userspace (netlink
NETDEV_A_DEV_XDP_FEATURES, bpftool, the AF_XDP selftests) can tell that this
NAPI callback may hand non-linear buffers to a program?
The commit message says the XDP program now "runs on the full (possibly
fragmented) packet instead of just the first buffer". Is that reachable with
this patch alone? stmmac_xdp_set_prog() is untouched and still has:
if (prog && dev->mtu > ETH_DATA_LEN) {
NL_SET_ERR_MSG_MOD(extack, "Jumbo frames not supported");
return -EOPNOTSUPP;
}
...
priv->sph_active = priv->sph_capable && !stmmac_xdp_is_enabled(priv);
so with a program attached the MTU is at most 1500 and split-header is off,
and stmmac_set_bfsize() then returns at least DEFAULT_BUFSIZE, which holds a
whole frame in one descriptor. Should the gating rework come with this
patch, or should the message describe the change as preparatory?
> + sinfo->xdp_frags_size = 0;
> + sinfo->nr_frags = 0;
> + }
> +
> + 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);
> + }
> +}
> +
[ ... ]
> @@ -5780,9 +5859,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> u32 hash;
>
> if (!count && rx_q->state_saved) {
> - skb = rx_q->state.skb;
> + ctx.xdp = rx_q->state.xdp;
> error = rx_q->state.error;
> len = rx_q->state.len;
> + first_desc = false;
[Severity: High]
This restore assumes that state_saved == true implies state.xdp holds a
valid in-progress buffer. Is that invariant maintained everywhere?
stmmac_rx_zc() still ends with:
if (status & rx_not_ls) {
rx_q->state_saved = true;
rx_q->state.error = error;
rx_q->state.len = len;
}
and never writes rx_q->state.xdp. If an AF_XDP socket is unbound after a
multi-descriptor frame straddled the budget boundary, the next non-zero-copy
stmmac_rx() takes the branch above with first_desc = false and a state.xdp
that was never written, and the first non-zero buf1_len/buf2_len reaches
stmmac_build_xdp_frags():
struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
...
sinfo->xdp_frags_size = 0;
sinfo->nr_frags = 0;
with sinfo == data_hard_start + frame_sz - SKB_DATA_ALIGN(sizeof(struct
skb_shared_info)). For a zeroed state that is a write near (void *)-320.
Should stmmac_rx_zc() either store a buffer too, or leave state_saved clear?
Separately, is anything releasing state.xdp on teardown?
dma_free_rx_skbufs() and __free_dma_rx_desc_resources() only walk
buf_pool[], while the head page and the already-collected fragments were
detached from it (buf->page = NULL, buf->sec_page = NULL), so
page_pool_destroy() would still see them in flight.
And after a reconfiguration that reuses the same struct stmmac_rx_queue with
a fresh rx_q->page_pool (ethtool ring/channel change, MTU change, XDP
attach/detach, tx-timeout reset), state_saved is still set, so the stale
buffer is restored, written to, and finally returned with
page_pool_put_page(rx_q->page_pool, ...) of a different pool than the one
that owns those pages. Should the init/teardown paths clear state_saved and
release state.xdp?
> } else {
> rx_q->state_saved = false;
> skb = NULL;
[ ... ]
> @@ -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);
> skb = NULL;
> count++;
[ ... ]
> @@ -5866,6 +5948,28 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> xdp_init_buff(&ctx.xdp, bufsz, &rx_q->xdp_rxq);
> xdp_prepare_buff(&ctx.xdp, page_address(buf->page),
> buf->page_offset, buf1_len, true);
> + first_desc = false;
> + buf->page = NULL;
> + } else if (buf1_len) {
> + stmmac_build_xdp_frags(priv, rx_q, buf1_len, buf->page,
> + buf->page_offset, dma_dir,
> + &ctx.xdp);
> + buf->page = NULL;
> + }
> +
> + if (buf2_len) {
> + stmmac_build_xdp_frags(priv, rx_q, buf2_len,
> + buf->sec_page, buf->page_offset,
> + dma_dir, &ctx.xdp);
> + buf->sec_page = NULL;
> + }
> +
> + if (likely(status & rx_not_ls))
> + goto read_again;
> +
> + first_desc = true;
> + if (!skb) {
> + unsigned int pre_len, sync_len;
>
> pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start -
> buf->page_offset;
[ ... ]
> @@ -5906,7 +6007,6 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> } else if (xdp_res & (STMMAC_XDP_TX |
> STMMAC_XDP_REDIRECT)) {
> xdp_status |= xdp_res;
> - buf->page = NULL;
> skb = NULL;
> count++;
> continue;
[Severity: Medium]
An XDP_TX verdict is treated as a successful hand-off here, but is the TX
side fragment aware? stmmac_xdp_xmit_xdpf() never tests
xdp_frame_has_frags(); it maps xdpf->data for xdpf->len, programs one
descriptor and then calls:
stmmac_set_tx_dma_last_segment(tx_q, entry);
xdp_update_frame_from_buff() copies xdp->flags into the frame but leaves
xdpf->len as the linear head length only, the total being
xdp_get_frame_len() = xdpf->len + sinfo->xdp_frags_size. Can a fragmented
buffer therefore be transmitted with only the head on the wire while still
counted as STMMAC_XDP_TX?
stmmac_xdp_set_prog() also calls xdp_features_set_redirect_target(dev,
false), which states that this xmit path cannot take scatter-gather frames.
Should fragmented frames be linearised or refused before XDP_TX?
> @@ -5914,51 +6014,13 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> }
> }
>
> - if (!skb) {
> - unsigned int head_pad_len;
> -
> - /* XDP program may expand or reduce tail */
> - buf1_len = ctx.xdp.data_end - ctx.xdp.data;
> -
> - 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);
> - rx_dropped++;
> - count++;
> - goto drain_data;
> - }
> -
[ ... ]
> -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;
> - if (!skb)
> - continue;
> + }
[Severity: High]
When napi_build_skb() inside stmmac_build_skb() fails, skb keeps
ERR_PTR(-ENOMEM) and len keeps the aborted frame's length, and the jump
lands on read_again:, which sits below the per-frame reset:
} else {
rx_q->state_saved = false;
skb = NULL;
error = 0;
len = 0;
}
read_again:
Is the next frame then processed with the error pointer and the stale length
still in place?
If the next descriptor reports discard_frame, the error path above runs
dev_kfree_skb(skb), and dev_kfree_skb() is consume_skb(), which only checks
for NULL before calling skb_unref(). Can that dereference ERR_PTR(-ENOMEM)
in softirq context?
The gate ahead of stmmac_xdp_run_prog() is "if (!skb)", which is false for a
non-NULL error pointer, so the program is skipped for the following frame
and that frame is then rebuilt by stmmac_build_skb() and passed to
napi_gro_receive(). Does this silently bypass the attached program for one
frame?
With split-header active, the stale non-zero len makes stmmac_rx_buf1_len()
return 0 and stmmac_rx_buf2_len() compute the unsigned
return plen - len;
which wraps when the following frame is shorter than the aborted one. That
value is then used as the dma_sync_single_for_cpu() size and as the fragment
length in stmmac_build_xdp_frags(). Would resetting skb and len (or using
continue, as the pre-patch code effectively did by falling through
drain_data into "if (!skb) continue;") be safer here?
>
> /* 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]
This assignment is now unconditional. netpoll polls with a zero budget:
net/core/netpoll.c:poll_one_napi()
/* We explicitly pass the polling call a budget of 0 to
* indicate that we are clearing the Tx path only.
*/
work = napi->poll(napi, 0);
and stmmac_napi_poll_rx() forwards that budget to stmmac_rx() with no early
return. With limit == 0 the while (count < limit) body never runs, so
first_desc keeps its initialiser value true and state_saved is cleared while
state.xdp still owns the head page plus the fragments collected so far,
which were detached from buf_pool with buf->page = NULL.
Can this orphan those page pool pages, and can the remaining descriptors of
that frame then be parsed as the start of a new frame, with
xdp_prepare_buff() run on a middle or last descriptor? The previous tail
condition "if (status & rx_not_ls || skb)" left state_saved untouched in
that case, since status is 0 and skb is NULL.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918-stmmac-rx-mb-v1-1-0b4517d404af%40oss.qualcomm.com
More information about the linux-arm-kernel
mailing list