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: Wed, 3 Aug 2022 16:35:56 -0700 From: Linus Torvalds <torvalds@...ux-foundation.org> To: Paolo Abeni <pabeni@...hat.com>, "Gustavo A. R. Silva" <gustavoars@...nel.org>, Wojciech Drewek <wojciech.drewek@...el.com>, Tony Nguyen <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
Powered by blists - more mailing lists