netdev - Re: [PATCH net v2] net: dsa: tag_rtl4

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]

Date: Mon, 30 Oct 2023 17:30:57 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Andrew Lunn <andrew@...n.ch>, Florian Fainelli <f.fainelli@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net v2] net: dsa: tag_rtl4_a: Bump min packet size

On Mon, Oct 30, 2023 at 05:23:25PM +0200, Vladimir Oltean wrote:
> On Mon, Oct 30, 2023 at 03:37:24PM +0100, Linus Walleij wrote:
> > On Mon, Oct 30, 2023 at 3:16 PM Vladimir Oltean <olteanv@...il.com> wrote:
> > 
> > > Could you please try to revert the effect of commit 339133f6c318 ("net:
> > > dsa: tag_rtl4_a: Drop bit 9 from egress frames") by setting that bit in
> > > the egress tag again? Who knows, maybe it is the bit which tells the
> > > switch to bypass the forwarding process.
> > 
> > I have already tried that, it was one of the first things I tried,
> > just looking over the git log and looking for usual suspects.
> > 
> > Sadly it has no effect whatsoever, the problem persists :(
> > 
> > > Or maybe there is a bit in a
> > > different position which does this. You could try to fill in all bits in
> > > unknown positions, check that there are no regressions with packet sizes
> > > < 1496, and then see if that made any changes to packet sizes >= 1496.
> > > If it did, try to see which bit made the difference.
> > 
> > Hehe now we're talking :D
> > 
> > I did something similar before, I think just switching a different bit
> > every 10 packets or so and running a persistent ping until it succeeds
> > or not.
> > 
> > I'll see what I can come up with.
> > 
> > Yours,
> > Linus Walleij
> 
> And the drop reason in ethtool -S also stays unchanged despite all the
> extra bits set in the tag? It still behaves as if the packet goes
> through the forwarding path?

Could you please place these skb_dump() calls before and after the magic
__skb_put_padto() call, for us to see if anything unexpected changes?
Maybe the socket buffers have some unusual geometry which the conduit
interface doesn't like, for some reason.

The number of skb dumps that you provide back should be even, and
ideally the first one should be the unaltered packet, to avoid confusion :)

diff --git a/net/dsa/tag_rtl4_a.c b/net/dsa/tag_rtl4_a.c
index 25238093686c..2ca189b5125e 100644
--- a/net/dsa/tag_rtl4_a.c
+++ b/net/dsa/tag_rtl4_a.c
@@ -41,18 +41,22 @@ static struct sk_buff *rtl4a_tag_xmit(struct sk_buff *skb,
 	u8 *tag;
 	u16 out;
 
-	/* Pad out to at least 60 bytes */
-	if (unlikely(__skb_put_padto(skb, ETH_ZLEN, false)))
-		return NULL;
-
 	/* Packets over 1496 bytes get dropped unless they get padded
 	 * out to 1518 bytes. 1496 is ETH_DATA_LEN - tag which is hardly
 	 * a coinicidence, and 1518 is ETH_FRAME_LEN + FCS so we define
 	 * the threshold size and padding like this.
 	 */
 	if (skb->len >= (ETH_DATA_LEN - RTL4_A_HDR_LEN)) {
+		skb_dump(KERN_ERR, skb, true);
+
 		if (unlikely(__skb_put_padto(skb, ETH_FRAME_LEN + ETH_FCS_LEN, false)))
 			return NULL;
+
+		skb_dump(KERN_ERR, skb, true);
+	} else {
+		/* Pad out to at least 60 bytes */
+		if (unlikely(__skb_put_padto(skb, ETH_ZLEN, false)))
+			return NULL;
 	}
 
 	netdev_dbg(dev, "add realtek tag to package to port %d\n",
-- 
2.34.1