netdev - Re: [PATCH v2 net-next 09/12] net: dsa: tag_ocelot: create separate tagger for Seville

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:   Sat, 13 Feb 2021 17:29:45 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Richard Cochran <richardcochran@...il.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Maxim Kochetkov <fido_max@...ox.ru>,
        UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH v2 net-next 09/12] net: dsa: tag_ocelot: create separate
 tagger for Seville



On 2/13/2021 14:37, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@....com>
> 
> The ocelot tagger is a hot mess currently, it relies on memory
> initialized by the attached driver for basic frame transmission.
> This is against all that DSA tagging protocols stand for, which is that
> the transmission and reception of a DSA-tagged frame, the data path,
> should be independent from the switch control path, because the tag
> protocol is in principle hot-pluggable and reusable across switches
> (even if in practice it wasn't until very recently). But if another
> driver like dsa_loop wants to make use of tag_ocelot, it couldn't.
> 
> This was done to have common code between Felix and Ocelot, which have
> one bit difference in the frame header format. Quoting from commit
> 67c2404922c2 ("net: dsa: felix: create a template for the DSA tags on
> xmit"):
> 
>      Other alternatives have been analyzed, such as:
>      - Create a separate tag_seville.c: too much code duplication for just 1
>        bit field difference.
>      - Create a separate DSA_TAG_PROTO_SEVILLE under tag_ocelot.c, just like
>        tag_brcm.c, which would have a separate .xmit function. Again, too
>        much code duplication for just 1 bit field difference.
>      - Allocate the template from the init function of the tag_ocelot.c
>        module, instead of from the driver: couldn't figure out a method of
>        accessing the correct port template corresponding to the correct
>        tagger in the .xmit function.
> 
> The really interesting part is that Seville should have had its own
> tagging protocol defined - it is not compatible on the wire with Ocelot,
> even for that single bit. In principle, a packet generated by
> DSA_TAG_PROTO_OCELOT when booted on NXP LS1028A would look in a certain
> way, but when booted on NXP T1040 it would look differently. The reverse
> is also true: a packet generated by a Seville switch would be
> interpreted incorrectly by Wireshark if it was told it was generated by
> an Ocelot switch.
> 
> Actually things are a bit more nuanced. If we concentrate only on the
> DSA tag, what I said above is true, but Ocelot/Seville also support an
> optional DSA tag prefix, which can be short or long, and it is possible
> to distinguish the two taggers based on an integer constant put in that
> prefix. Nonetheless, creating a separate tagger is still justified,
> since the tag prefix is optional, and without it, there is again no way
> to distinguish.
> 
> Claiming backwards binary compatibility is a bit more tough, since I've
> already changed the format of tag_ocelot once, in commit 5124197ce58b
> ("net: dsa: tag_ocelot: use a short prefix on both ingress and egress").
> Therefore I am not very concerned with treating this as a bugfix and
> backporting it to stable kernels (which would be another mess due to the
> fact that there would be lots of conflicts with the other DSA_TAG_PROTO*
> definitions). It's just simpler to say that the string values of the
> taggers have ABI value starting with kernel 5.12, which will be when the
> changing of tag protocol via /sys/class/net/<dsa-master>/dsa/tagging
> goes live.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>

Reviewed-by: Florian Fainelli <f.fainelli@...il.com>
-- 
Florian