[PATCH bpf-next 0/4] libbpf: Add support for 32-bit static data

[PATCH bpf-next 0/4] libbpf: Add support for 32-bit static data

[Joe Stringer]

This series adds support to libbpf for relocating references to 32-bit
static data inside ELF files, both for .data and .bss, similar to one of
the approaches proposed in LPC 2018[0]. This improves a common workflow
for BPF users, where the BPF program may be customised each time it is
loaded, for example to tailor IP addresses for each instance of the
loaded program. Current approaches require full recompilation of the
programs for each load, however with templatized BPF programs, one ELF
template program may be generated, then the static data can be easily
substituted prior to loading into the kernel without invoking the
compiler again.

The approach here is useful for templating limited static data for ELF
programs, and will work regardless of kernel support for static data
sections. Its main limitation is that static data must be defined as
32-bit values in the BPF C input code (or defined using macros that use
32-bit values as the underlying store). The alternative approach
proposed at LPC would be more general and is being actively explored,
however it requires kernel extension and so will not solve this problem
for any existing kernels that are in use today.

There are similar patches floating around for iproute2 which I would
like to upstream as well[1].

[0] https://linuxplumbersconf.org/event/2/contributions/115/
[1] https://github.com/joestringer/iproute2/tree/bss

Joe Stringer (4):
  libbpf: Refactor relocations
  libbpf: Support 32-bit static data loads
  libbpf: Support relocations for bss.
  selftests/bpf: Test static data relocation

 tools/lib/bpf/libbpf.c                        | 108 ++++++++++++------
 tools/testing/selftests/bpf/Makefile          |   2 +-
 tools/testing/selftests/bpf/test_progs.c      |  44 +++++++
 .../selftests/bpf/test_static_data_kern.c     |  47 ++++++++
 4 files changed, 168 insertions(+), 33 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/test_static_data_kern.c

-- 
2.19.1

[PATCH bpf-next 1/4] libbpf: Refactor relocations

[Joe Stringer]

Adjust the code for relocations slightly with no functional changes, so
that upcoming patches that will introduce support for relocations into
the .data and .bss sections can be added independent of these changes.

Signed-off-by: Joe Stringer <joe@wand.net.nz>
---
 tools/lib/bpf/libbpf.c | 62 ++++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 30 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index e3c39edfb9d3..1ec28d5154dc 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -852,20 +852,20 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
 				obj->efile.symbols = data;
 				obj->efile.strtabidx = sh.sh_link;
 			}
