linux-kernel - Re: [PATCH] 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]

Message-ID: <20231030125127.xccqwct3jgg6o2rl@skbuf>
Date:   Mon, 30 Oct 2023 14:51:27 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        "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] dsa: tag_rtl4_a: Bump min packet size

On Sun, Oct 29, 2023 at 04:38:04PM +0100, Linus Walleij wrote:
> On Fri, Oct 27, 2023 at 11:23 PM Florian Fainelli <f.fainelli@...il.com> wrote:
> 
> > > It turns out that sometimes, but not always, small packets are
> > > dropped by the switch for no reason.
> >
> > And we are positive that the Ethernet MAC is also properly padding
> > frames before having them ingress the switch?
> 
> I don't fully follow, this code is the one adding the padding isn't it?
> Then the result is transmitted to the switch from the ethernet
> MAC (drivers/net/ethernet/cortina/gemini.c).
> 
> What am I getting wrong here...

What you are missing is that the existing padding done by rtl4a_tag_xmit()
shouldn't be normally needed except for exceptional cases.

Socket buffers smaller than ETH_ZLEN can be passed to any network
device, and it is expected that either the driver or the hardware pads
up to ETH_ZLEN automatically. Thus, the conduit driver should already
know that it needs to pad packets to ETH_ZLEN.

The exceptional cases are:
- This is a tail tag (not the case here), which by definition needs to
  be located at the end of the skb. If you first put the tag then let
  the conduit interface pad, then the tail tag is no longer at the tail.
  So in that case, DSA pads first in generic code - dsa_user_xmit().
- The switch must handle the case where, after stripping the DSA tag
  from a ETH_ZLEN sized packet coming from the CPU port, it re-pads the
  packet on user port egress. Some switches don't handle that properly,
  and thus, we have isolated __skb_put_padto() calls within certain
  tagging protocols which address just that case.

So, what Florian was asking is whether the conduit interface is not
doing its expected job properly. You clarified that the problem is big
rather than small packets, but we still need an explanation for the
existing __skb_put_padto() call, given that it seems that it was placed
there due to a misunderstanding rather than due to an explicit need for
an exceptional case.