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

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

Comments

Joe Stringer - Feb. 12, 2019, 12:47 a.m.
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
Alexei Starovoitov - Feb. 12, 2019, 5:01 a.m.
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.
Joe Stringer - Feb. 12, 2019, 8:43 p.m.
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.
Alexei Starovoitov - Feb. 14, 2019, 12:35 a.m.
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.

Patch

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";