linux-kernel - Re: [PATCH] Revert "arm64: dts: ls1028a: sl28: use ocelot-8021q tagging by default"

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:   Thu, 27 Oct 2022 12:27:28 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Michael Walle <michael@...le.cc>
CC:     Shawn Guo <shawnguo@...nel.org>, Leo Li <leoyang.li@....com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Heiko Thiery <heiko.thiery@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH] Revert "arm64: dts: ls1028a: sl28: use ocelot-8021q
 tagging by default"

On Thu, Oct 27, 2022 at 03:05:19PM +0300, Vladimir Oltean wrote:
> On Thu, Oct 27, 2022 at 01:32:48PM +0200, Michael Walle wrote:
> > This reverts commit be0b178c50c37a666d54f435da71cf9f008362a0.
> > 
> > This commit will break networking on the sl28 boards if the tagger is
> > not compiled into the kernel. If a non-default tagger is used, the
> > kernel doesn't do a request_module(). Fixing that is also not that
> > trivial because the tagger modules are loaded by ids, not by name.
> > Thus for now, just revert to the default tagger until that is fixed.
> > 
> > Fixes: be0b178c50c3 ("arm64: dts: ls1028a: sl28: use ocelot-8021q tagging by default")
> > Reported-by: Heiko Thiery <heiko.thiery@...il.com>
> > Signed-off-by: Michael Walle <michael@...le.cc>
> > ---
> > Vladimir, I'm not sure how to fix that one. Adding aliases to the tagger
> > modules? Something like "MODULE_ALIAS("dsa_tag-ocelot-8021q");" and then do
> > a request_module() in dsa_find_tagger_by_name(), too?
> > 
> >  .../arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28.dts | 8 --------
> >  1 file changed, 8 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28.dts
> > index 72429b37a8b4..771c50c7f50a 100644
> > --- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28.dts
> > +++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28.dts
> > @@ -324,14 +324,6 @@ &lpuart1 {
> >  	status = "okay";
> >  };
> >  
> > -&mscc_felix_port4 {
> > -	dsa-tag-protocol = "ocelot-8021q";
> > -};
> > -
> > -&mscc_felix_port5 {
> > -	dsa-tag-protocol = "ocelot-8021q";
> > -};
> > -
> >  &usb0 {
> >  	status = "okay";
> >  };
> > -- 
> > 2.30.2
> >
> 
> Pretty nasty. Of all the switch drivers that support tagging protocol
> change, Ocelot/Felix is the only one with this bug, because in all other
> cases, the default and the alternative tagging protocol are part of the
> same .ko. Only here we have tag_ocelot.ko and tag_ocelot_8021q.ko.
> 
> The problem preventing us from calling request_module() is that currently,
> the string identifying the tagging protocol (to which we match device
> tree information) is part of the tag_*.ko. I think we'd need the
> translation table between string and enum dsa_tag_protocol to be part of
> dsa_core.ko.

I think we should treat what we committed to in terms of dt-bindings
with utmost respect, so I would consider your proposed revert as the
absolute last option. Reverting a device tree change doesn't mean that
the device trees without the revert will disappear from circulation.

So far we have 3 options for fixing this within the kernel

- make tag_ocelot.o and tag_ocelot_8021q.o link into the same
  tag_ocelot.ko

- change the MODULE_ALIAS() of all tagging protocol driver modules from
  "dsa_tag-<number" to something containing their string name - what you
  proposed. I don't know why the current MODULE_ALIAS() is formatted the
  way it is. Maybe Andrew can comment on whether this is feasible.
  I think there isn't any backwards compatibility concern, since only
  modules compiled for a certain kernel version are expected to be
  loaded.

- put a translation table between string and MODULE_ALIAS() inside
  dsa_core.ko, which potentially duplicates code. Maybe if we
  auto-generate it somehow?