-		} else if ((sh.sh_type == SHT_PROGBITS) &&
-			   (sh.sh_flags & SHF_EXECINSTR) &&
-			   (data->d_size > 0)) {
-			if (strcmp(name, ".text") == 0)
-				obj->efile.text_shndx = idx;
-			err = bpf_object__add_program(obj, data->d_buf,
-						      data->d_size, name, idx);
-			if (err) {
-				char errmsg[STRERR_BUFSIZE];
-				char *cp = libbpf_strerror_r(-err, errmsg,
-							     sizeof(errmsg));
-
-				pr_warning("failed to alloc program %s (%s): %s",
-					   name, obj->path, cp);
+		} else if (sh.sh_type == SHT_PROGBITS && data->d_size > 0) {
+			if (sh.sh_flags & SHF_EXECINSTR) {
+				if (strcmp(name, ".text") == 0)
+					obj->efile.text_shndx = idx;
+				err = bpf_object__add_program(obj, data->d_buf,
+							      data->d_size, name, idx);
+				if (err) {
+					char errmsg[STRERR_BUFSIZE];
+					char *cp = libbpf_strerror_r(-err, errmsg,
+								     sizeof(errmsg));
+
+					pr_warning("failed to alloc program %s (%s): %s",
+						   name, obj->path, cp);
+				}
 			}
 		} else if (sh.sh_type == SHT_REL) {
 			void *reloc = obj->efile.reloc;
@@ -1027,24 +1027,26 @@ bpf_program__collect_reloc(struct bpf_program *prog, GElf_Shdr *shdr,
 			return -LIBBPF_ERRNO__RELOC;
 		}
 
-		/* TODO: 'maps' is sorted. We can use bsearch to make it faster. */
-		for (map_idx = 0; map_idx < nr_maps; map_idx++) {
-			if (maps[map_idx].offset == sym.st_value) {
-				pr_debug("relocation: find map %zd (%s) for insn %u\n",
-					 map_idx, maps[map_idx].name, insn_idx);
-				break;
+		if (sym.st_shndx == maps_shndx) {
+			/* TODO: 'maps' is sorted. We can use bsearch to make it faster. */
+			for (map_idx = 0; map_idx < nr_maps; map_idx++) {
+				if (maps[map_idx].offset == sym.st_value) {
+					pr_debug("relocation: find map %zd (%s) for insn %u\n",
+						 map_idx, maps[map_idx].name, insn_idx);
+					break;
+				}
 			}
-		}
 
-		if (map_idx >= nr_maps) {
-			pr_warning("bpf relocation: map_idx %d large than %d\n",
-				   (int)map_idx, (int)nr_maps - 1);
-			return -LIBBPF_ERRNO__RELOC;
-		}
+			if (map_idx >= nr_maps) {
+				pr_warning("bpf relocation: map_idx %d large than %d\n",
+					   (int)map_idx, (int)nr_maps - 1);
+				return -LIBBPF_ERRNO__RELOC;
+			}
 
-		prog->reloc_desc[i].type = RELO_LD64;
-		prog->reloc_desc[i].insn_idx = insn_idx;
-		prog->reloc_desc[i].map_idx = map_idx;
+			prog->reloc_desc[i].type = RELO_LD64;
+			prog->reloc_desc[i].insn_idx = insn_idx;
+			prog->reloc_desc[i].map_idx = map_idx;
+		}
 	}
 	return 0;
 }
@@ -1392,7 +1394,7 @@ bpf_program__relocate(struct bpf_program *prog, struct bpf_object *obj)
 			}
 			insns[insn_idx].src_reg = BPF_PSEUDO_MAP_FD;
 			insns[insn_idx].imm = obj->maps[map_idx].fd;
