BPF Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH bpf] libbpf: fix global variable relocation
@ 2019-11-27  6:01 Andrii Nakryiko
  2019-11-27 18:19 ` Yonghong Song
  0 siblings, 1 reply; 3+ messages in thread
From: Andrii Nakryiko @ 2019-11-27  6:01 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko

Similarly to a0d7da26ce86 ("libbpf: Fix call relocation offset calculation
bug"), relocations against global variables need to take into account
referenced symbol's st_value, which holds offset into a corresponding data
section (and, subsequently, offset into internal backing map). For static
variables this offset is always zero and data offset is completely described
by respective instruction's imm field.

Convert a bunch of selftests to global variables. Previously they were relying
on `static volatile` trick to ensure Clang doesn't inline static variables,
which with global variables is not necessary anymore.

Fixes: 393cdfbee809 ("libbpf: Support initialized global variables")
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/lib/bpf/libbpf.c                        | 40 +++++++++----------
 .../testing/selftests/bpf/progs/fentry_test.c | 12 +++---
 .../selftests/bpf/progs/fexit_bpf2bpf.c       |  6 +--
 .../testing/selftests/bpf/progs/fexit_test.c  | 12 +++---
 tools/testing/selftests/bpf/progs/test_mmap.c |  4 +-
 5 files changed, 35 insertions(+), 39 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index b20f82e58989..4209b5a23a53 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -171,10 +171,8 @@ struct bpf_program {
 			RELO_DATA,
 		} type;
 		int insn_idx;
-		union {
-			int map_idx;
-			int text_off;
-		};
+		int map_idx;
+		int sym_off;
 	} *reloc_desc;
 	int nr_reloc;
 	int log_level;
@@ -1824,7 +1822,7 @@ static int bpf_program__record_reloc(struct bpf_program *prog,
 		}
 		reloc_desc->type = RELO_CALL;
 		reloc_desc->insn_idx = insn_idx;
-		reloc_desc->text_off = sym->st_value / 8;
+		reloc_desc->sym_off = sym->st_value;
 		obj->has_pseudo_calls = true;
 		return 0;
 	}
@@ -1868,6 +1866,7 @@ static int bpf_program__record_reloc(struct bpf_program *prog,
 		reloc_desc->type = RELO_LD64;
 		reloc_desc->insn_idx = insn_idx;
 		reloc_desc->map_idx = map_idx;
+		reloc_desc->sym_off = 0; /* sym->st_value determines map_idx */
 		return 0;
 	}
 
@@ -1899,6 +1898,7 @@ static int bpf_program__record_reloc(struct bpf_program *prog,
 	reloc_desc->type = RELO_DATA;
 	reloc_desc->insn_idx = insn_idx;
 	reloc_desc->map_idx = map_idx;
+	reloc_desc->sym_off = sym->st_value;
 	return 0;
 }
 
@@ -3563,8 +3563,8 @@ bpf_program__reloc_text(struct bpf_program *prog, struct bpf_object *obj,
 		return -LIBBPF_ERRNO__RELOC;
 
 	if (prog->idx == obj->efile.text_shndx) {
-		pr_warn("relo in .text insn %d into off %d\n",
-			relo->insn_idx, relo->text_off);
+		pr_warn("relo in .text insn %d into off %d (insn #%d)\n",
+			relo->insn_idx, relo->sym_off, relo->sym_off / 8);
 		return -LIBBPF_ERRNO__RELOC;
 	}
 
@@ -3599,7 +3599,7 @@ bpf_program__reloc_text(struct bpf_program *prog, struct bpf_object *obj,
 			 prog->section_name);
 	}
 	insn = &prog->insns[relo->insn_idx];
-	insn->imm += relo->text_off + prog->main_prog_cnt - relo->insn_idx;
+	insn->imm += relo->sym_off / 8 + prog->main_prog_cnt - relo->insn_idx;
 	return 0;
 }
 
@@ -3622,31 +3622,27 @@ bpf_program__relocate(struct bpf_program *prog, struct bpf_object *obj)
 		return 0;
 
 	for (i = 0; i < prog->nr_reloc; i++) {
+		struct reloc_desc *relo = &prog->reloc_desc[i];
+
 		if (prog->reloc_desc[i].type == RELO_LD64 ||
 		    prog->reloc_desc[i].type == RELO_DATA) {
-			bool relo_data = prog->reloc_desc[i].type == RELO_DATA;
-			struct bpf_insn *insns = prog->insns;
-			int insn_idx, map_idx;
-
-			insn_idx = prog->reloc_desc[i].insn_idx;
-			map_idx = prog->reloc_desc[i].map_idx;
+			struct bpf_insn *insn = &prog->insns[relo->insn_idx];
 
-			if (insn_idx + 1 >= (int)prog->insns_cnt) {
+			if (relo->insn_idx + 1 >= (int)prog->insns_cnt) {
 				pr_warn("relocation out of range: '%s'\n",
 					prog->section_name);
 				return -LIBBPF_ERRNO__RELOC;
 			}
 
-			if (!relo_data) {
-				insns[insn_idx].src_reg = BPF_PSEUDO_MAP_FD;
+			if (relo->type != RELO_DATA) {
+				insn[0].src_reg = BPF_PSEUDO_MAP_FD;
 			} else {
-				insns[insn_idx].src_reg = BPF_PSEUDO_MAP_VALUE;
-				insns[insn_idx + 1].imm = insns[insn_idx].imm;
+				insn[0].src_reg = BPF_PSEUDO_MAP_VALUE;
+				insn[1].imm = insn[0].imm + relo->sym_off;
 			}
-			insns[insn_idx].imm = obj->maps[map_idx].fd;
+			insn[0].imm = obj->maps[relo->map_idx].fd;
 		} else if (prog->reloc_desc[i].type == RELO_CALL) {
-			err = bpf_program__reloc_text(prog, obj,
-						      &prog->reloc_desc[i]);
+			err = bpf_program__reloc_text(prog, obj, relo);
 			if (err)
 				return err;
 		}
diff --git a/tools/testing/selftests/bpf/progs/fentry_test.c b/tools/testing/selftests/bpf/progs/fentry_test.c
index d2af9f039df5..615f7c6bca77 100644
--- a/tools/testing/selftests/bpf/progs/fentry_test.c
+++ b/tools/testing/selftests/bpf/progs/fentry_test.c
@@ -6,28 +6,28 @@
 
 char _license[] SEC("license") = "GPL";
 
-static volatile __u64 test1_result;
+__u64 test1_result = 0;
 BPF_TRACE_1("fentry/bpf_fentry_test1", test1, int, a)
 {
 	test1_result = a == 1;
 	return 0;
 }
 
-static volatile __u64 test2_result;
+__u64 test2_result = 0;
 BPF_TRACE_2("fentry/bpf_fentry_test2", test2, int, a, __u64, b)
 {
 	test2_result = a == 2 && b == 3;
 	return 0;
 }
 
-static volatile __u64 test3_result;
+__u64 test3_result = 0;
 BPF_TRACE_3("fentry/bpf_fentry_test3", test3, char, a, int, b, __u64, c)
 {
 	test3_result = a == 4 && b == 5 && c == 6;
 	return 0;
 }
 
-static volatile __u64 test4_result;
+__u64 test4_result = 0;
 BPF_TRACE_4("fentry/bpf_fentry_test4", test4,
 	    void *, a, char, b, int, c, __u64, d)
 {
@@ -35,7 +35,7 @@ BPF_TRACE_4("fentry/bpf_fentry_test4", test4,
 	return 0;
 }
 
-static volatile __u64 test5_result;
+__u64 test5_result = 0;
 BPF_TRACE_5("fentry/bpf_fentry_test5", test5,
 	    __u64, a, void *, b, short, c, int, d, __u64, e)
 {
@@ -44,7 +44,7 @@ BPF_TRACE_5("fentry/bpf_fentry_test5", test5,
 	return 0;
 }
 
-static volatile __u64 test6_result;
+__u64 test6_result = 0;
 BPF_TRACE_6("fentry/bpf_fentry_test6", test6,
 	    __u64, a, void *, b, short, c, int, d, void *, e, __u64, f)
 {
diff --git a/tools/testing/selftests/bpf/progs/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/progs/fexit_bpf2bpf.c
index 525d47d7b589..2d211ee98a1c 100644
--- a/tools/testing/selftests/bpf/progs/fexit_bpf2bpf.c
+++ b/tools/testing/selftests/bpf/progs/fexit_bpf2bpf.c
@@ -8,7 +8,7 @@ struct sk_buff {
 	unsigned int len;
 };
 
-static volatile __u64 test_result;
+__u64 test_result = 0;
 BPF_TRACE_2("fexit/test_pkt_access", test_main,
 	    struct sk_buff *, skb, int, ret)
 {
@@ -23,7 +23,7 @@ BPF_TRACE_2("fexit/test_pkt_access", test_main,
 	return 0;
 }
 
-static volatile __u64 test_result_subprog1;
+__u64 test_result_subprog1 = 0;
 BPF_TRACE_2("fexit/test_pkt_access_subprog1", test_subprog1,
 	    struct sk_buff *, skb, int, ret)
 {
@@ -56,7 +56,7 @@ struct args_subprog2 {
 	__u64 args[5];
 	__u64 ret;
 };
-static volatile __u64 test_result_subprog2;
+__u64 test_result_subprog2 = 0;
 SEC("fexit/test_pkt_access_subprog2")
 int test_subprog2(struct args_subprog2 *ctx)
 {
diff --git a/tools/testing/selftests/bpf/progs/fexit_test.c b/tools/testing/selftests/bpf/progs/fexit_test.c
index 2487e98edb34..86db0d60fb6e 100644
--- a/tools/testing/selftests/bpf/progs/fexit_test.c
+++ b/tools/testing/selftests/bpf/progs/fexit_test.c
@@ -6,28 +6,28 @@
 
 char _license[] SEC("license") = "GPL";
 
-static volatile __u64 test1_result;
+__u64 test1_result = 0;
 BPF_TRACE_2("fexit/bpf_fentry_test1", test1, int, a, int, ret)
 {
 	test1_result = a == 1 && ret == 2;
 	return 0;
 }
 
-static volatile __u64 test2_result;
+__u64 test2_result = 0;
 BPF_TRACE_3("fexit/bpf_fentry_test2", test2, int, a, __u64, b, int, ret)
 {
 	test2_result = a == 2 && b == 3 && ret == 5;
 	return 0;
 }
 
-static volatile __u64 test3_result;
+__u64 test3_result = 0;
 BPF_TRACE_4("fexit/bpf_fentry_test3", test3, char, a, int, b, __u64, c, int, ret)
 {
 	test3_result = a == 4 && b == 5 && c == 6 && ret == 15;
 	return 0;
 }
 
-static volatile __u64 test4_result;
+__u64 test4_result = 0;
 BPF_TRACE_5("fexit/bpf_fentry_test4", test4,
 	    void *, a, char, b, int, c, __u64, d, int, ret)
 {
@@ -37,7 +37,7 @@ BPF_TRACE_5("fexit/bpf_fentry_test4", test4,
 	return 0;
 }
 
-static volatile __u64 test5_result;
+__u64 test5_result = 0;
 BPF_TRACE_6("fexit/bpf_fentry_test5", test5,
 	    __u64, a, void *, b, short, c, int, d, __u64, e, int, ret)
 {
@@ -46,7 +46,7 @@ BPF_TRACE_6("fexit/bpf_fentry_test5", test5,
 	return 0;
 }
 
-static volatile __u64 test6_result;
+__u64 test6_result = 0;
 BPF_TRACE_7("fexit/bpf_fentry_test6", test6,
 	    __u64, a, void *, b, short, c, int, d, void *, e, __u64, f,
 	    int, ret)
diff --git a/tools/testing/selftests/bpf/progs/test_mmap.c b/tools/testing/selftests/bpf/progs/test_mmap.c
index 0d2ec9fbcf61..e808791b7047 100644
--- a/tools/testing/selftests/bpf/progs/test_mmap.c
+++ b/tools/testing/selftests/bpf/progs/test_mmap.c
@@ -15,8 +15,8 @@ struct {
 	__type(value, __u64);
 } data_map SEC(".maps");
 
-static volatile __u64 in_val;
-static volatile __u64 out_val;
+__u64 in_val = 0;
+__u64 out_val = 0;
 
 SEC("raw_tracepoint/sys_enter")
 int test_mmap(void *ctx)
-- 
2.17.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH bpf] libbpf: fix global variable relocation
  2019-11-27  6:01 [PATCH bpf] libbpf: fix global variable relocation Andrii Nakryiko
@ 2019-11-27 18:19 ` Yonghong Song
  2019-11-27 20:07   ` Andrii Nakryiko
  0 siblings, 1 reply; 3+ messages in thread
From: Yonghong Song @ 2019-11-27 18:19 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, netdev, Alexei Starovoitov, daniel
  Cc: andrii.nakryiko, Kernel Team



On 11/26/19 10:01 PM, Andrii Nakryiko wrote:
> Similarly to a0d7da26ce86 ("libbpf: Fix call relocation offset calculation
> bug"), relocations against global variables need to take into account
> referenced symbol's st_value, which holds offset into a corresponding data
> section (and, subsequently, offset into internal backing map). For static
> variables this offset is always zero and data offset is completely described
> by respective instruction's imm field.
> 
> Convert a bunch of selftests to global variables. Previously they were relying
> on `static volatile` trick to ensure Clang doesn't inline static variables,
> which with global variables is not necessary anymore.
> 
> Fixes: 393cdfbee809 ("libbpf: Support initialized global variables")
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>

LGTM with a few nits below.

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   tools/lib/bpf/libbpf.c                        | 40 +++++++++----------
>   .../testing/selftests/bpf/progs/fentry_test.c | 12 +++---
>   .../selftests/bpf/progs/fexit_bpf2bpf.c       |  6 +--
>   .../testing/selftests/bpf/progs/fexit_test.c  | 12 +++---
>   tools/testing/selftests/bpf/progs/test_mmap.c |  4 +-
>   5 files changed, 35 insertions(+), 39 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index b20f82e58989..4209b5a23a53 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -171,10 +171,8 @@ struct bpf_program {
>   			RELO_DATA,
>   		} type;
>   		int insn_idx;
> -		union {
> -			int map_idx;
> -			int text_off;
> -		};
> +		int map_idx;
> +		int sym_off;
>   	} *reloc_desc;
>   	int nr_reloc;
>   	int log_level;
[...]
> @@ -3622,31 +3622,27 @@ bpf_program__relocate(struct bpf_program *prog, struct bpf_object *obj)
>   		return 0;
>   
>   	for (i = 0; i < prog->nr_reloc; i++) {
> +		struct reloc_desc *relo = &prog->reloc_desc[i];
> +
>   		if (prog->reloc_desc[i].type == RELO_LD64 ||
>   		    prog->reloc_desc[i].type == RELO_DATA) {

Using relo->type == RELO_LD64 or RELO_DATA?

> -			bool relo_data = prog->reloc_desc[i].type == RELO_DATA;
> -			struct bpf_insn *insns = prog->insns;
> -			int insn_idx, map_idx;
> -
> -			insn_idx = prog->reloc_desc[i].insn_idx;
> -			map_idx = prog->reloc_desc[i].map_idx;
> +			struct bpf_insn *insn = &prog->insns[relo->insn_idx];
>   
> -			if (insn_idx + 1 >= (int)prog->insns_cnt) {
> +			if (relo->insn_idx + 1 >= (int)prog->insns_cnt) {
>   				pr_warn("relocation out of range: '%s'\n",
>   					prog->section_name);
>   				return -LIBBPF_ERRNO__RELOC;
>   			}
>   
> -			if (!relo_data) {
> -				insns[insn_idx].src_reg = BPF_PSEUDO_MAP_FD;
> +			if (relo->type != RELO_DATA) {
> +				insn[0].src_reg = BPF_PSEUDO_MAP_FD;
>   			} else {
> -				insns[insn_idx].src_reg = BPF_PSEUDO_MAP_VALUE;
> -				insns[insn_idx + 1].imm = insns[insn_idx].imm;
> +				insn[0].src_reg = BPF_PSEUDO_MAP_VALUE;
> +				insn[1].imm = insn[0].imm + relo->sym_off;
>   			}
> -			insns[insn_idx].imm = obj->maps[map_idx].fd;
> +			insn[0].imm = obj->maps[relo->map_idx].fd;
>   		} else if (prog->reloc_desc[i].type == RELO_CALL) {

Using relo->type == RELO_CALL?

> -			err = bpf_program__reloc_text(prog, obj,
> -						      &prog->reloc_desc[i]);
> +			err = bpf_program__reloc_text(prog, obj, relo);
>   			if (err)
>   				return err;
>   		}
> diff --git a/tools/testing/selftests/bpf/progs/fentry_test.c b/tools/testing/selftests/bpf/progs/fentry_test.c
> index d2af9f039df5..615f7c6bca77 100644
> --- a/tools/testing/selftests/bpf/progs/fentry_test.c
> +++ b/tools/testing/selftests/bpf/progs/fentry_test.c
> @@ -6,28 +6,28 @@
[...]

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH bpf] libbpf: fix global variable relocation
  2019-11-27 18:19 ` Yonghong Song
