netdev - Re: [PATCH bpf] libbpf: fix global variable relocation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ce16b691-8afa-2e0c-6a99-ec509a125115@fb.com>
Date:   Wed, 27 Nov 2019 18:19:42 +0000
From:   Yonghong Song <yhs@...com>
To:     Andrii Nakryiko <andriin@...com>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        "daniel@...earbox.net" <daniel@...earbox.net>
CC:     "andrii.nakryiko@...il.com" <andrii.nakryiko@...il.com>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf] libbpf: fix global variable relocation



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@...com>

LGTM with a few nits below.

Acked-by: Yonghong Song <yhs@...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 @@
[...]