-		} else {
+		} else if (prog->reloc_desc[i].type == RELO_CALL) {
 			err = bpf_program__reloc_text(prog, obj,
 						      &prog->reloc_desc[i]);
 			if (err)
-- 
2.19.1

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

[Joe Stringer]

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(-)

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

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

[Y Song]

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
>

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

[Joe Stringer]

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?

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

[Y Song]

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?

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

[Y Song]

FYI.The latest llvm trunk will not emit errors for static variables. The patch
also gives detailed information how to relate a particular static
variable to a particular
relocation.

https://reviews.llvm.org/rL354954

Thanks,
Yonghong

On Fri, Feb 15, 2019 at 12:18 PM Y Song <ys114321@gmail.com> wrote:
>
> 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?

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

[Alexei Starovoitov]

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?

[PATCH bpf-next 3/4] libbpf: Support relocations for bss.

[Joe Stringer]

The BSS section in an ELF generated by LLVM represents constants for
uninitialized variables or variables that are configured with a zero
value. Support initializing zeroed static data by parsing the
relocations with references to the .bss section and zeroing them.

Signed-off-by: Joe Stringer <joe@wand.net.nz>
---
 tools/lib/bpf/libbpf.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index da35d5559b22..ff66d7e970c9 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -141,6 +141,7 @@ struct bpf_program {
 			RELO_LD64,
 			RELO_CALL,
 			RELO_DATA,
+			RELO_ZERO,
 		} type;
 		int insn_idx;
 		union {
@@ -222,6 +223,7 @@ struct bpf_object {
 		int maps_shndx;
 		int text_shndx;
 		int data_shndx;
+		int bss_shndx;
 	} efile;
 	/*
 	 * All loaded bpf_object is linked in a list, which is
@@ -901,6 +903,8 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
 				obj->efile.reloc[n].shdr = sh;
 				obj->efile.reloc[n].data = data;
 			}
+		} else if (sh.sh_type == SHT_NOBITS && strcmp(name, ".bss") == 0) {
+			obj->efile.bss_shndx = idx;
 		} else {
 			pr_debug("skip section(%d) %s\n", idx, name);
 		}
@@ -971,6 +975,7 @@ bpf_program__collect_reloc(struct bpf_program *prog, GElf_Shdr *shdr,
 	int text_shndx = obj->efile.text_shndx;
 	int maps_shndx = obj->efile.maps_shndx;
 	int data_shndx = obj->efile.data_shndx;
+	int bss_shndx = obj->efile.bss_shndx;
 	struct bpf_map *maps = obj->maps;
 	size_t nr_maps = obj->nr_maps;
 	int i, nrels;
@@ -1010,7 +1015,7 @@ bpf_program__collect_reloc(struct bpf_program *prog, GElf_Shdr *shdr,
 			 (long long) sym.st_value, sym.st_name);
 
 		if (sym.st_shndx != maps_shndx && sym.st_shndx != text_shndx &&
-		    sym.st_shndx != data_shndx) {
+		    sym.st_shndx != data_shndx && sym.st_shndx != bss_shndx) {
 			pr_warning("Program '%s' contains unrecognized relo data pointing to section %u\n",
 				   prog->section_name, sym.st_shndx);
 			return -LIBBPF_ERRNO__RELOC;
@@ -1070,6 +1075,9 @@ bpf_program__collect_reloc(struct bpf_program *prog, GElf_Shdr *shdr,
 			prog->reloc_desc[i].type = RELO_DATA;
 			prog->reloc_desc[i].insn_idx = insn_idx;
 			prog->reloc_desc[i].data = *static_data;
+		} else if (sym.st_shndx == bss_shndx) {
+			prog->reloc_desc[i].type = RELO_ZERO;
+			prog->reloc_desc[i].insn_idx = insn_idx;
 		}
 	}
 	return 0;
@@ -1429,6 +1437,10 @@ bpf_program__relocate(struct bpf_program *prog, struct bpf_object *obj)
 
 			insn_idx = prog->reloc_desc[i].insn_idx;
 			insns[insn_idx].imm = prog->reloc_desc[i].data;
+		} else if (prog->reloc_desc[i].type == RELO_ZERO) {
+			int insn_idx = prog->reloc_desc[i].insn_idx;
+
+			prog->insns[insn_idx].imm = 0;
 		}
 	}
 
-- 
2.19.1

[PATCH bpf-next 4/4] selftests/bpf: Test static data relocation

[Joe Stringer]

Add tests for libbpf relocation of static variable references into the
.data and .bss sections of the ELF.

Signed-off-by: Joe Stringer <joe@wand.net.nz>
---
 tools/testing/selftests/bpf/Makefile          |  2 +-
 tools/testing/selftests/bpf/test_progs.c      | 44 +++++++++++++++++
 .../selftests/bpf/test_static_data_kern.c     | 47 +++++++++++++++++++
 3 files changed, 92 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/test_static_data_kern.c

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index c7e1e3255448..ef52a58e2368 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -36,7 +36,7 @@ BPF_OBJ_FILES = \
 	get_cgroup_id_kern.o socket_cookie_prog.o test_select_reuseport_kern.o \
 	test_skb_cgroup_id_kern.o bpf_flow.o netcnt_prog.o test_xdp_vlan.o \
 	xdp_dummy.o test_map_in_map.o test_spin_lock.o test_map_lock.o \
-	test_sock_fields_kern.o
+	test_sock_fields_kern.o test_static_data_kern.o
 
 # Objects are built with default compilation flags and with sub-register
 # code-gen enabled.
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index c52bd90fbb34..72899d58a77c 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -736,6 +736,49 @@ static void test_pkt_md_access(void)
 	bpf_object__close(obj);
 }
 
+static void test_static_data_access(void)
+{
+	const char *file = "./test_static_data_kern.o";
+	struct bpf_object *obj;
+	__u32 duration = 0, retval;
+	int i, err, prog_fd, map_fd;
+	uint32_t value;
+
+	err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
+	if (CHECK(err, "load program", "error %d loading %s\n", err, file))
+		return;
+
+	map_fd = bpf_find_map(__func__, obj, "result");
+	if (map_fd < 0) {
+		error_cnt++;
+		goto close_prog;
+	}
+
+	err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
+				NULL, NULL, &retval, &duration);
+	CHECK(err || retval, "pass packet",
+	      "err %d errno %d retval %d duration %d\n",
+	      err, errno, retval, duration);
+
+	struct {
+		char *name;
+		uint32_t key;
+		uint32_t value;
+	} tests[] = {
+		{ "relocate .bss reference", 0, 0 },
+		{ "relocate .data reference", 1, 42 },
+	};
+	for (i = 0; i < sizeof(tests) / sizeof(tests[0]); i++) {
+		err = bpf_map_lookup_elem(map_fd, &tests[i].key, &value);
+		CHECK (err || value != tests[i].value, tests[i].name,
+		       "err %d result %d expected %d\n",
+		       err, value, tests[i].value);
+	}
+
+close_prog:
+	bpf_object__close(obj);
+}
+
 static void test_obj_name(void)
 {
 	struct {
@@ -2138,6 +2181,7 @@ int main(void)
 	test_flow_dissector();
 	test_spinlock();
 	test_map_lock();
+	test_static_data_access();
 
 	printf("Summary: %d PASSED, %d FAILED\n", pass_cnt, error_cnt);
 	return error_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
diff --git a/tools/testing/selftests/bpf/test_static_data_kern.c b/tools/testing/selftests/bpf/test_static_data_kern.c
new file mode 100644
index 000000000000..f2485af6bd0b
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_static_data_kern.c
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2019 Isovalent, Inc.
+
+#include <linux/bpf.h>
+#include <linux/pkt_cls.h>
+
+#include <string.h>
+
+#include "bpf_helpers.h"
+
+#define NUM_CGROUP_LEVELS	4
+
+struct bpf_map_def SEC("maps") result = {
+	.type = BPF_MAP_TYPE_ARRAY,
+	.key_size = sizeof(__u32),
+	.value_size = sizeof(__u32),
+	.max_entries = 2,
+};
+
+#define __fetch(x) (__u32)(&(x))
+
+static __u32 static_bss = 0;	/* Reloc reference to .bss section */
+static __u32 static_data = 42;	/* Reloc reference to .data section */
+
+/**
+ * Load a u32 value from a static variable into a map, for the userland test
+ * program to validate.
+ */
+SEC("static_data_load")
+int load_static_data(struct __sk_buff *skb)
+{
+	__u32 key, value;
+
+	key = 0;
+	value = __fetch(static_bss);
+	bpf_map_update_elem(&result, &key, &value, 0);
+
+	key = 1;
+	value = __fetch(static_data);
+	bpf_map_update_elem(&result, &key, &value, 0);
+
+	return TC_ACT_OK;
+}
+
+int _version SEC("version") = 1;
+
+char _license[] SEC("license") = "GPL";
-- 
2.19.1

Re: [PATCH bpf-next 4/4] selftests/bpf: Test static data relocation

[Alexei Starovoitov]

On Mon, Feb 11, 2019 at 04:47:29PM -0800, Joe Stringer wrote:
> Add tests for libbpf relocation of static variable references into the
> .data and .bss sections of the ELF.
> 
> Signed-off-by: Joe Stringer <joe@wand.net.nz>
...
> +#define __fetch(x) (__u32)(&(x))
> +
> +static __u32 static_bss = 0;	/* Reloc reference to .bss section */
> +static __u32 static_data = 42;	/* Reloc reference to .data section */
> +
> +/**
> + * Load a u32 value from a static variable into a map, for the userland test
> + * program to validate.
> + */
> +SEC("static_data_load")
> +int load_static_data(struct __sk_buff *skb)
> +{
> +	__u32 key, value;
> +
> +	key = 0;
> +	value = __fetch(static_bss);

If we proceed with this approach we will not be able to add support
for normal 'value = static_bss;' C code in the future.
Let's figure out the way to do it right from the start.
Support for global and static variables is must have feature to add asap,
but let's not cut the corner like this.
We did such hacks in the past and every time it came back to bite us.

Re: [PATCH bpf-next 4/4] selftests/bpf: Test static data relocation

[Joe Stringer]

On Mon, Feb 11, 2019 at 9:01 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Feb 11, 2019 at 04:47:29PM -0800, Joe Stringer wrote:
> > Add tests for libbpf relocation of static variable references into the
> > .data and .bss sections of the ELF.
> >
> > Signed-off-by: Joe Stringer <joe@wand.net.nz>
> ...
> > +#define __fetch(x) (__u32)(&(x))
> > +
> > +static __u32 static_bss = 0; /* Reloc reference to .bss section */
> > +static __u32 static_data = 42;       /* Reloc reference to .data section */
> > +
> > +/**
> > + * Load a u32 value from a static variable into a map, for the userland test
> > + * program to validate.
> > + */
> > +SEC("static_data_load")
> > +int load_static_data(struct __sk_buff *skb)
> > +{
> > +     __u32 key, value;
> > +
> > +     key = 0;
> > +     value = __fetch(static_bss);
>
> If we proceed with this approach we will not be able to add support
> for normal 'value = static_bss;' C code in the future.

Hmm, completely agree that breaking future use of standard code is a
non-starter.

Digging around a bit more, I think I could drop the .bss patch/code
here and still end up with something that will work for my use case.
Just need to ensure that all template values are non-zero when run
through the compiler.

> Let's figure out the way to do it right from the start.
> Support for global and static variables is must have feature to add asap,
> but let's not cut the corner like this.
> We did such hacks in the past and every time it came back to bite us.

Do you see any value in having incremental support in libbpf that
could be used as a fallback for older kernels like in patch #2 of this
series? I could imagine libbpf probing kernel support for
global/static variables and attempting to handle references to .data
via some more comprehensive mechanism in-kernel, or falling back to
this approach if it is not available.

Re: [PATCH bpf-next 4/4] selftests/bpf: Test static data relocation

[Alexei Starovoitov]

On Tue, Feb 12, 2019 at 12:43:21PM -0800, Joe Stringer wrote:
> 
> Do you see any value in having incremental support in libbpf that
> could be used as a fallback for older kernels like in patch #2 of this
> series? I could imagine libbpf probing kernel support for
> global/static variables and attempting to handle references to .data
> via some more comprehensive mechanism in-kernel, or falling back to
> this approach if it is not available.

I don't think we have to view it as older vs new kernel and fallback discussion.
I think access to static vars can be implemented in libbpf today without
changing llvm and kernel.

For the following code:
static volatile __u32 static_data = 42;

SEC("anything")
int load_static_data(struct __sk_buff *skb)
{
   __u32 value = static_data;

llvm will generate asm:

  r1 = static_data ll
  r1 = *(u32 *)(r1 + 0)

libbpf can replace first insn with r1 = 0 (or remove it altogether)
and second insn with r1 = 42 _when it is safe_.

If there was no volatile keyword llvm would have optimized
these two instructions into operation with immediate constant.
libbpf can do this opimization instead of llvm.
libbpf can check that 'static_data' is indeed not global in elf file
and there are no store operations in all programs in that elf file.
Then every load from that address can be replaced with rX=imm
of the value from data section.
libbpf would need to do data flow analysis which is substantial
feature addition. I think it's inevitable next step anyway.

The key point that this approach will be compatible with future
global variables and modifiable static variables.
In such case load/store instructions will stay as-is
and kernel support will be needed to replace 'r1 = static_data ll'
with properly marked ld_imm64 insn.