Patchwork [bpf-next,2/4] libbpf: Support 32-bit static data loads

login
register
mail settings
Submitter Joe Stringer
Date Feb. 12, 2019, 12:47 a.m.
Message ID <20190212004729.535-3-joe@wand.net.nz>
Download mbox | patch
Permalink /patch/723443/
State New
Headers show

Comments

Joe Stringer - Feb. 12, 2019, 12:47 a.m.
Support loads of static 32-bit data when BPF writers make use of
convenience macros for accessing static global data variables. A later
patch in this series will demonstrate its usage in a selftest.

As of LLVM-7, this technique only works with 32-bit data, as LLVM will
complain if this technique is attempted with data of other sizes:

    LLVM ERROR: Unsupported relocation: try to compile with -O2 or above,
    or check your static variable usage

Based on the proof of concept by Daniel Borkmann (presented at LPC 2018).

Signed-off-by: Joe Stringer <joe@wand.net.nz>
---
 tools/lib/bpf/libbpf.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)
Y Song - Feb. 15, 2019, 5:38 a.m.
On Mon, Feb 11, 2019 at 4:48 PM Joe Stringer <joe@wand.net.nz> wrote:
>
> Support loads of static 32-bit data when BPF writers make use of
> convenience macros for accessing static global data variables. A later
> patch in this series will demonstrate its usage in a selftest.
>
> As of LLVM-7, this technique only works with 32-bit data, as LLVM will
> complain if this technique is attempted with data of other sizes:
>
>     LLVM ERROR: Unsupported relocation: try to compile with -O2 or above,
>     or check your static variable usage

A little bit clarification from compiler side.
The above compiler error is to prevent people use static variables since current
kernel/libbpf does not handle this. The compiler only warns if .bss or
.data section
has more than one definitions. The first definition always has section offset 0
and the compiler did not warn.

The restriction is a little strange. To only work with 32-bit data is
not a right
statement. The following are some examples.

The following static variable definitions will succeed:
static int a; /* one in .bss */
static long b = 2;  /* one in .data */

The following definitions will fail as both in .bss.
static int a;
static int b;

The following definitions will fail as both in .data:
static char a = 2;
static int b = 3;

Using global variables can prevent compiler errors.
maps are defined as globals and the compiler does not
check whether a particular global variable is defining a map or not.

If you just use static variable like below
static int a = 2;
without potential assignment to a, the compiler will replace variable
a with 2 at compile time.
An alternative is to define like below
static volatile int a = 2;
You can get a "load" for variable "a" in the bpf load even if there is
no assignment to a.

Maybe now is the time to remove the compiler assertions as
libbpf/kernel starts to
handle static variables?

