lkml.org 
[lkml]   [2022]   [Aug]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [GIT PULL] Networking for 6.0
Date


> -----Original Message-----
> From: Linus Torvalds <torvalds@linux-foundation.org>
> Sent: czwartek, 4 sierpnia 2022 01:36
> To: Paolo Abeni <pabeni@redhat.com>; Gustavo A. R. Silva <gustavoars@kernel.org>; Drewek, Wojciech
> <wojciech.drewek@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Cc: kuba@kernel.org; davem@davemloft.net; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [GIT PULL] Networking for 6.0
>
> On Wed, Aug 3, 2022 at 3:15 AM Paolo Abeni <pabeni@redhat.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
\
 
 \ /
  Last update: 2022-08-04 11:44    [W:0.089 / U:0.508 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site