strace for m68k bpf_prog_info mismatch

DISCUSSION Geert Uytterhoeven 2 participants 2 messages May 3, 2019 – May 21, 2019 3 messages not in the archive Generic include/asm header files

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): 874l6c89nd.fsf@tarshish

A message that is not in the archive (sent off-list or not yet received): CAMuHMdUT3ug+SCzrnA2eD=QyOLaHUGAe-ZrbWfDUWxTJ4CWEtQ@mail.gmail.com

A message that is not in the archive (sent off-list or not yet received): 8736lv92ls.fsf@tarshish

Geert Uytterhoeven

author

2019-05-03 12:16 UTC #3 lore raw

Hi Baruch,

Quoted 45 lines · Baruch Siach

Quoted 20 lines · Geert Uytterhoeven
Quoted 19 lines · Baruch Siach
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 */
Quoted 7 lines
Quoted 4 lines
        /* 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:
include/uapi/linux/bpf.h +1 -1
index 929c8e537a14..709d4dddc229 100644
@@ -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;

Quoted 1 line

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.

Quoted 15 lines

Quoted 11 lines
Quoted 8 lines
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:

Quoted 8 lines

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...

Gr{oetje,eeting}s,

Geert

Signature · 5 lines

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

Dmitry V. Levin

2019-05-21 22:00 UTC #4 lore raw

Hi Baruch, Geert,

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

Quoted 77 lines · Geert Uytterhoeven

Hi Baruch,
Quoted 45 lines · Baruch Siach
Quoted 20 lines · Geert Uytterhoeven
Quoted 19 lines · Baruch Siach
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 */
Quoted 7 lines
Quoted 4 lines
        /* 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:
include/uapi/linux/bpf.h +1 -1
index 929c8e537a14..709d4dddc229 100644
@@ -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;
Quoted 1 line
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.
Quoted 15 lines
Quoted 11 lines
Quoted 8 lines
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:
Quoted 8 lines
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 */

Quoted 7 lines

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.

Quoted 14 lines

Gr{oetje,eeting}s,

Geert

Signature · 9 lines

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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
[email protected]
https://lists.strace.io/mailman/listinfo/strace-devel

Signature · 1 line

ldv