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
| ||
|
Message-ID: <DM6PR11MB41245D212313D53CFB793BE8E18BA@DM6PR11MB4124.namprd11.prod.outlook.com> 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
Powered by blists - more mailing lists