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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sat, 30 Jul 2022 00:49:27 +0000 From: Vladimir Oltean <vladimir.oltean@....com> To: Marcin Wojtas <mw@...ihalf.com> CC: Florian Fainelli <f.fainelli@...il.com>, Andrew Lunn <andrew@...n.ch>, 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>, Vivien Didelot <vivien.didelot@...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>, Rob Herring <robh+dt@...nel.org>, Frank Rowand <frowand.list@...il.com>, Tomasz Nowicki <tn@...ihalf.com>, Grzegorz Jaszczyk <jaz@...ihalf.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 11:33:30PM +0200, Marcin Wojtas wrote: > Initially, I thought that the idea is a probe failure (hence the camps > to prevent that) - but it was clarified later, it's not the case. The idea _is_ a probe failure, but not for whom you seem to believe (not for the 'expected bad' drivers but for the 'expected good' ones). The probe failure is for drivers using OF whose compatibles are outside of dsa_switches_dont_enforce_validation, and all ACPI drivers (that's going to be your part), effective immediately. For example the sja1105 doesn't have any parasitic users, I'm absolutely sure about that because it already has validation, although only coincidentally. For the ocelot driver I'm also relatively certain practically speaking, because we're talking about an embedded switch where the description is in an SoC dtsi. But I can't totally exclude the possibility that somebody won't be crazy enough to /delete-property/ the fixed-link from a board device tree, or to redefine everything from scratch without including the SoC dtsi, and then claim that hey, this was actually working before this or that refactoring, due to some marginal condition which I never designed for. And that's the point of this patch. The DSA core is flawed because it doesn't let phylink fail when it should. It tries to save the day by making some odd decisions which make sense considering the old drivers, but simply don't, when you consider what the code would have looked like, were it not for the pre-phylink baggage. So this change essentially lets happen for new drivers what should have happened in a normal world where DSA would not interfere at all - nondescriptive bindings: fail to probe, bye. Rob is right, the DSA core shouldn't validate the OF node, it should just let phylink fail if that's what it will, and go about its day. Individual DSA drivers shouldn't have to validate their OF node either, but they'd have to, if they wanted to circumvent DSA's logic. And why put the burden on them? > I totally agree and I am all against breaking the backward > compatibility (this is why I work on ACPI support that much :) ). The > question is whether for existing deployments with 'broken' DT > description we would be ok to introduce a dev_warn/WARN_ON message > after a kernel update. Andrew suggested it. In v1, the array was for skipping DT validation (this implies skipping printing warnings) for drivers where we knew it's going to be violated anyway. But we thought, why not also tell users that what's going on isn't quite ideal. https://patchwork.kernel.org/project/netdevbpf/patch/20220723164635.1621911-1-vladimir.oltean@nxp.com/#24949229 This is a detail to me and not the purpose of the change. It would be nothing short of a miracle if people would suddenly see the light and update all DT blobs tomorrow. But no one here is planning based on that. Quite far from it - Russell King and Marek Behún were (are?) working on a patch set that actually pushes those incomplete DT blobs to the next level, and fakes what the proper OF node should have looked like, based on driver level knowledge. > That would be a case if the check is performed unconditionally - this > way we can keep compat strings out of net/dsa. What do you think? So you're saying keep the validation and the warnings, but don't fail the probing of anyone? Why? Being strict about validation allows me as a driver maintainer to be formally correct when I say that there are no users that expect to not register with phylink, rather than just guess or hope, or even waste time checking. And if I was someone new coming to the DSA framework, I'd want to have that guarantee. I didn't design for that possibility and I don't want to have it, just because I use the same framework as someone who put workarounds in place so that it works for him. We could argue about who of existing but otherwise newish drivers should be part of dsa_switches_dont_enforce_validation, and that's where the camps come in. Being warm and fuzzy about this, and just print, doesn't really describe anything except for a preference. After all, we're not abandoning the broken DT blobs. What's your concrete problem with compatible strings inside net/dsa anyway, and do you have a competent alternative? Is it the problem that you'll have to convert them to fwnode? You do realize that this array is not planned to expand at all after the patch is merged, including not to ACPI IDs, right? I've explained in the commit message why: | Because there is a pattern where newly added switches reuse existing | drivers more often than introducing new ones, I've opted for deciding | who gets to skip validation based on an OF compatible match table in the | DSA core. The alternative would have been to add another boolean | property to struct dsa_switch, like configure_vlan_while_not_filtering. | But this avoids situations where sometimes driver maintainers obfuscate | what goes on by sharing a common probing function, and therefore | making new switches inherit old quirks. The latter has actually happened no farther than a few weeks ago, refactoring was done to the microchip ksz driver, and a newly added switch gained support for an old one's quirks, by refactoring the code to share common logic (which I was otherwise very happy about). Now good luck with adding a compatible string to dsa_switches_dont_enforce_validation without anyone noticing.
Powered by blists - more mailing lists