Re: [PATCH net-next v5 3/3] net: stmmac: allow non-linear xdp_buff in XDP mode

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 1 · High: 2 (2) · Medium: 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).

- [Critical] Out-of-bounds read in the AF_XDP zero-copy RX path,
  triggerable by a remote peer.
- [High] XDP_TX and ndo_xdp_xmit frames longer than buffer1 of a legacy
  (normal or enhanced) TX descriptor are programmed wrongly.
- [High] On XDP_TX of the head buffer, stmmac_xdp_xmit_xdpf() computes
  the DMA address as `page_pool_get_dma_addr(virt_to_page(xdpf->data)) +…
- [Medium] On bugged_jumbo cores (snps,dwmac-3.40a, snps,dwmac-3.610,
  snps,dwmac-3.710 per stmmac_probe_config_dt()), TX checksum insertion…
- [Medium] net/stmmac: silent drop of jumbo frames with non-SG XDP

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  makes it much easier to hit.
- [High] This problem wasn't introduced by this patch, but the patch
  exposes it to XDP and to XDP_TX/redirect.

--

net: stmmac: allow non-linear xdp_buff in XDP mode

This commit removes the check that rejected attaching an XDP program
when the MTU is above ETH_DATA_LEN. It also removes the check that
refused to raise the MTU above ETH_DATA_LEN while XDP is enabled. The
reason given is that the RX path now builds multi-buffer xdp_buffs.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index be2cba8fcbc6d..cbeb43011c4bd 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -6351,12 +6351,6 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
>  		txfifosz = priv->dma_cap.tx_fifo_size;
>  
>  	txfifosz /= priv->plat->tx_queues_to_use;
> -
> -	if (stmmac_xdp_is_enabled(priv) && new_mtu > ETH_DATA_LEN) {
> -		netdev_dbg(priv->dev, "Jumbo frames not supported for XDP\n");
> -		return -EINVAL;
> -	}
> -

[Severity: Critical]

Can this lead to an out-of-bounds read in the AF_XDP zero-copy RX path?

ZC RX only runs while an XDP program is attached. Before this patch,
that kept dma_conf.dma_buf_sz at 1536.

With XDP attached and, say, MTU 9000, stmmac_setup_dma_desc() now picks
BUF_SIZE_16KiB. For a ZC queue, stmmac_set_queue_rx_buf_size() programs
the hardware buffer size from the pool:

    buf_size = xsk_pool_get_rx_frame_size(rx_q->xsk_pool);

So a jumbo frame gets split across several descriptors. stmmac_rx_zc()
still assumes a 1:1 mapping. It drops the non-last buffers without
adding to len or setting error:

    /* XSK pool expects RX frame 1:1 mapped to XSK buffer */
    if (likely(status & rx_not_ls)) {
        xsk_buff_free(buf->xdp);
        buf->xdp = NULL;
        dirty++;
        count++;
        goto read_again;
    }

When the last descriptor arrives, len is still 0, so stmmac_rx_buf1_len()
returns about the full frame length:

    return min_t(unsigned int, priv->dma_conf.dma_buf_sz, plen - len);

stmmac_rx_zc() then sets:

    buf->xdp->data_end = buf->xdp->data + buf1_len;

This puts data_end about 9000 bytes past data. The XSK chunk only holds
xsk_pool_get_rx_frame_size() bytes.

Three consumers would then read past the chunk:

  - the BPF program
  - the memcpy() in stmmac_construct_skb_zc() on XDP_PASS
  - xdp_convert_zc_to_xdp_frame() on XDP_TX

They would read neighbouring UMEM chunks, or fault past the end of the
UMEM mapping in NAPI context.

Both orders seem to reach this state:

  - bind the pool at MTU 1500, then raise the MTU here
  - raise the MTU first, then attach the program

The mtu + ETH_PAD_LEN <= frame_size check in xp_assign_dev() only runs
at bind time. stmmac_xdp_enable_pool() only requires frame_size >=
ETH_FRAME_LEN + VLAN_HLEN * 2, and its comment says "XDP ZC does not
span multiple frame".

The commit message says "XDP can operate on frames bigger than
ETH_DATA_LEN". That doesn't seem to hold for the ZC path, and the driver
still advertises NETDEV_XDP_ACT_XSK_ZEROCOPY.

Should the MTU limit stay in place while an XSK pool is bound, or be
checked against xsk_pool_get_rx_frame_size()?

[Severity: High]

This is a pre-existing issue, but this patch exposes it to XDP programs
and to XDP_TX/redirect. In chain mode (forced for dwmac-sun8i, or with
chain_mode=1), does the RX length accounting match what the hardware
writes?

chain_mode_ops has no set_16kib_bfsize, so stmmac_set_bfsize() picks
dma_buf_sz = BUF_SIZE_16KiB for len > BUF_SIZE_8KiB. The descriptor
init caps hardware buffer1:

enh_desc_init_rx_desc():
    bfsize1 = min(bfsize, BUF_SIZE_8KiB);

ndesc_init_rx_desc() caps it at 2047. Chain mode has no buffer2,
because des3 is the next-descriptor pointer.

stmmac_rx_buf1_len() still returns the full software size for every
non-last descriptor:

    if (status & rx_not_ls)
        return priv->dma_conf.dma_buf_sz;

For the last descriptor, the unsigned plen - len wraps (for example
9022 - 16368), so min_t() returns dma_buf_sz again.

Wouldn't a 9022-byte frame then become a ~32 KiB xdp_buff that is
mostly stale data from recycled page_pool pages? The XDP program would
see that data, and XDP_TX would send it back out. The non-XDP skb path
builds from the same xdp_buff and looks affected in the same way.

>  	new_mtu = STMMAC_ALIGN(new_mtu);
>  
>  	/* If condition true, FIFO is too small or MTU too large */
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
> index 9af9e0cb366c3..f6b3e8628a7cf 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
> @@ -109,14 +109,6 @@ int stmmac_xdp_set_prog(struct stmmac_priv *priv, struct bpf_prog *prog,
>  
>  	if_running = netif_running(dev);
>  
> -	if (prog && dev->mtu > ETH_DATA_LEN) {
> -		/* For now, the driver doesn't support XDP functionality with
> -		 * jumbo frames so we return error.
> -		 */
> -		NL_SET_ERR_MSG_MOD(extack, "Jumbo frames not supported");
> -		return -EOPNOTSUPP;
> -	}
> -

[Severity: Medium]
Removing both checks entirely means a single-buffer XDP program can now
be combined with a jumbo MTU. Neither stmmac_xdp_set_prog() nor
stmmac_change_mtu() looks at prog->aux->xdp_has_frags. That allows the
setup in either order:

  - attach a program without frags support at MTU 9000
  - raise the MTU while such a program is attached

After that, any frame that spans more than one RX buffer arrives as an
xdp_buff with frags. stmmac_xdp_run_prog() then skips the program:

    if (likely(!xdp_buff_has_frags(xdp) || prog->aux->xdp_has_frags))
        res = __stmmac_xdp_run_prog(priv, prog, xdp);

res stays at STMMAC_XDP_CONSUMED, so the frame is dropped. All traffic
larger than one RX buffer is blackholed. The user got no error when
setting this up that would explain why.

Other multi-buffer capable drivers reject this combination up front.
mvneta, for example, refuses to attach a program without xdp_has_frags
when the MTU exceeds its single-buffer size. It also refuses the
matching MTU change while such a program is loaded.

Should these two checks be kept, but applied only when the program
lacks xdp_has_frags? If so, is ETH_DATA_LEN still the right threshold,
or should it be the largest frame that fits in one RX buffer?

[Severity: High]

With XDP now allowed at jumbo MTU, can stmmac_xdp_xmit_xdpf() program
legacy (normal or enhanced) TX descriptors with buffers longer than
buffer1?

stmmac_xdp_xmit_xdpf() writes one descriptor per buffer. It sets only
des2 and passes the full length:

    stmmac_set_desc_addr(priv, desc, dma_addr);
    stmmac_prepare_tx_desc(priv, desc, !i, len, csum,
                           priv->descriptor_mode, !!i, last_frame,
                           xdp_get_frame_len(xdpf));

In ring mode, enh_set_tx_desc_len_on_ring() moves the excess into the
buffer2 size field when len > 4096:

    if (unlikely(len > buffer1_max_length)) {
        p->des1 |= cpu_to_le32(FIELD_PREP(ETDES1_BUFFER2_SIZE_MASK,
                               len - buffer1_max_length) |

norm_set_tx_desc_len_on_ring() does the same above 2047. The DMA engine
then reads buffer2 from des3, which the XDP path never sets. It is
usually 0, or a stale value.

The skb path handles this in stmmac_xmit() through
stmmac_is_jumbo_frm(), with jumbo_frm() in ring_mode.c doing:

    desc->des3 = cpu_to_le32(lower_32_bits(des2) + BUF_SIZE_4KiB);

In chain mode, des3 is the next-descriptor pointer and the length is
masked with ETDES1_BUFFER1_SIZE_MASK. A 9018-byte frame would go out
truncated.

Should stmmac_xdp_xmit_xdpf() split oversized buffers the same way the
skb path does?

[Severity: High]

Does stmmac_xdp_xmit_xdpf() still compute the right DMA address for the
head buffer, now that RX pages can be compound?

    page = frag ? skb_frag_page(frag)
                : virt_to_page(xdpf->data);
    dma_addr = page_pool_get_dma_addr(page);

The offset added afterwards (sizeof(*xdpf) + xdpf->headroom) is relative
to the head page. virt_to_page() returns the page that contains
xdpf->data.

At jumbo MTU, __alloc_dma_rx_desc_resources() now ends up with
pp_params.order = order_base_2(num_pages) > 0 while XDP is enabled.

Suppose a program moves data forward with bpf_xdp_adjust_head() by more
than about a page and returns XDP_TX. Then xdpf->data is in a tail page,
and the tail page doesn't hold the page_pool DMA address. Wouldn't
dma_sync_single_for_device() and the TX descriptor get a bogus bus
address?

stmmac_xdp_put_buff() in the same file already uses
virt_to_head_page(xdp->data) for the same buffer.

[Severity: Medium]

The bugged_jumbo cores are snps,dwmac-3.40a, snps,dwmac-3.610 and
snps,dwmac-3.710. On these, stmmac_fix_features() disables TX checksum
insertion at jumbo MTUs:

    if (priv->plat->bugged_jumbo && (dev->mtu > ETH_DATA_LEN))
        features &= ~NETIF_F_CSUM_MASK;

stmmac_xdp_xmit_xdpf() makes its own decision:

    bool csum = !priv->plat->tx_queues_cfg[queue].coe_unsupported;

enh_desc_prepare_tx_desc() then sets TX_CIC_FULL.

Now that XDP is allowed at MTU > ETH_DATA_LEN, wouldn't XDP_TX and
redirected jumbo frames on these cores go out with full checksum
insertion enabled? That is what the bugged_jumbo workaround is meant to
avoid.

[Severity: High]

This isn't a bug introduced by this patch, but the patch makes it much
easier to hit. Further down in stmmac_xdp_set_prog(), the result of the
reopen is ignored and the function returns 0:

    if (if_running && need_update)
        stmmac_xdp_open(dev);

If stmmac_xdp_open() fails, its error paths have already freed the DMA
resources:

init_error:
    free_dma_desc_resources(priv, &priv->dma_conf);

The NAPIs that stmmac_xdp_release() disabled stay disabled, and the
device stays IFF_UP.

The next ndo_stop or XDP toggle reaches __stmmac_disable_all_queues():

    if (queue < rx_queues_cnt)
        napi_disable(&ch->rx_napi);

napi_disable() on a NAPI that is already disabled loops forever in
napi_disable_locked() while holding RTNL.

Any path that gets past that calls free_dma_desc_resources() again.
__free_dma_rx_desc_resources() doesn't clear dma_rx, buf_pool or
page_pool, so that would be a double free.

With this patch, attaching or detaching XDP at jumbo MTU makes the
reopen fill every RX ring entry with order-2/3 page_pool pages. These
are allocated with GFP_ATOMIC and can fail on a fragmented system.

Should the stmmac_xdp_open() error be propagated, with the device state
cleaned up?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009-stmmac-rx-mb-v5-0-c38fa4eaa138%40oss.qualcomm.com