[PATCH bpf-next v2 0/1] Avoid size mismatches in skeletons

[PATCH bpf-next v2 0/1] Avoid size mismatches in skeletons

[Delyan Kratunov]

As reported in [0], kernel and userspace can sometimes disagree
on the size of a type. This leads to trouble when userspace maps the memory of
a bpf program and reads/writes to it assuming a different memory layout.

With this change, the skeletons now contain size asserts to ensure the
types in userspace are compatible in size with the types in the bpf program.
In particular, we emit asserts for all top-level fields in the data/rodata/bss/etc
structs, but not recursively for the individual members inside - this strikes a
compromise between diagnostics precision and still catching all possible size
mismatches.

The generated asserts are somewhat ugly but are able to handle anonymous structs:

  struct test_skeleton__data {
          int in1;
          char __pad0[4];
          long long in2;
          int out1;
          char __pad1[4];
          long long out2;
  } *data;
  BPF_STATIC_ASSERT(sizeof(((struct test_skeleton__data*)0)->in1) == 4, "unexpe
cted size of field in1");
  BPF_STATIC_ASSERT(sizeof(((struct test_skeleton__data*)0)->in2) == 8, "unexpe
cted size of field in2");
  BPF_STATIC_ASSERT(sizeof(((struct test_skeleton__data*)0)->out1) == 4, "unexp
ected size of field out1");
  BPF_STATIC_ASSERT(sizeof(((struct test_skeleton__data*)0)->out2) == 8, "unexp
ected size of field out2");
  struct test_skeleton__rodata {
          struct {
                  int in6;
          } in;
  } *rodata;
  BPF_STATIC_ASSERT(sizeof(((struct test_skeleton__rodata*)0)->in) == 4, "unexp
ected size of field in");

I'm open to pushing more of the ugliness into a macro, I was going primarily for
simplicity in the diagnostic messages (it's unfortunate enough that we need a level
of macro expansion for C++ support). If we need this to be prettier, what's a good
header I could push any extra complexity into, so it's not spelled out in gen.c?

Delyan Kratunov (1):
  bpftool: bpf skeletons assert type sizes

 tools/bpf/bpftool/gen.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

--
2.34.1

[PATCH bpf-next v2 1/1] bpftool: bpf skeletons assert type sizes

[Delyan Kratunov]

When emitting type declarations in skeletons, bpftool will now also emit
static assertions on the size of the data/bss/rodata/etc fields. This
ensures that in situations where userspace and kernel types have the same
name but differ in size we do not silently produce incorrect results but
instead break the build.

This was reported in [1] and as expected the repro in [2] fails to build
on the new size assert after this change.

  [1]: Closes: https://github.com/libbpf/libbpf/issues/433
  [2]: https://github.com/fuweid/iovisor-bcc-pr-3777

Signed-off-by: Delyan Kratunov <delyank@fb.com>
---
 tools/bpf/bpftool/gen.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
index 6f2e20be0c62..e7f11899437a 100644
--- a/tools/bpf/bpftool/gen.c
+++ b/tools/bpf/bpftool/gen.c
@@ -205,6 +205,29 @@ static int codegen_datasec_def(struct bpf_object *obj,
 		off = sec_var->offset + sec_var->size;
 	}
 	printf("	} *%s;\n", sec_ident);
+
+	/* Walk through the section again to emit size asserts */
+	sec_var = btf_var_secinfos(sec);
+	for (i = 0; i < vlen; i++, sec_var++) {
+		const struct btf_type *var = btf__type_by_id(btf, sec_var->type);
+		const char *var_name = btf__name_by_offset(btf, var->name_off);
+		__u32 var_type_id = var->type;
+		__s64 var_size = btf__resolve_size(btf, var_type_id);
+
+		/* static variables are not exposed through BPF skeleton */
+		if (btf_var(var)->linkage == BTF_VAR_STATIC)
+			continue;
+
+		var_ident[0] = '\0';
+		strncat(var_ident, var_name, sizeof(var_ident) - 1);
+		sanitize_identifier(var_ident);
+
+		printf("\tBPF_STATIC_ASSERT(");
+		printf("sizeof(((struct %s__%s*)0)->%s) == %lld, ",
+		       obj_name, sec_ident, var_ident, var_size);
+		printf("\"unexpected size of field %s\");\n", var_ident);
+	}
+
 	return 0;
 }

@@ -756,6 +779,12 @@ static int do_skeleton(int argc, char **argv)
 									    \n\
 		#include <bpf/skel_internal.h>				    \n\
 									    \n\
+		#ifdef __cplusplus					    \n\
+		#define	BPF_STATIC_ASSERT static_assert			    \n\
+		#else							    \n\
+		#define	BPF_STATIC_ASSERT _Static_assert		    \n\
+		#endif							    \n\
+									    \n\
 		struct %1$s {						    \n\
 			struct bpf_loader_ctx ctx;			    \n\
 		",
@@ -774,6 +803,12 @@ static int do_skeleton(int argc, char **argv)
 		#include <stdlib.h>					    \n\
 		#include <bpf/libbpf.h>					    \n\
 									    \n\
+		#ifdef __cplusplus					    \n\
+		#define	BPF_STATIC_ASSERT static_assert			    \n\
+		#else							    \n\
+		#define	BPF_STATIC_ASSERT _Static_assert		    \n\
+		#endif							    \n\
+									    \n\
 		struct %1$s {						    \n\
 			struct bpf_object_skeleton *skeleton;		    \n\
 			struct bpf_object *obj;				    \n\
--
2.34.1

Re: [PATCH bpf-next v2 1/1] bpftool: bpf skeletons assert type sizes

[Andrii Nakryiko]

On Mon, Feb 14, 2022 at 4:27 PM Delyan Kratunov <delyank@fb.com> wrote:
>
> When emitting type declarations in skeletons, bpftool will now also emit
> static assertions on the size of the data/bss/rodata/etc fields. This
> ensures that in situations where userspace and kernel types have the same
> name but differ in size we do not silently produce incorrect results but
> instead break the build.
>
> This was reported in [1] and as expected the repro in [2] fails to build
> on the new size assert after this change.
>
>   [1]: Closes: https://github.com/libbpf/libbpf/issues/433
>   [2]: https://github.com/fuweid/iovisor-bcc-pr-3777
>
> Signed-off-by: Delyan Kratunov <delyank@fb.com>
> ---
>  tools/bpf/bpftool/gen.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> index 6f2e20be0c62..e7f11899437a 100644
> --- a/tools/bpf/bpftool/gen.c
> +++ b/tools/bpf/bpftool/gen.c
> @@ -205,6 +205,29 @@ static int codegen_datasec_def(struct bpf_object *obj,
>                 off = sec_var->offset + sec_var->size;
>         }
>         printf("        } *%s;\n", sec_ident);
> +
> +       /* Walk through the section again to emit size asserts */
> +       sec_var = btf_var_secinfos(sec);
> +       for (i = 0; i < vlen; i++, sec_var++) {
> +               const struct btf_type *var = btf__type_by_id(btf, sec_var->type);
> +               const char *var_name = btf__name_by_offset(btf, var->name_off);
> +               __u32 var_type_id = var->type;
> +               __s64 var_size = btf__resolve_size(btf, var_type_id);
> +
> +               /* static variables are not exposed through BPF skeleton */
> +               if (btf_var(var)->linkage == BTF_VAR_STATIC)
> +                       continue;
> +
> +               var_ident[0] = '\0';
> +               strncat(var_ident, var_name, sizeof(var_ident) - 1);
> +               sanitize_identifier(var_ident);
> +
> +               printf("\tBPF_STATIC_ASSERT(");
> +               printf("sizeof(((struct %s__%s*)0)->%s) == %lld, ",
> +                      obj_name, sec_ident, var_ident, var_size);
> +               printf("\"unexpected size of field %s\");\n", var_ident);
> +       }
> +

So doing it right after each section really pollutes the layout of the
skeleton's struct and hurts readability a lot.

How about adding all those _Static_asserts in <skeleton__elf_bytes()
function, after the huge binary dump, to get it out of sight? I think
if we are doing asserts, we might as well validate that not just
sizes, but also each variable's offset within the section is right.

Those huge struct casts are also pretty verbose. What if we do
something like this (assuming we are in a separate function, but we
can easily just do that in __elf_bytes(). Let's use test_skeleton as
skeleton name

struct test_skeleton *s = (void *)0;

_Static_assert(sizeof(s->data->in1) == 4, "invalid size of in1");
_Static_assert(offsetof(typeof(*skel->data), in1) == 0, "invalid
offset of in1");
...
_Static_assert(sizeof(s->data_read_mostly->read_mostly_var) == 4,
"invalid size of read_mostly_var");
_Static_assert(offsetof(typeof(*skel->data_read_mostly),
read_mostly_var) == 0, "invalid offset of read_mostly_var");

(void)s; /* avoid unused variable warning */

WDYT?

>         return 0;
>  }
>
> @@ -756,6 +779,12 @@ static int do_skeleton(int argc, char **argv)
>                                                                             \n\
>                 #include <bpf/skel_internal.h>                              \n\
>                                                                             \n\
> +               #ifdef __cplusplus                                          \n\
> +               #define BPF_STATIC_ASSERT static_assert                     \n\
> +               #else                                                       \n\
> +               #define BPF_STATIC_ASSERT _Static_assert                    \n\
> +               #endif                                                      \n\

Maybe just:

#ifdef __cplusplus
#define _Static_assert static_assert
#endif

? Or that doesn't work?

BPF_STATIC_ASSERT sounds very BPF-y, while this should stay within the skeleton.

Also any such macro has to be #undef in this file, otherwise it will
"leak" into the user's code (as this is just a header file included in
user's .c files).



> +                                                                           \n\
>                 struct %1$s {                                               \n\
>                         struct bpf_loader_ctx ctx;                          \n\
>                 ",
> @@ -774,6 +803,12 @@ static int do_skeleton(int argc, char **argv)
>                 #include <stdlib.h>                                         \n\
>                 #include <bpf/libbpf.h>                                     \n\
>                                                                             \n\
> +               #ifdef __cplusplus                                          \n\
> +               #define BPF_STATIC_ASSERT static_assert                     \n\
> +               #else                                                       \n\
> +               #define BPF_STATIC_ASSERT _Static_assert                    \n\
> +               #endif                                                      \n\
> +                                                                           \n\
>                 struct %1$s {                                               \n\
>                         struct bpf_object_skeleton *skeleton;               \n\
>                         struct bpf_object *obj;                             \n\
> --
> 2.34.1

Re: [PATCH bpf-next v2 1/1] bpftool: bpf skeletons assert type sizes

[Delyan Kratunov]

On Mon, 2022-02-14 at 21:11 -0800, Andrii Nakryiko wrote:
> So doing it right after each section really pollutes the layout of the
> skeleton's struct and hurts readability a lot.
> 
> How about adding all those _Static_asserts in <skeleton__elf_bytes()
> function, after the huge binary dump, to get it out of sight? 

I can just add a `void __attribute__((unused)) skeleton__assert_sizes()` at the
end? Or a `struct skeleton__type_asserts`? It feels weird to just put them in
elf_bytes, they don't belong there.

> I think
> if we are doing asserts, we might as well validate that not just
> sizes, but also each variable's offset within the section is right.

Sure, can do.


> _Static_assert(sizeof(s->data->in1) == 4, "invalid size of in1");
> _Static_assert(offsetof(typeof(*skel->data), in1) == 0, "invalid
> offset of in1");
> ...
> _Static_assert(sizeof(s->data_read_mostly->read_mostly_var) == 4,
> "invalid size of read_mostly_var");
> _Static_assert(offsetof(typeof(*skel->data_read_mostly),
> read_mostly_var) == 0, "invalid offset of read_mostly_var");
> 
> (void)s; /* avoid unused variable warning */
> 
> WDYT?

That's fine by me, I have no objections. I'll see if a function or a struct is
more readable. 

I suspect `SIZE_ASSERT(data, in1, 4); OFFSET_ASSERT(data, in1, 0);` is probably
most readable but I hate that I'd have to include the macros inline (to emit the
skeleton type name).

> >         return 0;
> >  }
> > 
> > @@ -756,6 +779,12 @@ static int do_skeleton(int argc, char **argv)
> >                                                                             \n\
> >                 #include <bpf/skel_internal.h>                              \n\
> >                                                                             \n\
> > +               #ifdef __cplusplus                                          \n\
> > +               #define BPF_STATIC_ASSERT static_assert                     \n\
> > +               #else                                                       \n\
> > +               #define BPF_STATIC_ASSERT _Static_assert                    \n\
> > +               #endif                                                      \n\
> 
> Maybe just:
> 
> #ifdef __cplusplus
> #define _Static_assert static_assert
> #endif
> 
> ? Or that doesn't work?

It does work, it's just less explicit. I'd be happy to remove the macro
expansion on the C path though, it would make diagnostics shorter.


> Also any such macro has to be #undef in this file, otherwise it will
> "leak" into the user's code (as this is just a header file included in
> user's .c files).

My bad, just thought of that too.

--

To summarize, structurally I'll do this:

1. Put them all in one place. (tbd what type)
2. Put them at the end of the file.
3. Add offsets.
4. Fix up the macro usage.

Re: [PATCH bpf-next v2 1/1] bpftool: bpf skeletons assert type sizes

[Andrii Nakryiko]

On Tue, Feb 15, 2022 at 9:27 AM Delyan Kratunov <delyank@fb.com> wrote:
>
> On Mon, 2022-02-14 at 21:11 -0800, Andrii Nakryiko wrote:
> > So doing it right after each section really pollutes the layout of the
> > skeleton's struct and hurts readability a lot.
> >
> > How about adding all those _Static_asserts in <skeleton__elf_bytes()
> > function, after the huge binary dump, to get it out of sight?
>
> I can just add a `void __attribute__((unused)) skeleton__assert_sizes()` at the
> end? Or a `struct skeleton__type_asserts`? It feels weird to just put them in
> elf_bytes, they don't belong there.

SGTM.

>
> > I think
> > if we are doing asserts, we might as well validate that not just
> > sizes, but also each variable's offset within the section is right.
>
> Sure, can do.

Alexei pointed out that it's very unlikely that we'll mess up offsets
(we have actual offset from BTF and then we control alignment in
skeleton's struct, so should never get out of sync), so let's skip
offset assertion for now.

>
>
> > _Static_assert(sizeof(s->data->in1) == 4, "invalid size of in1");
> > _Static_assert(offsetof(typeof(*skel->data), in1) == 0, "invalid
> > offset of in1");
> > ...
> > _Static_assert(sizeof(s->data_read_mostly->read_mostly_var) == 4,
> > "invalid size of read_mostly_var");
> > _Static_assert(offsetof(typeof(*skel->data_read_mostly),
> > read_mostly_var) == 0, "invalid offset of read_mostly_var");
> >
> > (void)s; /* avoid unused variable warning */
> >
> > WDYT?
>
> That's fine by me, I have no objections. I'll see if a function or a struct is
> more readable.
>
> I suspect `SIZE_ASSERT(data, in1, 4); OFFSET_ASSERT(data, in1, 0);` is probably
> most readable but I hate that I'd have to include the macros inline (to emit the
> skeleton type name).

No one should read those asserts, so putting them somewhere after
elf_bytes function and writing out _Static_assert() directly is
probably best for when one of those asserts fires. It will result in
simpler compiler error (rather than unscrambling a chain of macro
invocations). So yeah, I'd stick to a bit more verbose _Static_assert.


>
> > >         return 0;
> > >  }
> > >
> > > @@ -756,6 +779,12 @@ static int do_skeleton(int argc, char **argv)
> > >                                                                             \n\
> > >                 #include <bpf/skel_internal.h>                              \n\
> > >                                                                             \n\
> > > +               #ifdef __cplusplus                                          \n\
> > > +               #define BPF_STATIC_ASSERT static_assert                     \n\
> > > +               #else                                                       \n\
> > > +               #define BPF_STATIC_ASSERT _Static_assert                    \n\
> > > +               #endif                                                      \n\
> >
> > Maybe just:
> >
> > #ifdef __cplusplus
> > #define _Static_assert static_assert
> > #endif
> >
> > ? Or that doesn't work?
>
> It does work, it's just less explicit. I'd be happy to remove the macro
> expansion on the C path though, it would make diagnostics shorter.

Yep, it was my thinking that we should "optimize" for pure C case.

>
>
> > Also any such macro has to be #undef in this file, otherwise it will
> > "leak" into the user's code (as this is just a header file included in
> > user's .c files).
>
> My bad, just thought of that too.
>
> --
>
> To summarize, structurally I'll do this:
>
> 1. Put them all in one place. (tbd what type)
> 2. Put them at the end of the file.
> 3. Add offsets.
> 4. Fix up the macro usage.
>