netdev - Re: [GIT PULL] Networking for 6.0

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=wiwRtpyMVn1F9KT14H64tajiWsPnd0FfL5-BFnPOuFa_w@mail.gmail.com>
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