netdev - RE: [PATCH v2 net] net: dsa: microchip: provide a list of valid protocols for xmit handler

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, 7 Dec 2023 10:04:13 +0000
From: <Madhuri.Sripada@...rochip.com>
To: <olteanv@...il.com>, <sean@...nix.com>
CC: <Woojung.Huh@...rochip.com>, <UNGLinuxDriver@...rochip.com>,
	<andrew@...n.ch>, <f.fainelli@...il.com>, <davem@...emloft.net>,
	<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
	<Arun.Ramadoss@...rochip.com>, <ceggers@...i.de>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 net] net: dsa: microchip: provide a list of valid
 protocols for xmit handler

Vlad,

> -----Original Message-----
> From: Vladimir Oltean <olteanv@...il.com>
> Sent: Wednesday, December 6, 2023 11:32 PM
> To: Sean Nyekjaer <sean@...nix.com>
> Cc: Madhuri Sripada - I34878 <Madhuri.Sripada@...rochip.com>; Woojung
> Huh - C21699 <Woojung.Huh@...rochip.com>; UNGLinuxDriver
> <UNGLinuxDriver@...rochip.com>; andrew@...n.ch; f.fainelli@...il.com;
> davem@...emloft.net; edumazet@...gle.com; kuba@...nel.org;
> pabeni@...hat.com; Arun Ramadoss - I17769
> <Arun.Ramadoss@...rochip.com>; ceggers@...i.de;
> netdev@...r.kernel.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH v2 net] net: dsa: microchip: provide a list of valid
> protocols for xmit handler
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On Wed, Dec 06, 2023 at 06:45:12PM +0100, Sean Nyekjaer wrote:
> > > Don't just leave it there, also explain why.
> >
> > Message to me?
> >
> > /Sean
> 
> No, to Madhuri (as the To: field suggests).
> 
> In the Linux kernel it's not a good practice to put defensive checks which don't
> have a logical justification, because other people end up not knowing why
> they're there, and when they can be removed. Checking for the tagging
> protocol gives a very clear indication and traceability of why it is being done,
> on the other hand.
> 
> If the ds->tagger_data is NULL for a tagging protocol for which it was expected
> it shouldn't be, and the DSA core still decides to call
> ds->ops->connect_tag_protocol() anyway, this is a violation of the API
> contract established with all drivers that use this mechanism. Papering over a
> bug in the DSA core results in silent failures, which means that any further
> behavior is unpredictable. So I'd very much prefer the system to fail fast in case
> of a bug in the framework, so that it can be reported and fixed. With rigorous
> testing, it will fail earlier than in the production stage.
> 
> I only said "don't leave it there, also explain why" because I really don't
> appreciate review comments spreading FUD, for which I'd have to spend 20-
> 30 minutes to explain why leaving out the NULL pointer checking is, in fact,
> safe.
> 
> Of course, I am not excluding a not-yet-found bug either, but I am strongly
> encouraging Madhuri to walk through the code path and point it to us, and
> strongly discouraging lazy review comments. It's not fair for me to reply to a 5
> word sentence with a wall of text. So I replied with a phrase of comparable
> length to the suggestion.

I am new in this community and reviews. And was reviewing from code point of view where NULL check is a primary requirement and a general practice.
I understand the justification and will make a note of it in my further reviews and my kernel development as well.
Thanks for your inputs.

-Madhuri