netdev - Re: [PATCH net-next] stmmac: align RX buffers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210817020157.3b9d015e@linux.microsoft.com>
Date:   Tue, 17 Aug 2021 02:01:57 +0200
From:   Matteo Croce <mcroce@...ux.microsoft.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Marc Zyngier <maz@...nel.org>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Thierry Reding <thierry.reding@...il.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-riscv@...ts.infradead.org,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        "David S. Miller" <davem@...emloft.net>,
        Palmer Dabbelt <palmer@...belt.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Drew Fustini <drew@...gleboard.org>,
        Emil Renner Berthing <kernel@...il.dk>,
        Jon Hunter <jonathanh@...dia.com>,
        Will Deacon <will@...nel.org>
Subject: Re: [PATCH net-next] stmmac: align RX buffers

On Mon, 16 Aug 2021 08:12:08 -0700
Jakub Kicinski <kuba@...nel.org> wrote:

> On Thu, 12 Aug 2021 12:05:38 +0100 Marc Zyngier wrote:
> > > A possible fix, which takes in account also the XDP headroom for
> > > stmmac_rx_buf1_len() only could be (only compile tested, I don't
> > > have the hardware now):  
> > 
> > However, this doesn't fix my issue. I still get all sort of
> > corruption. Probably stmmac_rx_buf2_len() also need adjusting (it
> > has a similar logic as its buf1 counterpart...)
> > 
> > Unless you can fix it very quickly, and given that we're towards the
> > end of the cycle, I'd be more comfortable if we reverted this patch.
> 
> Any luck investigating this one? The rc6 announcement sounds like
> there may not be that many more rc releases for 5.14.

Hi Jackub.

Unfortunately I have only a device with stmmac, and it works fine with
the patch. It seems that not all hardware suffers from this issue.

Also, using NET_IP_ALIGN on RX is a common pattern, I think that any
ethernet device is doing the same to align the IPv4 header.

Anyway, I asked for two tests on the affected device:
1. Change NET_IP_ALIGN with 8, to see if the DMA has problems in
   receiving to a non word aligned address
2. load a nop XDP program (I provided one), to see if the problem is
   already there when XDP is used

I doubt that changing also stmmac_rx_buf2_len would help,
but it's worth a try, here is a patch:

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 7b8404a21544..73d1f0ec66ff 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -93,7 +93,7 @@ static int tc = TC_DEFAULT;
 module_param(tc, int, 0644);
 MODULE_PARM_DESC(tc, "DMA threshold control value");
 
-#define	DEFAULT_BUFSIZE	1536
+#define	DEFAULT_BUFSIZE	1536 + XDP_PACKET_HEADROOM + NET_IP_ALIGN
 static int buf_sz = DEFAULT_BUFSIZE;
 module_param(buf_sz, int, 0644);
 MODULE_PARM_DESC(buf_sz, "DMA buffer size");
@@ -4508,12 +4508,12 @@ static unsigned int stmmac_rx_buf1_len(struct stmmac_priv *priv,
 
 	/* First descriptor, not last descriptor and not split header */
 	if (status & rx_not_ls)
-		return priv->dma_buf_sz;
+		return priv->dma_buf_sz - stmmac_rx_offset(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_buf_sz, plen);
+	return min_t(unsigned int, priv->dma_buf_sz - stmmac_rx_offset(priv), plen);
 }
 
 static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
@@ -4529,12 +4529,12 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
 
 	/* Not last descriptor */
 	if (status & rx_not_ls)
-		return priv->dma_buf_sz;
+		return priv->dma_buf_sz - stmmac_rx_offset(priv);
 
 	plen = stmmac_get_rx_frame_len(priv, p, coe);
 
 	/* Last descriptor */
-	return plen - len;
+	return plen - len - stmmac_rx_offset(priv);
 }
 
 static int stmmac_xdp_xmit_xdpf(struct stmmac_priv *priv, int queue,


Regards,
-- 
per aspera ad upstream