Message-ID: <aV-gxT3Ijk_8cmHS@shell.armlinux.org.uk>
Date: Thu, 8 Jan 2026 12:19:17 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>,
	linux-arm-kernel@...ts.infradead.org,
	linux-stm32@...md-mailman.stormreply.com,
	Maxime Coquelin <mcoquelin.stm32@...il.com>, netdev@...r.kernel.org,
	Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next 4/9] net: stmmac: descs: fix buffer 1 off-by-one
 error

On Thu, Jan 08, 2026 at 11:41:15AM +0000, Russell King (Oracle) wrote:
> On Wed, Jan 07, 2026 at 10:28:30AM +0100, Maxime Chevallier wrote:
> > Hi Russell,
> > 
> > On 06/01/2026 21:31, Russell King (Oracle) wrote:
> > > norm_set_tx_desc_len_on_ring() incorrectly tests the buffer length,
> > > leading to a length of 2048 being squeezed into a bitfield covering
> > > bits 10:0 - which results in the buffer 1 size being zero.
> > > 
> > > If this field is zero, buffer 1 is ignored, and thus is equivalent
> > > to transmitting a zero length buffer.
> > > 
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
> > 
> > Should it be a fix ? I've tried to trigger the bug without success, this
> > seems to be fairly specific so I'm OK with it going to net-next.
> 
> Note that you need hardware that doesn't use enhanced descriptors -
> which descriptors get used are dependent on the hardware rather than a
> runtime option.
> 
> Note that we have this silly code, which I've brought up in the past:
> 
>         if (priv->plat->core_type == DWMAC_CORE_XGMAC)
>                 ndev->max_mtu = XGMAC_JUMBO_LEN;
>         else if (priv->plat->enh_desc || priv->synopsys_id >= DWMAC_CORE_4_00)
>                 ndev->max_mtu = JUMBO_LEN;
>         else
>                 ndev->max_mtu = SKB_MAX_HEAD(NET_SKB_PAD + NET_IP_ALIGN);
> 
> where the "silly" part is that last line - SKB_MAX_HEAD() is dependent
> on PAGE_SIZE. So, if you build your kernel for e.g. 64K page sizes, but
> stmmac doesn't have enhanced descriptor support, ->max_mtu ends up being
> close to 64K, and you can configure the netdev's MTU to be that large.
> 
> Even with a 4KiB page size, max_mtu will certainly be greater than
> 2KiB.
> 
> That means stmmac_xmit() can be called with packets >= 2KiB in length.
> As stmmac_xmit() has this:
> 
>         /* To program the descriptors according to the size of the frame */
>         if (enh_desc)
>                 is_jumbo = stmmac_is_jumbo_frm(priv, skb->len, enh_desc);
> 
> the code will not treat them as jumbo frames, and thus
> stmmac_jumbo_frm() will not be called. This means we'll call
> stmmac_set_desc_addr() and stmmac_prepare_tx_desc() only for each
> fragment of the skb, which only supports buffer 1 in the descriptor.
> 
> There is the possibility for a descriptor to supply the next chunk of
> the packet in buffer 2 (with its separate length field of the same
> bit size) but the driver doesn't do that in this path.
> 
> So, even if we did get a fragment >= 2KiB, the code would only be able
> to send up to the maximum size that can fit in the descriptor.

Reviewing the docs again, I actually think it's worse than this.

Consider the case where enh_desc = false, so we're using normal
descriptors. in stmmac_xmit() is_jumbo will always be false because
we never check stmmac_is_jumbo_frm() for normal descriptors. Thus,
we use the paths in stmmac_xmit() which only call
stmmac_set_desc_addr() and stmmac_prepare_tx_desc().

For normal descriptors, these correspond with ndesc_set_addr() and
ndesc_prepare_tx_desc().

ndesc_set_addr() sets tdes2 to the address - this is the buffer1 DMA
address.

In ring mode, ndesc_prepare_tx_desc() calls
norm_set_tx_desc_len_on_ring() which divides the buffer between the
buffer1 length and buffer2 length. This means, in theory, that normal
descriptors can transmit up to 4KiB - 2 split across two 2KiB - 1
buffers. However, nothing sets tdes3, which is the buffer2 DMA address,
so either we transmit garbage (I suspect whatever happens to be at
DMA address zero) or DMA fails.

In chain mode, ndesc_prepare_tx_desc() calls
norm_set_tx_desc_len_on_chain() which only sets the buffer1 length
(masking off the bits that don't fit in the field.) This is because
tdes3 is used to point at the next descriptor, so each normal
descriptor can only contain buffers up to 2KiB - 1.

This all comes down to the "silly" code that I mentioned previously.

While one can argue that it would be nice to fully fix this, I suspect
the reality is that almost no one cares, because hardly anyone uses
"normal" descriptors, especially with more recent hardware.

However, my thoughts more centre around the idiotic max_mtu setting
that didn't gain any traction when I tried to bring that topic up.
The simple solution here would be to ensure max_mtu isn't set to
SKB_MAX_HEAD() but left as the standard setting (thus not allowing
the MTU to be increased beyond ETH_DATA_LEN, aka 1500) which would
then prevent buffer1 being anywhere near crossing the 2KiB-1
threshold.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists