netdev - Re: [PATCH net-next v3 1/2] net: dsa: tag_rtl8_4: add rtl8

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] [day] [month] [year] [list]

Date:   Fri, 25 Feb 2022 03:10:46 -0300
From:   Luiz Angelo Daros de Luca <luizluca@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     "open list:NETWORKING DRIVERS" <netdev@...r.kernel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        Arınç ÜNAL <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next v3 1/2] net: dsa: tag_rtl8_4: add rtl8_4t
 trailing variant

Em qua., 23 de fev. de 2022 às 21:04, Vladimir Oltean
<olteanv@...il.com> escreveu:
>
> On Tue, Feb 22, 2022 at 07:47:57PM -0300, Luiz Angelo Daros de Luca wrote:
> > diff --git a/net/dsa/tag_rtl8_4.c b/net/dsa/tag_rtl8_4.c
> > index 02686ad4045d..2e81ab49d928 100644
> > --- a/net/dsa/tag_rtl8_4.c
> > +++ b/net/dsa/tag_rtl8_4.c
> > @@ -9,11 +9,6 @@
> >   *
> >   * This tag header has the following format:
> >   *
> > - *  -------------------------------------------
> > - *  | MAC DA | MAC SA | 8 byte tag | Type | ...
> > - *  -------------------------------------------
> > - *     _______________/            \______________________________________
> > - *    /                                                                   \
> >   *  0                                  7|8                                 15
> >   *  |-----------------------------------+-----------------------------------|---
> >   *  |                               (16-bit)                                | ^
> > @@ -58,6 +53,28 @@
> >   *    TX/RX      | TX (switch->CPU): port number the packet was received on
> >   *               | RX (CPU->switch): forwarding port mask (if ALLOW=0)
> >   *               |                   allowance port mask (if ALLOW=1)
> > + *
> > + * The tag can be positioned before Ethertype, using tag "rtl8_4":
> > + *
> > + *  +--------+--------+------------+------+-----
> > + *  | MAC DA | MAC SA | 8 byte tag | Type | ...
> > + *  +--------+--------+------------+------+-----
> > + *
> > + * If checksum offload is enabled for CPU port device, it might break if the
> > + * driver does not use csum_start/csum_offset.
>
> Please. This is true of any DSA header. If you feel you have something
> to add on this topic please do so in Documentation/networking/dsa/dsa.rst
> under "Switch tagging protocols".

OK. I'll remove it from this series and add that info to docs in an
independent commit.

> Also, s/CPU port device/DSA master/.
>
> > + *
> > + * The tag can also appear between the end of the payload and before the CRC,
> > + * using tag "rtl8_4t":
> > + *
> > + * +--------+--------+------+-----+---------+------------+-----+
> > + * | MAC DA | MAC SA | TYPE | ... | payload | 8-byte tag | CRC |
> > + * +--------+--------+------+-----+---------+------------+-----+
> > + *
> > + * The added bytes after the payload will break most checksums, either in
> > + * software or hardware. To avoid this issue, if the checksum is still pending,
> > + * this tagger checksum the packet before adding the tag, rendering any
>
> s/checksum/checksums/
>
> > + * checksum offload useless.
>
> If you're adding a tail tagging driver to work around checksum offload
> issues, this solution is about as bad as it gets. You're literally not
> gaining anything in performance over fixing your DSA master driver to
> turn off checksum offloading for unrecognized DSA tagging protocols.

I wasn't adding it as a way to disable offload but as an alternative
to keep the hardware offload enabled. However, in the end, if the HW
already does not understand the tag, adding a tag after the payload
will not only break checksum offload but the software checksum as
well.

> And on top of that, you're requiring your users to be aware of this
> issue and make changes to their configuration, for something that can be
> done automatically.

I don't see how we could automatically detect that in an unspecific
Ethernet driver. No driver expects to have some bytes after the
payload it should ignore. Not even the software checksum functions
consider that case. And we should not adapt all Ethernet drivers for
DSA. The easier solution is to calculate the checksum before the tag
was added, even if that prevents checksum offload for a possible
compatible HW. After the tag was added, it is already too late for the
driver to make a decision (with existing Linux code).

> Do you have another use case as well?

Allowing the user to change the tag position is a nice tool to detect
compatible problems. I got the checksum offload but it could also help
with stacking incompatible switches, when the switch does not like the
added Ethertype DSA tag.

Regards,

Luiz