netdev - Re: [PATCH v2 net-next 4/4] net: dsa: validate that DT nodes of shared ports have the properties they need

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: <20220729170107.h4ariyl5rvhcrhq3@skbuf>
Date:   Fri, 29 Jul 2022 17:01:08 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Rob Herring <robh+dt@...nel.org>
CC:     netdev <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Oleksij Rempel <linux@...pel-privat.de>,
        Christian Marangi <ansuelsmth@...il.com>,
        John Crispin <john@...ozen.org>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Mans Rullgard <mans@...sr.com>,
        Arun Ramadoss <arun.ramadoss@...rochip.com>,
        Woojung Huh <woojung.huh@...rochip.com>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        George McCollister <george.mccollister@...il.com>,
        DENG Qingfang <dqfext@...il.com>,
        Sean Wang <sean.wang@...iatek.com>,
        Landen Chao <Landen.Chao@...iatek.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Hauke Mehrtens <hauke@...ke-m.de>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        Aleksander Jan Bajkowski <olek2@...pl>,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        Luiz Angelo Daros de Luca <luizluca@...il.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Pawel Dembicki <paweldembicki@...il.com>,
        Clément Léger <clement.leger@...tlin.com>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Russell King <rmk+kernel@...linux.org.uk>,
        Marek Behún <kabel@...nel.org>,
        Marcin Wojtas <mw@...ihalf.com>,
        Frank Rowand <frowand.list@...il.com>
Subject: Re: [PATCH v2 net-next 4/4] net: dsa: validate that DT nodes of
 shared ports have the properties they need

On Fri, Jul 29, 2022 at 10:22:49AM -0600, Rob Herring wrote:
> It's not the kernel's job to validate the DT. If it was, it does a
> horrible job.

I'm surprised by you saying this.

The situation is as follows: phylink parses the fwnode it's given, and
errors out if it can't find everything it needs. See phylink_parse_mode()
and phylink_parse_fixedlink(). This is a matter of fact - if you start
parsing stuff, you'll eventually need to treat the case where what
you're searching for isn't there, or isn't realistic.

DSA is a common framework used by multiple drivers, and it wasn't always
integrated with phylink. The DT nodes of some ports will lack what
phylink needs, but these ports don't really need phylink to work, it's
optional, they work without it too. However if we begin the process of
registering them with phylink and we let phylink fail, this process is
irreversible and the ports don't work anymore.

So what DSA currently does (even before this patch set) is it
pre-validates that phylink has what it needs, and skips phylink if it
doesn't. It's only that it doesn't name it this way, and it doesn't
print anything.

Being a common framework, new drivers opt into this behavior willy-nilly.
I am adding a table of compatible strings of old drivers where the
behavior is retained. For new drivers, we fail them in DSA rather than
in phylink, this is true. Maybe this is what you disagree with?
We do this as a matter of practicality - we already need to predetermine
whether phylink has a chance of working, and if we find something missing,
we know it won't. Seems illogical to let phylink go through the same
parsing again.

As for the lousy job, I can't contradict you...

> Is the schema providing this validation? If not, you need to add it.

No, it's not. I can also look into providing a patch that statically
validates this. But I'm afraid, with all due respect, that not many
people take the YAML validator too seriously? With the volume of output
it even throws, it would be even hard to detect something new, you'd
need to know to search for it. Most of the DSA drivers aren't even
converted to YAML, and it is precisely the biggest offenders that
aren't. And even if the schema says a property is required but the code
begs to differ, and a future DT blob gets to enter production based on
undocumented behavior, who's right?