From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 52DB0C4338F for ; Thu, 19 Aug 2021 16:29:54 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 1B428610A5 for ; Thu, 19 Aug 2021 16:29:54 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 1B428610A5 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Subject:Cc:To:From:Message-ID:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=jSZKGSjnnXzez4fOxr5/fn7kT/Z7BLrzhE0+WLNdr3w=; b=F8YK32eAoMiodk uqbjtNNckwx6eO53r4AsKc+1Aook5uZ3ELqz3rUcfZR0HeXvck88gTfsnmJsFI+6yg+28OnB6/pLB bZXBM1T1+UguhfMw/mNLNClsgWDeox64MKWGLErz7gxob17pVDXJEIYYMEQK9yF88XepfXXIeDuIc ThVUfr0O90ejT8T3O9BqmSyvhM8PH9WSPAF+aBKVHZeEE/wZfWIhklUBu1NW+qL0sTHbGTXo7Xn3Q P48qYrJE7tZ9x7wHKfxA+oEliNTY//daXiAtyycGhozsEKjy41j8hjALFYbp2M3EZw8YYLxtA1u1B RwcFa4DZFUZ7A3JyoqkQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mGkv5-008xmv-Fc; Thu, 19 Aug 2021 16:29:31 +0000 Received: from mail.kernel.org ([198.145.29.99]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mGkv3-008xmK-E7 for linux-riscv@lists.infradead.org; Thu, 19 Aug 2021 16:29:30 +0000 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 04C6661106; Thu, 19 Aug 2021 16:29:29 +0000 (UTC) Received: from sofa.misterjones.org ([185.219.108.64] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1mGkv1-00619Z-1o; Thu, 19 Aug 2021 17:29:27 +0100 Date: Thu, 19 Aug 2021 17:29:26 +0100 Message-ID: <87wnohqty1.wl-maz@kernel.org> From: Marc Zyngier To: Matteo Croce Cc: Eric Dumazet , Thierry Reding , netdev@vger.kernel.org, Linux Kernel Mailing List , linux-riscv , Giuseppe Cavallaro , Alexandre Torgue , "David S. Miller" , Jakub Kicinski , Palmer Dabbelt , Paul Walmsley , Drew Fustini , Emil Renner Berthing , Jon Hunter , Will Deacon Subject: Re: [PATCH net-next] stmmac: align RX buffers In-Reply-To: References: <20210614022504.24458-1-mcroce@linux.microsoft.com> <871r71azjw.wl-maz@kernel.org> <202417ef-f8ae-895d-4d07-1f9f3d89b4a4@gmail.com> <87o8a49idp.wl-maz@kernel.org> <20210812121835.405d2e37@linux.microsoft.com> <874kbuapod.wl-maz@kernel.org> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: mcroce@linux.microsoft.com, eric.dumazet@gmail.com, thierry.reding@gmail.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org, peppe.cavallaro@st.com, alexandre.torgue@foss.st.com, davem@davemloft.net, kuba@kernel.org, palmer@dabbelt.com, paul.walmsley@sifive.com, drew@beagleboard.org, kernel@esmil.dk, jonathanh@nvidia.com, will@kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210819_092929_555088_DC9AC8EE X-CRM114-Status: GOOD ( 46.52 ) X-BeenThere: linux-riscv@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-riscv" Errors-To: linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org On Thu, 12 Aug 2021 12:18:48 +0100, Matteo Croce wrote: > > [1 ] > On Thu, Aug 12, 2021 at 1:05 PM Marc Zyngier wrote: > > > > On Thu, 12 Aug 2021 11:18:35 +0100, > > Matteo Croce wrote: > > > > > > On Thu, 12 Aug 2021 10:48:03 +0200 > > > Eric Dumazet wrote: > > > > > > > > > > > > > > > On 8/11/21 4:16 PM, Marc Zyngier wrote: > > > > > On Wed, 11 Aug 2021 13:53:59 +0100, > > > > > Eric Dumazet 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