Re: [PATCH] net: stmmac: dwxgmac2: enable L2 split header support

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 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] The new writes in dwxgmac2_enable_sph() use XGMAC register
  offsets, but the XLGMAC core runs the same code and its register map…
- [High] net: stmmac: kernel memory information leak in split-header path
- [Medium] In dwxgmac2_enable_sph(), the new MAC-global writes (SPLM(1)
  and SAVE_EN in XGMAC_EXT_CFG1, and clearing VPRE in XGMAC_EXT_CFG0)…
- [Low] The commit message says it fixes an existing bug in
  dwxgmac2_get_rx_header_len(): on a first-but-not-last descriptor,…

--

net: stmmac: dwxgmac2: enable L2 split header support

This patch programs SPLM and SAVE_EN in XGMAC_EXT_CFG1 and clears the
variable-length preamble bit in XGMAC_EXT_CFG0 from dwxgmac2_enable_sph().
It also changes dwxgmac2_get_rx_header_len() to read RDES2[9:0] on
non-last descriptors, and adds L2T-based header length decoding on the
last descriptor.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
> index 6719ac6e395bc..65ac5403ec583 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
> @@ -292,8 +292,18 @@ static int dwxgmac2_get_rx_hash(struct dma_desc *p, u32 *hash,
>  
>  static void dwxgmac2_get_rx_header_len(struct dma_desc *p, unsigned int *len)
>  {
> -	if (le32_to_cpu(p->des3) & XGMAC_RDES3_L34T)
> +	u32 rdes3 = le32_to_cpu(p->des3);
> +
> +	/* when FD=1 and LD=0, HL is RDES2[9:0] */
> +	if (!(rdes3 & XGMAC_RDES3_LD)) {
> +		*len = le32_to_cpu(p->des2) & XGMAC_RDES2_HL;
> +		return;
> +	}

[Severity: Low]
The commit message presents this as a fix for an existing bug: on a
first-but-not-last descriptor, RDES3.L34T can hold stale write-back data.
The L34T-gated logic being replaced came from commit 67afd6d1cfdf ("net:
stmmac: Add Split Header support and enable it in XGMAC cores") and commit
96147375d49f ("net: stmmac: xgmac: Only get SPH header len if available").

If that is correct, current kernels would compute the wrong buffer 1
length for multi-descriptor split packets in stmmac_rx_buf1_len():

	stmmac_get_rx_header_len(priv, p, &hlen);
	if (priv->sph_active && hlen) {

Could this part go in its own patch with a Fixes: tag, so it can reach
stable? Right now it is bundled into a feature patch. The new
L2T/NONIPHL decoding also relies on the SPLM/SAVE_EN programming added
in the same patch, so the fix cannot be backported by itself.

> +
> +	if (rdes3 & XGMAC_RDES3_L34T)
>  		*len = le32_to_cpu(p->des2) & XGMAC_RDES2_HL;
> +	else if (rdes3 & XGMAC_RDES3_L2T)
> +		*len = (le32_to_cpu(p->des2) & XGMAC_RDES2_NONIPHL) >> 2;
>  }

[Severity: High]
The non-IP path masks RDES2 with GENMASK(9, 2) and shifts the result
down by 2. If RDES2 holds the header length in bytes, as it does on
the L34T path, this gives roughly a quarter of the real length. A
14-byte untagged Ethernet header becomes 3, and an 18-byte tagged
header becomes 4.

stmmac_rx_buf1_len() returns that hlen as the buffer 1 length on the
first descriptor. stmmac_rx_buf2_len() then computes the buffer 2
length on the last descriptor as the frame length minus the
accumulated len. An undersized buffer 1 length therefore has two
effects:

- the header copied into the skb is truncated;
- buffer 2 is treated as longer than what the DMA wrote, by the same
  number of bytes.

Those trailing bytes come from a recycled page_pool page. They can
hold data from an earlier packet, which is then handed to the stack
and possibly to userspace or forwarded out.

The wire can trigger this with any non-IP frame (ARP, AVTP, ...) that
fits in one descriptor, since FD and LD are both set then. That is the
common case for the tagged AV traffic this patch targets.

This is also inconsistent within the function. The non-last
descriptor path just above reads RDES2[9:0] with no shift.
dwmac4_get_rx_header_len() uses RDES2_HL, bits 9:0, directly as a
byte count.

What unit does the XGMAC2 databook give for the header length on L2T
packets? If it is a byte count in RDES2[9:0], should the mask and
shift be dropped? If the field really is an 8-bit byte count starting
at bit 2, could a comment or the commit message say so, and explain
why the non-LD path reads the same register differently?

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
> index ff83858ebc1f3..0a9c787305a4e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
> @@ -561,6 +561,16 @@ static void dwxgmac2_enable_sph(struct stmmac_priv *priv, void __iomem *ioaddr,
>  	value |= XGMAC_CONFIG_HDSMS_256; /* Segment max 256 bytes */
>  	writel(value, ioaddr + XGMAC_RX_CONFIG);
>  
> +	value = readl(ioaddr + XGMAC_EXT_CFG1);
> +	value |= XGMAC_CONFIG1_SPLM(1);
> +	value |= XGMAC_CONFIG1_SAVE_EN;
> +	writel(value, ioaddr + XGMAC_EXT_CFG1);
> +
> +	/* Disable variable preambles to keep the L2 split offset deterministic. */
> +	value = readl(ioaddr + XGMAC_EXT_CFG0);
> +	value &= ~XGMAC_EXT_CFG0_VPRE;
> +	writel(value, ioaddr + XGMAC_EXT_CFG0);

[Severity: High]
XLGMAC cores run this code as well. Is offset 0x140 still EXT_CFG0 on
those cores? The XLGMAC entry in stmmac_hw[] in hwif.c reuses the XGMAC
DMA ops:

		.dma = &dwxgmac210_dma_ops,
		.mac = &dwxlgmac2_ops,

On XLGMAC, 0x140 is the RX queue enable register (dwxlgmac2.h):

#define XLGMAC_RXQ_ENABLE_CTRL0		0x00000140

dwxlgmac2_rx_queue_enable() programs it using 2-bit per-queue fields:

	value = readl(ioaddr + XLGMAC_RXQ_ENABLE_CTRL0) & ~XGMAC_RXQEN(queue);
	if (mode == MTL_QUEUE_AVB)
		value |= 0x1 << XGMAC_RXQEN_SHIFT(queue);

With RX queue 4 in AVB mode, that sets BIT(8), which is the same bit as
XGMAC_EXT_CFG0_VPRE. Would clearing VPRE here disable RX queue 4 on
XLGMAC?

In stmmac_hw_setup(), stmmac_mtl_configuration() ->
stmmac_mac_enable_rx_queues() runs first. The split header loop runs
after it, with no sph_capable check:

	sph_en = (priv->hw->rx_csum > 0) && priv->sph_active;
	for (chan = 0; chan < rx_cnt; chan++)
		stmmac_enable_sph(priv, priv->ioaddr, sph_en, chan);

That means the clear would happen on every open, whether or not SPH is
supported. stmmac_xdp_open() and stmmac_set_features() repeat it.

The EXT_CFG1 write at 0x144 (SPLM bits 9:8 and bit 24) also lands on
XLGMAC. The driver headers do not say what register sits at that
offset on XLGMAC.

Should these writes be limited to XGMAC cores?

> +
>  	value = readl(ioaddr + XGMAC_DMA_CH_CONTROL(chan));
>  	if (en)
>  		value |= XGMAC_SPH;

[Severity: Medium]
Only XGMAC_SPH depends on en here. The new MAC-global EXT_CFG0/EXT_CFG1
writes run for both values of en.

stmmac_hw_setup() and stmmac_xdp_open() call stmmac_enable_sph() for
every RX channel without checking priv->sph_capable. Does this mean
every XGMAC gets VPRE cleared and SPLM/SAVE_EN set? That would include
parts without SPH, parts with STMMAC_FLAG_SPH_DISABLE, and parts where
SPH is inactive because of XDP or because rx_csum is off.

That does not match the commit message:

    Finally, disable the variable-length preamble when L2 splitting
    is enabled.

When SPH is turned off at runtime, through stmmac_set_features() or an
XDP attach via stmmac_xdp_open(), only the DMA SPH bit is cleared. VPRE
is never restored, and SPLM/SAVE_EN are never cleared.

Also, value |= XGMAC_CONFIG1_SPLM(1) does not clear GENMASK(9, 8) first.
If the field already holds a non-zero value, it is ORed with 1 rather
than replaced. And since enable_sph() is called per channel, these
MAC-global writes are repeated once per RX channel.

dwmac4_enable_sph() also writes SPLM/SAVE_EN unconditionally, but the
unconditional VPRE clear is new in this patch.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922232116.889630-1-sagar.cheluvegowda%40oss.qualcomm.com