Message-ID: <20190521220020.GA13769@altlinux.org>
Date:   Wed, 22 May 2019 01:00:20 +0300
From:   "Dmitry V. Levin" <ldv@...linux.org>
To:     Baruch Siach <baruch@...s.co.il>,
        Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Linux-Arch <linux-arch@...r.kernel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-m68k <linux-m68k@...ts.linux-m68k.org>,
        strace-devel@...ts.strace.io
Subject: Re: strace for m68k bpf_prog_info mismatch

Hi Baruch, Geert,

Could you share these findings with bpf and netdev people, please?

On Fri, May 03, 2019 at 02:16:04PM +0200, Geert Uytterhoeven wrote:
> Hi Baruch,
> 
> On Fri, May 3, 2019 at 1:52 PM Baruch Siach <baruch@...s.co.il> wrote:
> > On Fri, May 03 2019, Geert Uytterhoeven wrote:
> > > On Fri, May 3, 2019 at 6:06 AM Baruch Siach <baruch@...s.co.il> wrote:
> > >> strace 5.0 fails to build for m86k/5208 with the Buildroot generated
> > >> toolchain:
> > >>
> > >> In file included from bpf_attr_check.c:6:0:
> > >> static_assert.h:20:25: error: static assertion failed: "bpf_prog_info_struct.nr_jited_ksyms offset mismatch"
> > >>  #  define static_assert _Static_assert
> > >>                          ^
> > >> bpf_attr_check.c:913:2: note: in expansion of macro ‘static_assert’
> > >>   static_assert(offsetof(struct bpf_prog_info_struct, nr_jited_ksyms) == offsetof(struct bpf_prog_info, nr_jited_ksyms),
> > >>   ^~~~~~~~~~~~~
> > >>
> > >> The direct cause is a difference in the hole after the gpl_compatible
> > >> field. Here is pahole output for the kernel struct (from v4.19):
> > >>
> > >> struct bpf_prog_info {
> > >>         ...
> > >>         __u32                      ifindex;              /*    80     4 */
> > >>         __u32                      gpl_compatible:1;     /*    84: 0  4 */
> > >>
> > >>         /* XXX 15 bits hole, try to pack */
> > >>         /* Bitfield combined with next fields */
> > >>
> > >>         __u64                      netns_dev;            /*    86     8 */
> > >
> > > I guess that should be "__aligned_u64 netns_dev;", to not rely on
> > > implicit alignment.
> >
> > Thanks. I can confirm that this minimal change fixes strace build:
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 929c8e537a14..709d4dddc229 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -2869,7 +2869,7 @@ struct bpf_prog_info {
> >         char name[BPF_OBJ_NAME_LEN];
> >         __u32 ifindex;
> >         __u32 gpl_compatible:1;
> > -       __u64 netns_dev;
> > +       __aligned_u64 netns_dev;
> >         __u64 netns_ino;
> >         __u32 nr_jited_ksyms;
> >         __u32 nr_jited_func_lens;
> >
> > Won't that break ABI compatibility for affected architectures?
> 
> Yes it will. Or it may have been unusable without the fix. I don't know
> for sure.
> 
> > >> And this is for the strace struct:
> > >>
> > >> struct bpf_prog_info_struct {
> > >>         ...
> > >>         uint32_t                   ifindex;              /*    80     4 */
> > >>         uint32_t                   gpl_compatible:1;     /*    84: 0  4 */
> > >>
> > >>         /* XXX 31 bits hole, try to pack */
> > >
> > > How come the uint64_t below is 8-byte aligned, not 2-byte aligned?
> > > Does strace use a special definition of uint64_t?
> >
> > I guess this is because of the netns_dev field definition in struct
> > bpf_prog_info_struct at bpf_attr.h:
> >
> > struct bpf_prog_info_struct {
> >        ...
> >         uint32_t gpl_compatible:1;
> >         /*
> >          * The kernel UAPI is broken by Linux commit
> >          * v4.16-rc1~123^2~227^2~5^2~2 .
> >          */
> >         uint64_t ATTRIBUTE_ALIGNED(8) netns_dev; /* skip check */
> 
> Oh, the bug was even documented, with its cause ;-)
> That's commit 675fc275a3a2d905 ("bpf: offload: report device information
> for offloaded programs").
> 
> Partially fixed by commit 36f9814a494a874d ("bpf: fix uapi hole for 32 bit
> compat applications"), which left architectures with 16-bit alignment
> broken...

The offending commit seems to be the merge commit v4.18-rc1~114
that replaced "__u32 :32;" from the fix commit v4.17~4^2^2 with
"__u32 gpl_compatible:1;" from earlier commit v4.18-rc1~114^2~376^2~6.

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> -- 
> Strace-devel mailing list
> Strace-devel@...ts.strace.io
> https://lists.strace.io/mailman/listinfo/strace-devel

-- 
ldv

Download attachment "signature.asc" of type "application/pgp-signature" (802 bytes)

Powered by blists - more mailing lists