@ 2019-11-27 20:07   ` Andrii Nakryiko
  0 siblings, 0 replies; 3+ messages in thread
From: Andrii Nakryiko @ 2019-11-27 20:07 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Andrii Nakryiko, bpf, netdev, Alexei Starovoitov, daniel, Kernel Team

On Wed, Nov 27, 2019 at 10:19 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 11/26/19 10:01 PM, Andrii Nakryiko wrote:
> > Similarly to a0d7da26ce86 ("libbpf: Fix call relocation offset calculation
> > bug"), relocations against global variables need to take into account
> > referenced symbol's st_value, which holds offset into a corresponding data
> > section (and, subsequently, offset into internal backing map). For static
> > variables this offset is always zero and data offset is completely described
> > by respective instruction's imm field.
> >
> > Convert a bunch of selftests to global variables. Previously they were relying
> > on `static volatile` trick to ensure Clang doesn't inline static variables,
> > which with global variables is not necessary anymore.
> >
> > Fixes: 393cdfbee809 ("libbpf: Support initialized global variables")
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>
> LGTM with a few nits below.
>

Good points, I was too laser-focused. Updated in v2, thanks!

> Acked-by: Yonghong Song <yhs@fb.com>
>
> > ---
> >   tools/lib/bpf/libbpf.c                        | 40 +++++++++----------
> >   .../testing/selftests/bpf/progs/fentry_test.c | 12 +++---
> >   .../selftests/bpf/progs/fexit_bpf2bpf.c       |  6 +--
> >   .../testing/selftests/bpf/progs/fexit_test.c  | 12 +++---
> >   tools/testing/selftests/bpf/progs/test_mmap.c |  4 +-
> >   5 files changed, 35 insertions(+), 39 deletions(-)
> >

[...]

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-27  6:01 [PATCH bpf] libbpf: fix global variable relocation Andrii Nakryiko
2019-11-27 18:19 ` Yonghong Song
2019-11-27 20:07   ` Andrii Nakryiko

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git