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