netdev - Re: [net-next PATCH] net: phy: Validate PHY LED OPs presence before registering

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: <20241003230357.z2czrn3pymhuywud@skbuf>
Date: Fri, 4 Oct 2024 02:03:57 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Christian Marangi <ansuelsmth@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [net-next PATCH] net: phy: Validate PHY LED OPs presence before
 registering

On Fri, Oct 04, 2024 at 12:33:17AM +0200, Christian Marangi wrote:
> On Fri, Oct 04, 2024 at 01:24:00AM +0300, Vladimir Oltean wrote:
> > On Fri, Oct 04, 2024 at 12:12:48AM +0200, Christian Marangi wrote:
> > > Validate PHY LED OPs presence before registering and parsing them.
> > > Defining LED nodes for a PHY driver that actually doesn't supports them
> > > is wrong and should be reported.
> > 
> > What about the case where a PHY driver gets LED support in the future?
> > Shouldn't the current kernel driver work with future device trees which
> > define LEDs, and just ignore that node, rather than fail to probe?
> 
> Well this just skip leds node parse and return 0, so no fail to probe.
> This just adds an error. Maybe I should use warn instead?
> 
> (The original idea was to return -EINVAL but it was suggested by Daniel
> that this was too much and a print was much better)

Ok, the "exit" label returns 0, not a probe failure, but as you say,
there's still the warning message printed to dmesg. What's its intended
value, exactly?

What would you do if you were working on a board which wasn't supported
in mainline but instead you only had the DTB for it, and you had to run
a git bisect back to when the driver didn't support parsing the PHY LED
nodes? What would you do, edit the DTB to add/remove the node at each
bisect step, so that the kernel gets what it understands in the device
tree and nothing more?

Why would the kernel even act so weird about it and print warnings or
return errors in the first place? Nobody could possibly develop anything
new with patches like this, without introducing some sort of mishap in
past kernels. Is there some larger context around this patch which I'm
missing?