From: Marc Zyngier <maz@kernel.org> To: Matteo Croce <mcroce@linux.microsoft.com> Cc: Eric Dumazet <eric.dumazet@gmail.com>, Thierry Reding <thierry.reding@gmail.com>, netdev@vger.kernel.org, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, linux-riscv <linux-riscv@lists.infradead.org>, Giuseppe Cavallaro <peppe.cavallaro@st.com>, Alexandre Torgue <alexandre.torgue@foss.st.com>, "David S. Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>, Palmer Dabbelt <palmer@dabbelt.com>, Paul Walmsley <paul.walmsley@sifive.com>, Drew Fustini <drew@beagleboard.org>, Emil Renner Berthing <kernel@esmil.dk>, Jon Hunter <jonathanh@nvidia.com>, Will Deacon <will@kernel.org> Subject: Re: [PATCH net-next] stmmac: align RX buffers Date: Thu, 19 Aug 2021 17:29:26 +0100 [thread overview] Message-ID: <87wnohqty1.wl-maz@kernel.org> (raw) In-Reply-To: <CAFnufp2=1t2+fmxyGJ0Qu3Z+=wRwAX8faaPvrJdFpFeTS3J7Uw@mail.gmail.com> On Thu, 12 Aug 2021 12:18:48 +0100, Matteo Croce <mcroce@linux.microsoft.com> wrote: > > [1 <text/plain; UTF-8 (7bit)>] > On Thu, Aug 12, 2021 at 1:05 PM Marc Zyngier <maz@kernel.org> wrote: > > > > On Thu, 12 Aug 2021 11:18:35 +0100, > > Matteo Croce <mcroce@linux.microsoft.com> wrote: > > > > > > On Thu, 12 Aug 2021 10:48:03 +0200 > > > Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > > > > > > > > > > > > On 8/11/21 4:16 PM, Marc Zyngier wrote: > > > > > On Wed, 11 Aug 2021 13:53:59 +0100, > > > > > Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > > > > > > >> Are you sure you do not need to adjust stmmac_set_bfsize(), > > > > >> stmmac_rx_buf1_len() and stmmac_rx_buf2_len() ? > > > > >> > > > > >> Presumably DEFAULT_BUFSIZE also want to be increased by NET_SKB_PAD > > > > >> > > > > >> Patch for stmmac_rx_buf1_len() : > > > > >> > > > > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > > >> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index > > > > >> 7b8404a21544cf29668e8a14240c3971e6bce0c3..041a74e7efca3436bfe3e17f972dd156173957a9 > > > > >> 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ > > > > >> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -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 - NET_SKB_PAD - > > > > >> NET_IP_ALIGN; > > > > >> 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 - NET_SKB_PAD > > > > >> - NET_IP_ALIGN, plen); } > > > > >> > > > > >> static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv, > > > > > > > > > > Feels like a major deficiency of the original patch. Happy to test a > > > > > more complete patch if/when you have one. > > > > > > > > I wont have time in the immediate future. > > > > > > > > Matteo, if you do not work on a fix, I suggest we revert > > > > a955318fe67ec0d962760b5ee58e74bffaf649b8 stmmac: align RX buffers > > > > > > > > before a more polished version can be submitted. > > > > > > > > > > Better to use stmmac_rx_offset() so to have the correct length when > > > using XDP. Also, when XDP is enabled, the offset was > > > XDP_PACKET_HEADROOM (i.e. 256 bytes) even before the change, so it > > > could be already broken. Mark, can you try on the Jetson TX2 by > > > attaching an XDP program and see if it works without my change? > > > > Sorry, you'll have to hold my hand here, as I know exactly nothing > > about XDP.... > > > > Attach the attached object with: > > ip link set eth0 xdp object passall.o > > This is an empty XDP program, its source: > > __attribute__((section("prog"), used)) > int xdp_main(struct xdp_md *ctx) > { > return XDP_PASS; > } > > Every packet will pass untouched, but the offset will be shifted from > 0 to 256 bytes, which could trigger the problem anyway: Nope. On 5.13, which doesn't have the issue, adding this payload doesn't result in any problem and the whole thing is rock solid. > > > > 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. > > > > Can it be that the HW can't do DMA on an address which is not word aligned? > What if you replace NET_SKB_PAD with, let's say, 8? With this: diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h index fcdb1d20389b..244aa6579ef4 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h @@ -341,7 +341,7 @@ static inline unsigned int stmmac_rx_offset(struct stmmac_priv *priv) if (stmmac_xdp_is_enabled(priv)) return XDP_PACKET_HEADROOM + NET_IP_ALIGN; - return NET_SKB_PAD + NET_IP_ALIGN; + return 8 + NET_IP_ALIGN; } void stmmac_disable_rx_queue(struct stmmac_priv *priv, u32 queue); I don't see the system corrupting packets anymore. Is that exactly what you had in mind? This really seems to point to a basic buffer overflow. Thanks, M. -- Without deviation from the norm, progress is not possible.
WARNING: multiple messages have this Message-ID (diff)
From: Marc Zyngier <maz@kernel.org> To: Matteo Croce <mcroce@linux.microsoft.com> Cc: Eric Dumazet <eric.dumazet@gmail.com>, Thierry Reding <thierry.reding@gmail.com>, netdev@vger.kernel.org, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, linux-riscv <linux-riscv@lists.infradead.org>, Giuseppe Cavallaro <peppe.cavallaro@st.com>, Alexandre Torgue <alexandre.torgue@foss.st.com>, "David S. Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>, Palmer Dabbelt <palmer@dabbelt.com>, Paul Walmsley <paul.walmsley@sifive.com>, Drew Fustini <drew@beagleboard.org>, Emil Renner Berthing <kernel@esmil.dk>, Jon Hunter <jonathanh@nvidia.com>, Will Deacon <will@kernel.org> Subject: Re: [PATCH net-next] stmmac: align RX buffers Date: Thu, 19 Aug 2021 17:29:26 +0100 [thread overview] Message-ID: <87wnohqty1.wl-maz@kernel.org> (raw) In-Reply-To: <CAFnufp2=1t2+fmxyGJ0Qu3Z+=wRwAX8faaPvrJdFpFeTS3J7Uw@mail.gmail.com> On Thu, 12 Aug 2021 12:18:48 +0100, Matteo Croce <mcroce@linux.microsoft.com> wrote: > > [1 <text/plain; UTF-8 (7bit)>] > On Thu, Aug 12, 2021 at 1:05 PM Marc Zyngier <maz@kernel.org> wrote: > > > > On Thu, 12 Aug 2021 11:18:35 +0100, > > Matteo Croce <mcroce@linux.microsoft.com> wrote: > > > > > > On Thu, 12 Aug 2021 10:48:03 +0200 > > > Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > > > > > > > > > > > > On 8/11/21 4:16 PM, Marc Zyngier wrote: > > > > > On Wed, 11 Aug 2021 13:53:59 +0100, > > > > > Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > > > > > > >> Are you sure you do not need to adjust stmmac_set_bfsize(), > > > > >> stmmac_rx_buf1_len() and stmmac_rx_buf2_len() ? > > > > >> > > > > >> Presumably DEFAULT_BUFSIZE also want to be increased by NET_SKB_PAD > > > > >> > > > > >> Patch for stmmac_rx_buf1_len() : > > > > >> > > > > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > > >> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index > > > > >> 7b8404a21544cf29668e8a14240c3971e6bce0c3..041a74e7efca3436bfe3e17f972dd156173957a9 > > > > >> 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ > > > > >> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -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 - NET_SKB_PAD - > > > > >> NET_IP_ALIGN; > > > > >> 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 - NET_SKB_PAD > > > > >> - NET_IP_ALIGN, plen); } > > > > >> > > > > >> static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv, > > > > > > > > > > Feels like a major deficiency of the original patch. Happy to test a > > > > > more complete patch if/when you have one. > > > > > > > > I wont have time in the immediate future. > > > > > > > > Matteo, if you do not work on a fix, I suggest we revert > > > > a955318fe67ec0d962760b5ee58e74bffaf649b8 stmmac: align RX buffers > > > > > > > > before a more polished version can be submitted. > > > > > > > > > > Better to use stmmac_rx_offset() so to have the correct length when > > > using XDP. Also, when XDP is enabled, the offset was > > > XDP_PACKET_HEADROOM (i.e. 256 bytes) even before the change, so it > > > could be already broken. Mark, can you try on the Jetson TX2 by > > > attaching an XDP program and see if it works without my change? > > > > Sorry, you'll have to hold my hand here, as I know exactly nothing > > about XDP.... > > > > Attach the attached object with: > > ip link set eth0 xdp object passall.o > > This is an empty XDP program, its source: > > __attribute__((section("prog"), used)) > int xdp_main(struct xdp_md *ctx) > { > return XDP_PASS; > } > > Every packet will pass untouched, but the offset will be shifted from > 0 to 256 bytes, which could trigger the problem anyway: Nope. On 5.13, which doesn't have the issue, adding this payload doesn't result in any problem and the whole thing is rock solid. > > > > 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. > > > > Can it be that the HW can't do DMA on an address which is not word aligned? > What if you replace NET_SKB_PAD with, let's say, 8? With this: diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h index fcdb1d20389b..244aa6579ef4 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h @@ -341,7 +341,7 @@ static inline unsigned int stmmac_rx_offset(struct stmmac_priv *priv) if (stmmac_xdp_is_enabled(priv)) return XDP_PACKET_HEADROOM + NET_IP_ALIGN; - return NET_SKB_PAD + NET_IP_ALIGN; + return 8 + NET_IP_ALIGN; } void stmmac_disable_rx_queue(struct stmmac_priv *priv, u32 queue); I don't see the system corrupting packets anymore. Is that exactly what you had in mind? This really seems to point to a basic buffer overflow. Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2021-08-19 16:29 UTC|newest] Thread overview: 74+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-14 2:25 [PATCH net-next] stmmac: align RX buffers Matteo Croce 2021-06-14 2:25 ` Matteo Croce 2021-06-14 19:51 ` David Miller 2021-06-14 19:51 ` David Miller 2021-06-14 23:21 ` Matteo Croce 2021-06-14 23:21 ` Matteo Croce 2021-06-15 17:28 ` David Miller 2021-06-15 17:28 ` David Miller 2021-06-15 17:30 ` patchwork-bot+netdevbpf 2021-06-15 17:30 ` patchwork-bot+netdevbpf 2021-08-10 19:07 ` Marc Zyngier 2021-08-10 19:07 ` Marc Zyngier 2021-08-11 10:28 ` Thierry Reding 2021-08-11 10:28 ` Thierry Reding 2021-08-11 12:53 ` Eric Dumazet 2021-08-11 12:53 ` Eric Dumazet 2021-08-11 14:16 ` Marc Zyngier 2021-08-11 14:16 ` Marc Zyngier 2021-08-12 8:48 ` Eric Dumazet 2021-08-12 8:48 ` Eric Dumazet 2021-08-12 10:18 ` Matteo Croce 2021-08-12 10:18 ` Matteo Croce 2021-08-12 11:05 ` Marc Zyngier 2021-08-12 11:05 ` Marc Zyngier 2021-08-12 11:18 ` Matteo Croce 2021-08-12 11:18 ` Matteo Croce 2021-08-19 16:29 ` Marc Zyngier [this message] 2021-08-19 16:29 ` Marc Zyngier 2021-08-20 10:37 ` Matteo Croce 2021-08-20 10:37 ` Matteo Croce 2021-08-20 16:26 ` Marc Zyngier 2021-08-20 16:26 ` Marc Zyngier 2021-08-20 16:38 ` Matteo Croce 2021-08-20 16:38 ` Matteo Croce 2021-08-20 17:09 ` Marc Zyngier 2021-08-20 17:09 ` Marc Zyngier 2021-08-20 17:14 ` Matteo Croce 2021-08-20 17:14 ` Matteo Croce 2021-08-20 17:24 ` Marc Zyngier 2021-08-20 17:24 ` Marc Zyngier 2021-08-20 17:35 ` Matteo Croce 2021-08-20 17:35 ` Matteo Croce 2021-08-20 17:51 ` Marc Zyngier 2021-08-20 17:51 ` Marc Zyngier 2021-08-20 17:56 ` Matteo Croce 2021-08-20 17:56 ` Matteo Croce 2021-08-20 18:05 ` Matteo Croce 2021-08-20 18:05 ` Matteo Croce 2021-08-20 18:14 ` Marc Zyngier 2021-08-20 18:14 ` Marc Zyngier 2021-08-20 18:09 ` Marc Zyngier 2021-08-20 18:09 ` Marc Zyngier 2021-08-20 18:14 ` Matteo Croce 2021-08-20 18:14 ` Matteo Croce 2021-08-20 18:41 ` Marc Zyngier 2021-08-20 18:41 ` Marc Zyngier 2021-08-16 15:12 ` Jakub Kicinski 2021-08-16 15:12 ` Jakub Kicinski 2021-08-17 0:01 ` Matteo Croce 2021-08-17 0:01 ` Matteo Croce 2021-08-19 15:26 ` Marc Zyngier 2021-08-19 15:26 ` Marc Zyngier 2021-08-11 10:41 ` Thierry Reding 2021-08-11 10:41 ` Thierry Reding 2021-08-11 10:56 ` Joakim Zhang 2021-08-11 10:56 ` Joakim Zhang 2021-08-11 13:23 ` Marc Zyngier 2021-08-11 13:23 ` Marc Zyngier 2021-08-12 14:29 ` Thierry Reding 2021-08-12 14:29 ` Thierry Reding 2021-08-12 15:26 ` Marc Zyngier 2021-08-12 15:26 ` Marc Zyngier 2021-08-13 14:44 ` Thierry Reding 2021-08-13 14:44 ` Thierry Reding
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87wnohqty1.wl-maz@kernel.org \ --to=maz@kernel.org \ --cc=alexandre.torgue@foss.st.com \ --cc=davem@davemloft.net \ --cc=drew@beagleboard.org \ --cc=eric.dumazet@gmail.com \ --cc=jonathanh@nvidia.com \ --cc=kernel@esmil.dk \ --cc=kuba@kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-riscv@lists.infradead.org \ --cc=mcroce@linux.microsoft.com \ --cc=netdev@vger.kernel.org \ --cc=palmer@dabbelt.com \ --cc=paul.walmsley@sifive.com \ --cc=peppe.cavallaro@st.com \ --cc=thierry.reding@gmail.com \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.