>
> Based on the proof of concept by Daniel Borkmann (presented at LPC 2018).
>
> Signed-off-by: Joe Stringer <joe@wand.net.nz>
> ---
>  tools/lib/bpf/libbpf.c | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 1ec28d5154dc..da35d5559b22 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -140,11 +140,13 @@ struct bpf_program {
>                 enum {
>                         RELO_LD64,
>                         RELO_CALL,
> +                       RELO_DATA,
>                 } type;
>                 int insn_idx;
>                 union {
>                         int map_idx;
>                         int text_off;
> +                       uint32_t data;
>                 };
>         } *reloc_desc;
>         int nr_reloc;
> @@ -210,6 +212,7 @@ struct bpf_object {
>                 Elf *elf;
>                 GElf_Ehdr ehdr;
>                 Elf_Data *symbols;
> +               Elf_Data *global_data;
>                 size_t strtabidx;
>                 struct {
>                         GElf_Shdr shdr;
> @@ -218,6 +221,7 @@ struct bpf_object {
>                 int nr_reloc;
>                 int maps_shndx;
>                 int text_shndx;
> +               int data_shndx;
>         } efile;
>         /*
>          * All loaded bpf_object is linked in a list, which is
> @@ -476,6 +480,7 @@ static void bpf_object__elf_finish(struct bpf_object *obj)
>                 obj->efile.elf = NULL;
>         }
>         obj->efile.symbols = NULL;
> +       obj->efile.global_data = NULL;
>
>         zfree(&obj->efile.reloc);
>         obj->efile.nr_reloc = 0;
> @@ -866,6 +871,9 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
>                                         pr_warning("failed to alloc program %s (%s): %s",
>                                                    name, obj->path, cp);
>                                 }
> +                       } else if (strcmp(name, ".data") == 0) {
> +                               obj->efile.global_data = data;
> +                               obj->efile.data_shndx = idx;
>                         }
>                 } else if (sh.sh_type == SHT_REL) {
>                         void *reloc = obj->efile.reloc;
> @@ -962,6 +970,7 @@ bpf_program__collect_reloc(struct bpf_program *prog, GElf_Shdr *shdr,
>         Elf_Data *symbols = obj->efile.symbols;
>         int text_shndx = obj->efile.text_shndx;
>         int maps_shndx = obj->efile.maps_shndx;
> +       int data_shndx = obj->efile.data_shndx;
>         struct bpf_map *maps = obj->maps;
>         size_t nr_maps = obj->nr_maps;
>         int i, nrels;
> @@ -1000,8 +1009,9 @@ bpf_program__collect_reloc(struct bpf_program *prog, GElf_Shdr *shdr,
>                          (long long) (rel.r_info >> 32),
>                          (long long) sym.st_value, sym.st_name);
>
> -               if (sym.st_shndx != maps_shndx && sym.st_shndx != text_shndx) {
> -                       pr_warning("Program '%s' contains non-map related relo data pointing to section %u\n",
> +               if (sym.st_shndx != maps_shndx && sym.st_shndx != text_shndx &&
> +                   sym.st_shndx != data_shndx) {
> +                       pr_warning("Program '%s' contains unrecognized relo data pointing to section %u\n",
>                                    prog->section_name, sym.st_shndx);
>                         return -LIBBPF_ERRNO__RELOC;
>                 }
> @@ -1046,6 +1056,20 @@ bpf_program__collect_reloc(struct bpf_program *prog, GElf_Shdr *shdr,
>                         prog->reloc_desc[i].type = RELO_LD64;
>                         prog->reloc_desc[i].insn_idx = insn_idx;
>                         prog->reloc_desc[i].map_idx = map_idx;
> +               } else if (sym.st_shndx == data_shndx) {
> +                       Elf_Data *global_data = obj->efile.global_data;
> +                       uint32_t *static_data;
> +
> +                       if (sym.st_value + sizeof(uint32_t) > (int)global_data->d_size) {
> +                               pr_warning("bpf relocation: static data load beyond data size %lu\n",
> +                                          global_data->d_size);
> +                               return -LIBBPF_ERRNO__RELOC;
> +                       }
> +
> +                       static_data = global_data->d_buf + sym.st_value;
> +                       prog->reloc_desc[i].type = RELO_DATA;
> +                       prog->reloc_desc[i].insn_idx = insn_idx;
> +                       prog->reloc_desc[i].data = *static_data;
>                 }
>         }
>         return 0;
> @@ -1399,6 +1423,12 @@ bpf_program__relocate(struct bpf_program *prog, struct bpf_object *obj)
>                                                       &prog->reloc_desc[i]);
>                         if (err)
>                                 return err;
> +               } else if (prog->reloc_desc[i].type == RELO_DATA) {
> +                       struct bpf_insn *insns = prog->insns;
> +                       int insn_idx;
> +
> +                       insn_idx = prog->reloc_desc[i].insn_idx;
> +                       insns[insn_idx].imm = prog->reloc_desc[i].data;
>                 }
>         }
>
> --
> 2.19.1
>
Joe Stringer - Feb. 15, 2019, 7:16 a.m.
On Thu, 14 Feb 2019 at 21:39, Y Song <ys114321@gmail.com> wrote:
>
> On Mon, Feb 11, 2019 at 4:48 PM Joe Stringer <joe@wand.net.nz> wrote:
> >
> > Support loads of static 32-bit data when BPF writers make use of
> > convenience macros for accessing static global data variables. A later
> > patch in this series will demonstrate its usage in a selftest.
> >
> > As of LLVM-7, this technique only works with 32-bit data, as LLVM will
> > complain if this technique is attempted with data of other sizes:
> >
> >     LLVM ERROR: Unsupported relocation: try to compile with -O2 or above,
> >     or check your static variable usage
>
> A little bit clarification from compiler side.
> The above compiler error is to prevent people use static variables since current
> kernel/libbpf does not handle this. The compiler only warns if .bss or
> .data section
> has more than one definitions. The first definition always has section offset 0
> and the compiler did not warn.

Ah, interesting. I observed that warning when I attempted to define
global variables of multiple sizes, and I thought also with sizes
other than 32-bit. This clarifies things a bit, thanks.

For the .bss my observation was that if you had a definition like:

static int a = 0;

Then this will be placed into .bss, hence why I looked into the
approach from this patch for patch 3 as well.

> The restriction is a little strange. To only work with 32-bit data is
> not a right
> statement. The following are some examples.
>
> The following static variable definitions will succeed:
> static int a; /* one in .bss */
> static long b = 2;  /* one in .data */
>
> The following definitions will fail as both in .bss.
> static int a;
> static int b;
>
> The following definitions will fail as both in .data:
> static char a = 2;
> static int b = 3;

Are there type restrictions or something? I've been defining multiple
static uint32_t and using them per the approach in this patch series
without hitting this compiler assertion.

> Using global variables can prevent compiler errors.
> maps are defined as globals and the compiler does not
> check whether a particular global variable is defining a map or not.
>
> If you just use static variable like below
> static int a = 2;
> without potential assignment to a, the compiler will replace variable
> a with 2 at compile time.
> An alternative is to define like below
> static volatile int a = 2;
> You can get a "load" for variable "a" in the bpf load even if there is
> no assignment to a.

I'll take a closer look at this too.

> Maybe now is the time to remove the compiler assertions as
> libbpf/kernel starts to
> handle static variables?

I don't understand why those assertions exists in this form. It
already allows code which will not load with libbpf (ie generate any
.data/.bss), does it help prevent unexpected situations for
developers?
Alexei Starovoitov - Feb. 15, 2019, 4:17 p.m.
On Thu, Feb 14, 2019 at 9:38 PM Y Song <ys114321@gmail.com> wrote:
>
> On Mon, Feb 11, 2019 at 4:48 PM Joe Stringer <joe@wand.net.nz> wrote:
> >
> > Support loads of static 32-bit data when BPF writers make use of
> > convenience macros for accessing static global data variables. A later
> > patch in this series will demonstrate its usage in a selftest.
> >
> > As of LLVM-7, this technique only works with 32-bit data, as LLVM will
> > complain if this technique is attempted with data of other sizes:
> >
> >     LLVM ERROR: Unsupported relocation: try to compile with -O2 or above,
> >     or check your static variable usage
>
> A little bit clarification from compiler side.
> The above compiler error is to prevent people use static variables since current
> kernel/libbpf does not handle this. The compiler only warns if .bss or
> .data section
> has more than one definitions. The first definition always has section offset 0
> and the compiler did not warn.

ahh. missed that while playing with .s output.
when target is asm clang doesn't complain.
let's relax this llvm error.
will set of existing relocation be enough to generate
properly formed .o ?
or we need to define a new one?
Y Song - Feb. 15, 2019, 8:18 p.m.
On Thu, Feb 14, 2019 at 11:16 PM Joe Stringer <joe@wand.net.nz> wrote:
>
> On Thu, 14 Feb 2019 at 21:39, Y Song <ys114321@gmail.com> wrote:
> >
> > On Mon, Feb 11, 2019 at 4:48 PM Joe Stringer <joe@wand.net.nz> wrote:
> > >
> > > Support loads of static 32-bit data when BPF writers make use of
> > > convenience macros for accessing static global data variables. A later
> > > patch in this series will demonstrate its usage in a selftest.
> > >
> > > As of LLVM-7, this technique only works with 32-bit data, as LLVM will
> > > complain if this technique is attempted with data of other sizes:
> > >
> > >     LLVM ERROR: Unsupported relocation: try to compile with -O2 or above,
> > >     or check your static variable usage
> >
> > A little bit clarification from compiler side.
> > The above compiler error is to prevent people use static variables since current
> > kernel/libbpf does not handle this. The compiler only warns if .bss or
> > .data section
> > has more than one definitions. The first definition always has section offset 0
> > and the compiler did not warn.
>
> Ah, interesting. I observed that warning when I attempted to define
> global variables of multiple sizes, and I thought also with sizes
> other than 32-bit. This clarifies things a bit, thanks.
>
> For the .bss my observation was that if you had a definition like:
>
> static int a = 0;
>
> Then this will be placed into .bss, hence why I looked into the
> approach from this patch for patch 3 as well.
>
> > The restriction is a little strange. To only work with 32-bit data is
> > not a right
> > statement. The following are some examples.
> >
> > The following static variable definitions will succeed:
> > static int a; /* one in .bss */
> > static long b = 2;  /* one in .data */
> >
> > The following definitions will fail as both in .bss.
> > static int a;
> > static int b;
> >
> > The following definitions will fail as both in .data:
> > static char a = 2;
> > static int b = 3;
>
> Are there type restrictions or something? I've been defining multiple

There is no type restrictions.
-bash-4.4$ cat g.c
struct t {
  int a;
  char b;
  long c;
};
static volatile struct t v;
int test() { return v.a + v.b; }
-bash-4.4$ clang -O2 -g -c -target bpf g.c
-bash-4.4$

> static uint32_t and using them per the approach in this patch series
> without hitting this compiler assertion.

-bash-4.4$ cat g1.c
static volatile int a;
static volatile int b;
int test() { return a + b; }
-bash-4.4$ clang -O2 -g -c -target bpf g1.c
fatal error: error in backend: Unsupported relocation: try to compile
with -O2 or above, or check your static variable
      usage
-bash-4.4$

>
> > Using global variables can prevent compiler errors.
> > maps are defined as globals and the compiler does not
> > check whether a particular global variable is defining a map or not.
> >
> > If you just use static variable like below
> > static int a = 2;
> > without potential assignment to a, the compiler will replace variable
> > a with 2 at compile time.
> > An alternative is to define like below
> > static volatile int a = 2;
> > You can get a "load" for variable "a" in the bpf load even if there is
> > no assignment to a.
>
> I'll take a closer look at this too.
>
> > Maybe now is the time to remove the compiler assertions as
> > libbpf/kernel starts to
> > handle static variables?
>
> I don't understand why those assertions exists in this form. It
> already allows code which will not load with libbpf (ie generate any
> .data/.bss), does it help prevent unexpected situations for
> developers?

The error is introduced by the following llvm commit:
commit 39184e407cd937f2f20d3f61eec205925bae7b13
Author: Yonghong Song <yhs@fb.com>
Date:   Wed Aug 22 21:21:03 2018 +0000

    bpf: fix an assertion in BPFAsmBackend applyFixup()

    Fix bug https://bugs.llvm.org/show_bug.cgi?id=38643

    In BPFAsmBackend applyFixup(), there is an assertion for FixedValue to be 0.
    This may not be true, esp. for optimiation level 0.
    For example, in the above bug, for the following two
    static variables:
      @bpf_map_lookup_elem = internal global i8* (i8*, i8*)*
          inttoptr (i64 1 to i8* (i8*, i8*)*), align 8
      @bpf_map_update_elem = internal global i32 (i8*, i8*, i8*, i64)*
          inttoptr (i64 2 to i32 (i8*, i8*, i8*, i64)*), align 8

    The static variable @bpf_map_update_elem will have a symbol
    offset of 8 and a FK_SecRel_8 with FixupValue 8 will cause
    the assertion if llvm is built with -DLLVM_ENABLE_ASSERTIONS=ON.

    The above relocations will not exist if the program is compiled
    with optimization level -O1 and above as the compiler optimizes
    those static variables away. In the below error message, -O2
    is suggested as this is the common practice.

    Note that FixedValue = 0 in applyFixup() does exist and is valid,
    e.g., for the global variable my_map in the above bug. The bpf
    loader will process them properly for map_id's before loading
    the program into the kernel.

    The static variables, which are not optimized away by compiler,
    may have FK_SecRel_8 relocation with non-zero FixedValue.

    The patch removed the offending assertion and will issue
    a hard error as below if the FixedValue in applyFixup()
    is not 0.
      $ llc -march=bpf -filetype=obj fixup.ll
      LLVM ERROR: Unsupported relocation: try to compile with -O2 or above,
          or check your static variable usage

Its main purpose is to fix a behavior difference with and without
-DLLVM_ENABLE_ASSERTIONS=ON. The patch generated an error
regardless whether the compiler time assertion is turned on or not.

It does not catch all the cases e.g., only one static variable is defined,
which needs fine tuning as there are legitimate cases (e.g., in some dwarf
sessions) where the Fixup is valid with FixedValue = 0.

If you try to use more than onee static variables, the compiler will
print an error and let you know your potential issues.

The question is since we are on the path to allow static variables
in the bpf programs for later patching, we probably should remove
this compiler fatal error?

Patch

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 1ec28d5154dc..da35d5559b22 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -140,11 +140,13 @@  struct bpf_program {
 		enum {
 			RELO_LD64,
 			RELO_CALL,
+			RELO_DATA,
 		} type;
 		int insn_idx;
 		union {
 			int map_idx;
 			int text_off;
+			uint32_t data;
 		};
 	} *reloc_desc;
 	int nr_reloc;
@@ -210,6 +212,7 @@  struct bpf_object {
 		Elf *elf;
 		GElf_Ehdr ehdr;
 		Elf_Data *symbols;
+		Elf_Data *global_data;
 		size_t strtabidx;
 		struct {
 			GElf_Shdr shdr;
@@ -218,6 +221,7 @@  struct bpf_object {
 		int nr_reloc;
 		int maps_shndx;
 		int text_shndx;
+		int data_shndx;
 	} efile;
 	/*
 	 * All loaded bpf_object is linked in a list, which is
@@ -476,6 +480,7 @@  static void bpf_object__elf_finish(struct bpf_object *obj)
 		obj->efile.elf = NULL;
 	}
 	obj->efile.symbols = NULL;
+	obj->efile.global_data = NULL;
 
 	zfree(&obj->efile.reloc);
 	obj->efile.nr_reloc = 0;
@@ -866,6 +871,9 @@  static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
 					pr_warning("failed to alloc program %s (%s): %s",
 						   name, obj->path, cp);
 				}
+			} else if (strcmp(name, ".data") == 0) {
+				obj->efile.global_data = data;
+				obj->efile.data_shndx = idx;
 			}
 		} else if (sh.sh_type == SHT_REL) {
 			void *reloc = obj->efile.reloc;
@@ -962,6 +970,7 @@  bpf_program__collect_reloc(struct bpf_program *prog, GElf_Shdr *shdr,
 	Elf_Data *symbols = obj->efile.symbols;
 	int text_shndx = obj->efile.text_shndx;
 	int maps_shndx = obj->efile.maps_shndx;
+	int data_shndx = obj->efile.data_shndx;
 	struct bpf_map *maps = obj->maps;
 	size_t nr_maps = obj->nr_maps;
 	int i, nrels;
@@ -1000,8 +1009,9 @@  bpf_program__collect_reloc(struct bpf_program *prog, GElf_Shdr *shdr,
 			 (long long) (rel.r_info >> 32),
 			 (long long) sym.st_value, sym.st_name);
 
-		if (sym.st_shndx != maps_shndx && sym.st_shndx != text_shndx) {
-			pr_warning("Program '%s' contains non-map related relo data pointing to section %u\n",
+		if (sym.st_shndx != maps_shndx && sym.st_shndx != text_shndx &&
+		    sym.st_shndx != data_shndx) {
+			pr_warning("Program '%s' contains unrecognized relo data pointing to section %u\n",
 				   prog->section_name, sym.st_shndx);
 			return -LIBBPF_ERRNO__RELOC;
 		}
@@ -1046,6 +1056,20 @@  bpf_program__collect_reloc(struct bpf_program *prog, GElf_Shdr *shdr,
 			prog->reloc_desc[i].type = RELO_LD64;
 			prog->reloc_desc[i].insn_idx = insn_idx;
 			prog->reloc_desc[i].map_idx = map_idx;
+		} else if (sym.st_shndx == data_shndx) {
+			Elf_Data *global_data = obj->efile.global_data;
+			uint32_t *static_data;
+
+			if (sym.st_value + sizeof(uint32_t) > (int)global_data->d_size) {
+				pr_warning("bpf relocation: static data load beyond data size %lu\n",
+					   global_data->d_size);
+				return -LIBBPF_ERRNO__RELOC;
+			}
+
+			static_data = global_data->d_buf + sym.st_value;
+			prog->reloc_desc[i].type = RELO_DATA;
+			prog->reloc_desc[i].insn_idx = insn_idx;
+			prog->reloc_desc[i].data = *static_data;
 		}
 	}
 	return 0;
@@ -1399,6 +1423,12 @@  bpf_program__relocate(struct bpf_program *prog, struct bpf_object *obj)
 						      &prog->reloc_desc[i]);
 			if (err)
 				return err;
+		} else if (prog->reloc_desc[i].type == RELO_DATA) {
+			struct bpf_insn *insns = prog->insns;
+			int insn_idx;
+
+			insn_idx = prog->reloc_desc[i].insn_idx;
+			insns[insn_idx].imm = prog->reloc_desc[i].data;
 		}
 	}