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
| ||
|
Date: Thu, 4 Aug 2022 09:43:02 +0000 From: "Drewek, Wojciech" <wojciech.drewek@...el.com> To: "Torvalds, Linus" <torvalds@...ux-foundation.org>, Paolo Abeni <pabeni@...hat.com>, "Gustavo A. R. Silva" <gustavoars@...nel.org>, "Nguyen, Anthony L" <anthony.l.nguyen@...el.com> CC: "kuba@...nel.org" <kuba@...nel.org>, "davem@...emloft.net" <davem@...emloft.net>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org> Subject: RE: [GIT PULL] Networking for 6.0 > -----Original Message----- > From: Linus Torvalds <torvalds@...ux-foundation.org> > Sent: czwartek, 4 sierpnia 2022 01:36 > To: Paolo Abeni <pabeni@...hat.com>; Gustavo A. R. Silva <gustavoars@...nel.org>; Drewek, Wojciech > <wojciech.drewek@...el.com>; Nguyen, Anthony L <anthony.l.nguyen@...el.com> > Cc: kuba@...nel.org; davem@...emloft.net; netdev@...r.kernel.org; linux-kernel@...r.kernel.org > Subject: Re: [GIT PULL] Networking for 6.0 > > On Wed, Aug 3, 2022 at 3:15 AM Paolo Abeni <pabeni@...hat.com> wrote: > > > > At the time of writing we have two known conflicts, one with arm-soc: > > Hmm. There's actually a third one, this one semantic (but mostly > harmless). I wonder how it was overlooked. > > It causes an odd gcc "note" report: > > net/core/flow_dissector.c: In function ‘is_pppoe_ses_hdr_valid’: > net/core/flow_dissector.c:898:13: note: the ABI of passing struct > with a flexible array member has changed in GCC 4.4 > 898 | static bool is_pppoe_ses_hdr_valid(struct pppoe_hdr hdr) > | ^~~~~~~~~~~~~~~~~~~~~~ > > and it looks like a semantic merge conflict between commits > > 94dfc73e7cf4 ("treewide: uapi: Replace zero-length arrays with > flexible-array members") > 46126db9c861 ("flow_dissector: Add PPPoE dissectors") > > where that first commit makes 'struct pppoe_hdr' have a flexible array > member at the end, and the second second commit passes said pppoe_hdr > by value as an argument. > > I don't think there is any reason to pass that 'struct pppoe_hdr' by > value in the first place, and that is not a normal pattern for the > kernel. Sure, we sometimes do use opaque types that may be structures > (eg 'pte_t') by value as arguments, but that is not how that code is > written. > > Any sane compiler will inline that thing anyway, so the end result > ends up being the same, but passing a structure with an array at the > end (whether zero-sized or flexible) by value is just cray-cray, to > use the technical term. > > So I resolved this semantic conflict by simply making that function > take a 'const struct pppoe_hdr *hdr' argument instead. That's the > proper way. > > Why was this not noticed in linux-next? Is it because nobody actually > *looks* at the output? Because it's a "note" and not a "warning", it > ends up not aborting the build, but I do think the compiler is > pointing out a very real issue. > > It would be perhaps worthwhile looking at code that passes structures > by value as arguments (or as return values). It can generate truly > horrendously bad code, and even when said structures are small, it's > uisually not the right thing to do. > > And yes, as noted, we sometimes do have that pattern very much on > purpose, sometimes because of abstraction reasons (pte_t) and > sometimes because we explicitly want the magic "two words of result" > ('struct fd' and fdget()). > > So it's not a strict no-no, but it's not generally a good idea unless > you have a very good reason for it (and it's particularly not a good > idea when there's an array at the end). > > I've fixed this up in my tree, and it's all fine (and while I'm not > *happy* with the fact that apparently nobody looks at linux-next > output, I guess it is what it is). > > Linus Thanks for fixing that. I'll pay more attention in the future when passing structures by value. Wojtek
Powered by blists - more mailing lists