[PATCH net] net: stmmac: fix rx Scatter-Gather support
netdev-bot+sashiko at kernel.org
netdev-bot+sashiko at kernel.org
Sun Sep 20 08:40:22 PDT 2026
Thank you for your contribution! Sashiko AI review found 9 potential
issue(s) to consider.
Critical: 1 · High: 1 (1) · Medium: 6 · 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).
- [Critical] stmmac_rx() strips the FCS with an unchecked unsigned
subtraction (`buf2_len -= ETH_FCS_LEN` at stmmac_main.c:5803,…
- [High] The patch unconditionally allocates a secondary RX page and
unconditionally reports a non-zero buffer2 length for every MAC core,…
- [Medium] The RX page pool is created with `pp_params.offset =
stmmac_rx_offset(priv)` and `max_len = dma_conf->dma_buf_sz`, and only…
- [Medium] For GMAC4 without SPH, stmmac_rx_buf2_len() now assumes every
intermediate descriptor's buffer2 is completely filled (returns…
- [Medium] The patch makes the secondary page-pool page mandatory for
every RX descriptor on every stmmac device and configuration…
- [Medium] With a native XDP program attached, stmmac_xdp_set_prog()
explicitly clears priv->sph_active, and this patch now arms buffer2…
- [Medium] The AF_XDP zero-copy refill path is left untouched:
stmmac_rx_refill_zc() still calls `stmmac_set_desc_sec_addr(priv,…
- [Medium] The new length arithmetic depends on `priv->hw->rx_csum`,
which stmmac_set_features() (stmmac_main.c:6210-6214) mutates from…
- [Low] The descriptor-position comments in the changed RX length helpers
no longer describe the code.
Pre-existing issues:
- [High] In stmmac_rx(), when napi_build_skb() fails the code calls
page_pool_recycle_direct(rx_q->page_pool, buf->page) and jumps to…
--
net: stmmac: fix rx Scatter-Gather support
The patch always allocates and programs the secondary RX buffer (sec_page)
instead of doing so only when split-header is active, so that a frame
overflowing buffer1 does not make the DMA write to an unmapped address.
stmmac_rx_buf1_len() and stmmac_rx_buf2_len() are adjusted to account for
buffer2 in the non-SPH case.
A few questions about the buffer2 accounting and the cores/paths that are
now affected.
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 1fb5f804ea234..be7cb0cafeb50 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1659,17 +1659,14 @@ static int stmmac_init_rx_buffers(struct stmmac_priv *priv,
> buf->page_offset = stmmac_rx_offset(priv);
> }
>
> - if (priv->sph_active && !buf->sec_page) {
> + if (!buf->sec_page) {
> buf->sec_page = page_pool_alloc_pages(rx_q->page_pool, gfp);
> if (!buf->sec_page)
> return -ENOMEM;
>
> buf->sec_addr = page_pool_get_dma_addr(buf->sec_page);
> - stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
> - } else {
> - buf->sec_page = NULL;
> - stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, false);
> }
> + stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
[Severity: High]
Does the "always mark buffer2 as valid" part hold on the legacy cores?
Only dwmac4_descs.c and dwxgmac2_descs.c provide a set_sec_addr callback;
enh_desc_ops (dwmac1000) and ndesc_ops (dwmac100) do not, and the
dispatch macro silently skips an absent callback:
drivers/net/ethernet/stmicro/stmmac/hwif.h:stmmac_do_void_callback() {
int __result = -EINVAL; \
if ((__priv)->hw->__module && (__priv)->hw->__module->__cname) { \
(__priv)->hw->__module->__cname((__arg0), ##__args); \
...
}
So on those cores buf->sec_addr is never handed to the hardware, and in
ring mode with 16 KiB buffers the hardware buffer2 is inside the primary
page anyway:
drivers/net/ethernet/stmicro/stmmac/ring_mode.c:refill_desc3() {
if (priv->dma_conf.dma_buf_sz == BUF_SIZE_16KiB)
p->des3 = cpu_to_le32(le32_to_cpu(p->des2) + BUF_SIZE_8KiB);
}
With the "!priv->sph_active" early return gone from stmmac_rx_buf2_len(),
buf2_len becomes non-zero on those cores too, and stmmac_rx() then does:
if (buf2_len) {
dma_sync_single_for_cpu(priv->device, buf->sec_addr,
buf2_len, dma_dir);
skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
buf->sec_page, 0, buf2_len,
priv->dma_conf.dma_buf_sz);
Can this publish the contents of a page the DMA never wrote to the network
stack? On dwmac1000/dwmac100 the intermediate descriptors of a scattered
frame are discarded by enh_desc_get_rx_status()/ndesc_get_rx_status(), so
the last descriptor is processed with len = 0 and its FL field is the total
frame length; a 3000-byte frame with dma_buf_sz = 2048 would give
buf1_len = 2048 and buf2_len = 952 out of the untouched sec_page. For
larger jumbo frames buf2_len (plen - dma_buf_sz) can also exceed the
order-0 sec_page. Should buffer2 be gated on the cores that can actually
program an independently mapped buffer2?
[Severity: Medium]
The secondary buffer is handed to the DMA at page offset 0, but the page
pool is only configured with offset 0 when SPH is active:
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:__alloc_dma_rx_desc_resources() {
pp_params.offset = stmmac_rx_offset(priv);
pp_params.max_len = dma_conf->dma_buf_sz;
if (priv->sph_active) {
pp_params.offset = 0;
pp_params.max_len += stmmac_rx_offset(priv);
}
}
net/core/page_pool.c:__page_pool_dma_sync_for_device() {
dma_sync_size = min(dma_sync_size, pool->p.max_len);
__dma_sync_single_for_device(pool->p.dev, dma_addr + pool->p.offset,
dma_sync_size, pool->p.dma_dir);
}
With !sph_active the pool syncs from dma_addr + NET_SKB_PAD + NET_IP_ALIGN,
so the first bytes of every secondary page are given to the device without
a sync-for-device, both on first map and on recycle. On a non-coherent
platform, can a dirty cache line in that prefix be written back over data
the device has already placed there? The SPH path sets offset = 0 for
exactly this reason.
> @@ -5097,7 +5094,7 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
> break;
> }
>
> - if (priv->sph_active && !buf->sec_page) {
> + if (!buf->sec_page) {
> buf->sec_page = page_pool_alloc_pages(rx_q->page_pool, gfp);
> if (!buf->sec_page)
> break;
[Severity: Medium]
This makes a second page-pool page mandatory for every RX descriptor on
every stmmac device, doubling the RX buffer footprint (order-2/order-3
pages for the 8/16 KiB bfsize cases), while pp_params.pool_size stays at
dma_conf->dma_rx_size, i.e. the recycle ring is now sized for half the
in-flight pages.
For MTU-conformant traffic dma_buf_sz is always above the MTU:
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_set_bfsize() {
if (mtu >= BUF_SIZE_8KiB)
ret = BUF_SIZE_16KiB;
...
else
ret = DEFAULT_BUFSIZE;
}
so buffer1 alone holds the frame and the second page is unused. Could the
commit message state in which configuration an in-spec frame exceeds
dma_buf_sz, and mention the extra page per descriptor plus the pool_size
mismatch?
> @@ -5108,10 +5105,7 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
> buf->addr = page_pool_get_dma_addr(buf->page) + buf->page_offset;
>
> stmmac_set_desc_addr(priv, p, buf->addr);
> - if (priv->sph_active)
> - stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
> - else
> - stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, false);
> + stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
> stmmac_refill_desc3(priv, rx_q, p);
[Severity: Medium]
Native XDP always runs in the non-SPH mode that this patch now arms
buffer2 for:
drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c:stmmac_xdp_set_prog() {
priv->sph_active = priv->sph_capable && !stmmac_xdp_is_enabled(priv);
}
In stmmac_rx() the program only ever sees buffer1:
xdp_prepare_buff(&ctx.xdp, page_address(buf->page),
buf->page_offset, buf1_len, true);
and on a TX/redirect verdict the loop leaves before the buf2_len block:
} else if (xdp_res & (STMMAC_XDP_TX |
STMMAC_XDP_REDIRECT)) {
xdp_status |= xdp_res;
buf->page = NULL;
skb = NULL;
count++;
continue;
}
XDP caps the configured MTU at ETH_DATA_LEN, but GMAC_CORE_INIT in
dwmac4.h contains GMAC_CONFIG_JE and XGMAC_CORE_INIT_RX programs
GPSL = XGMAC_JUMBO_LEN, so an oversized frame is still accepted and split.
Does this silently forward a truncated frame, and should the multi-buffer
case be rejected instead?
[Severity: Medium]
The AF_XDP zero-copy refill path is left as it was:
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_rx_refill_zc() {
stmmac_set_desc_addr(priv, rx_desc, dma_addr);
stmmac_set_desc_sec_addr(priv, rx_desc, 0, false);
}
but on XGMAC the validity argument is ignored and there is no BUF2V bit in
the RX read descriptor:
drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c:dwxgmac2_set_sec_addr() {
p->des2 = cpu_to_le32(lower_32_bits(addr));
p->des3 = cpu_to_le32(upper_32_bits(addr));
}
so buffer2 stays armed at DMA address 0 while RBSZ for a ZC queue is
xsk_pool_get_rx_frame_size() and GPSL is the jumbo limit. Is the SMMU
translation fault quoted in the commit message still reachable on this
path? The multi-descriptor rejection in stmmac_rx_zc() happens after the
DMA write.
> @@ -5160,7 +5154,7 @@ static unsigned int stmmac_rx_buf1_len(struct stmmac_priv *priv,
> plen = stmmac_get_rx_frame_len(priv, p, coe);
>
> /* First descriptor and last descriptor and not split header */
> - return min_t(unsigned int, priv->dma_conf.dma_buf_sz, plen);
> + return min_t(unsigned int, priv->dma_conf.dma_buf_sz, plen - len);
> }
[Severity: Low]
This isn't a bug, but the descriptor-position comments here no longer
match the code. The non-first-descriptor early return at the top of
stmmac_rx_buf1_len() is gated on priv->sph_active, so with non-SPH
scatter-gather a continuation descriptor (len != 0) falls through to the
branches commented "First descriptor, not last descriptor and not split
header" and "First descriptor and last descriptor and not split header",
while plen - len only makes sense for a continuation descriptor.
> @@ -5170,10 +5164,6 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
> int coe = priv->hw->rx_csum;
> unsigned int plen = 0;
>
> - /* Not split header, buffer is not available */
> - if (!priv->sph_active)
> - return 0;
> -
[Severity: Critical]
With this return gone, the last-descriptor path returns the exact
remainder plen - len, which can be 1, 2 or 3. stmmac_rx() then strips the
FCS with an unchecked unsigned subtraction:
/* ACS is disabled; strip manually. */
if (likely(!(status & rx_not_ls))) {
if (buf2_len) {
buf2_len -= ETH_FCS_LEN;
len -= ETH_FCS_LEN;
} else if (buf1_len) {
buf1_len -= ETH_FCS_LEN;
len -= ETH_FCS_LEN;
}
}
and passes the result straight on:
if (buf2_len) {
dma_sync_single_for_cpu(priv->device, buf->sec_addr,
buf2_len, dma_dir);
skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
buf->sec_page, 0, buf2_len,
priv->dma_conf.dma_buf_sz);
Can a frame that overflows a buffer boundary by 1..3 bytes wrap this?
GMAC_CORE_INIT in dwmac4.h sets GMAC_CONFIG_JE unconditionally, so with
MTU 1500 and dma_buf_sz = DEFAULT_BUFSIZE = 1536 a 1537-byte frame is
accepted and split as 1536 + 1. On that first-and-last descriptor
stmmac_rx_buf1_len() returns min(1536, 1537 - 0) = 1536, then
stmmac_rx_buf2_len() returns 1537 - 1536 = 1, and 1 - 4 gives 0xfffffffd
being handed to dma_sync_single_for_cpu() and skb_add_rx_frag(). The
buf1_len branch is reachable the same way with frames of
2 * n * dma_buf_sz + 1..3 bytes, and stmmac_rx_zc() also subtracts
ETH_FCS_LEN from stmmac_rx_buf1_len() unconditionally. Before this patch
the non-SPH path returned 0 here, and in SPH mode buffer1 ends at the
split header so the whole FCS always landed in buffer2.
> /* For GMAC4, when split header is enabled, in some rare cases, the
> * hardware does not fill buf2 of the first descriptor with payload.
> * Thus we cannot assume buf2 is always fully filled if it is not
> @@ -5188,8 +5178,9 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
> * Thus 'plen - len' always gives the correct length of buf2.
> */
>
> - /* Not GMAC4 and not last descriptor */
> - if (priv->plat->core_type != DWMAC_CORE_GMAC4 && (status & rx_not_ls))
> + /* Not GMAC4, or non-SPH and not last descriptor */
> + if ((priv->plat->core_type != DWMAC_CORE_GMAC4 || !priv->sph_active) &&
> + (status & rx_not_ls))
> return priv->dma_conf.dma_buf_sz;
>
> /* GMAC4 or last descriptor */
[Severity: Medium]
For GMAC4 without SPH this now assumes every intermediate descriptor's
buffer2 is completely filled, discarding the accumulated-PL value that the
comment just above describes as always giving the correct length of buf2.
The fall-through has no len <= plen guard:
plen = stmmac_get_rx_frame_len(priv, p, coe);
return plen - len;
and stmmac_rx_buf1_len() clamps the same wrapped value with
min_t(unsigned int, dma_buf_sz, plen - len) rather than detecting the
inconsistency. One way the assumption can break is the RBSZ field itself:
DMA_RBSZ_MASK is GENMASK(14, 1) in dwmac4_dma.h and cannot represent
BUF_SIZE_16KiB, so dwmac4_set_dma_bfsize() writes a truncated RBSZ while
the driver keeps assuming dma_buf_sz per buffer. If an intermediate
buffer2 is short by even one byte, does the accumulated len exceed plen and
turn buf2_len into a multi-gigabyte fragment length? Would a
plen > len ? plen - len : 0 form be safer here?
The commit message says the GMAC4 plus split-header path keeps the
accumulated payload semantics "since there an intermediate descriptor's
buffer2 can be only partially filled" - is there a databook statement that
this cannot happen for GMAC4 outside SPH mode?
[Severity: Medium]
Both helpers read priv->hw->rx_csum independently for the same descriptor
(int coe = priv->hw->rx_csum;) and feed it to stmmac_get_rx_frame_len(),
whose COE_TYPE1 path subtracts 2 from the descriptor frame length. The
writer runs from process context with no synchronization against the RX
NAPI poll:
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_set_features() {
if (features & NETIF_F_RXCSUM)
priv->hw->rx_csum = priv->plat->rx_coe;
else
priv->hw->rx_csum = 0;
...
stmmac_rx_ipc(priv, priv->hw);
}
If ethtool -K ethX rx off/on lands between the stmmac_rx_buf1_len() and
stmmac_rx_buf2_len() reads on type-1-COE hardware, plen in the second call
is 2 bytes smaller than the one used to accumulate len, so plen - len
wraps. Before this patch the non-SPH path returned 0 here and buf1_len did
no subtraction, so the worst case was a 2-byte length inaccuracy. Could
coe/plen be read once per descriptor?
[Severity: High]
This isn't a bug introduced by this patch, but while looking at the RX
buffer lifecycle the patch reworks: in stmmac_rx() the napi_build_skb()
failure path releases the page without clearing buf->page:
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;
}
Every other consumption site in the function sets buf->page = NULL after
handing the page away. With the stale pointer, stmmac_rx_refill() skips
the replacement allocation and programs the already-recycled page back
into the descriptor:
if (!buf->page) {
buf->page = page_pool_alloc_pages(rx_q->page_pool, gfp);
and at teardown stmmac_free_rx_buffer() puts it a second time:
page_pool_put_full_page(rx_q->page_pool, buf->page, false);
buf->page = NULL;
Can this alias one page into two descriptors and underflow the page-pool
refcount?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916-stmmac-rx-sg-fix-v1-1-b49b7b8f725f%40oss.qualcomm.com
More information about the linux-arm-kernel
mailing list