netdev - Re: [PATCH net-next 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJq09z5KEFUjpKuyPu0AL6pDessi8+1cjV454AgGoMyCYVw+ng@mail.gmail.com>
Date:   Fri, 18 Feb 2022 02:29:06 -0300
From:   Luiz Angelo Daros de Luca <luizluca@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>, Andrew Lunn <andrew@...n.ch>
Cc:     "open list:NETWORKING DRIVERS" <netdev@...r.kernel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        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 1/2] net: dsa: tag_rtl8_4: add rtl8_4t tailing variant

> Re: title. Tail or trailing?

I guess I'll stick with trailing. It looks like the most used term.

>
> On Wed, Feb 09, 2022 at 06:13:11PM -0300, Luiz Angelo Daros de Luca wrote:
> > +static inline void rtl8_4_write_tag(struct sk_buff *skb, struct net_device *dev,
> > +                                 char *tag)
> >  {
> >       struct dsa_port *dp = dsa_slave_to_port(dev);
> > -     __be16 *tag;
> > -
> > -     skb_push(skb, RTL8_4_TAG_LEN);
> > -
> > -     dsa_alloc_etype_header(skb, RTL8_4_TAG_LEN);
> > -     tag = dsa_etype_header_pos_tx(skb);
> > +     __be16 *tag16 = (__be16 *)tag;
>
> Can the tail tag be aligned to an odd offset? In that case, should you
> access byte by byte, maybe? I'm not sure how arches handle this.

I see. I'm not sure my big endian MIPS device is able to test it
properly. I tried a wide range of payload sizes without any issue. But
I believe we need to play safe here.

Andrew Lunn, you suggested using get_unaligned_be16(). However, as
using get_unaligned_be16() I will already save the tag (or at least
part of it) in the stack, could it be a memcpy from stack to the
buffer (and the opposite while reading the tag)? It will be less
intervention than rewriting the code to deal with each byte at a time.
I'm not sure if I need to or can help the compiler optimize it. In my
MIPS, it seems it did a good job, replacing the memcpy with four
swl/swr instructions (used to write unaligned 32-bit values).

> >
> >       /* Set Realtek EtherType */
> > -     tag[0] = htons(ETH_P_REALTEK);
> > +     tag16[0] = htons(ETH_P_REALTEK);
> >
> >       /* Set Protocol; zero REASON */
> > -     tag[1] = htons(FIELD_PREP(RTL8_4_PROTOCOL, RTL8_4_PROTOCOL_RTL8365MB));
> > +     tag16[1] = htons(FIELD_PREP(RTL8_4_PROTOCOL, RTL8_4_PROTOCOL_RTL8365MB));
> >
> >       /* Zero FID_EN, FID, PRI_EN, PRI, KEEP; set LEARN_DIS */
> > -     tag[2] = htons(FIELD_PREP(RTL8_4_LEARN_DIS, 1));
> > +     tag16[2] = htons(FIELD_PREP(RTL8_4_LEARN_DIS, 1));
> >
> >       /* Zero ALLOW; set RX (CPU->switch) forwarding port mask */
> > -     tag[3] = htons(FIELD_PREP(RTL8_4_RX, BIT(dp->index)));
> > +     tag16[3] = htons(FIELD_PREP(RTL8_4_RX, BIT(dp->index)));
> > +}
> > +
> > +static struct sk_buff *rtl8_4_tag_xmit(struct sk_buff *skb,
> > +                                    struct net_device *dev)
> > +{
> > +     skb_push(skb, RTL8_4_TAG_LEN);
> > +
> > +     dsa_alloc_etype_header(skb, RTL8_4_TAG_LEN);
> > +
> > +     rtl8_4_write_tag(skb, dev, dsa_etype_header_pos_tx(skb));
> >
> >       return skb;
> >  }
> >
> > -static struct sk_buff *rtl8_4_tag_rcv(struct sk_buff *skb,
> > -                                   struct net_device *dev)
> > +static struct sk_buff *rtl8_4t_tag_xmit(struct sk_buff *skb,
> > +                                     struct net_device *dev)
> > +{
>
> Why don't you want to add:
>
>         if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb))
>                 return NULL;
>
> and then you'll make this tagging protocol useful in production too.

It works like a charm! Thanks! And now I have a pretty good use for
this new tag: force checksum in software. Whenever the cpu ethernet
driver cannot correctly deal with the checksum offloading, switch to
rtl8_4t and be happy. It will work either adding 'dsa-tag-protocol =
"rtl8_4t";' to the CPU port in the device-tree file or even changing
the tag at runtime.

Now checksum will only break if you stack two trailing tags and the
first one added does not calculate the checksum and the second one is
"rtl8_4t". When "rtl8_4t" does calculate the checksum, it will include
the other tag in the sum. But it might even be considered a bug in the
first tagger.

Regards,

Luiz