Re: [PATCH perf/core 1/6] tools lib bpf: Fix map offsets in relocation

[Wangnan (F)]

On 2017/1/19 7:57, Joe Stringer wrote:
> Commit 4708bbda5cb2 ("tools lib bpf: Fix maps resolution") attempted to
> fix map resolution by identifying the number of symbols that point to
> maps, and using this number to resolve each of the maps.
>
> However, during relocation the original definition of the map size was
> still in use.


Oops... Didn't realize that.


>   For up to two maps, the calculation was correct if there
> was a small difference in size between the map definition in libbpf and
> the one that the client library uses. However if the difference was
> large, particularly if more than two maps were used in the BPF program,
> the relocation would fail.
>
> For example, when using a map definition with size 28, with three maps,
> map relocation would count
>      (sym_offset / sizeof(struct bpf_map_def) => map_idx)
>      (0 / 16 => 0), ie map_idx = 0
>      (28 / 16 => 1), ie map_idx = 1
>      (56 / 16 => 3), ie map_idx = 3
>
> So, libbpf reports:
>      libbpf: bpf relocation: map_idx 3 large than 2

Understand.

> Fix map relocation by tracking the size of the map definition during
> map counting, then reuse this instead of the size of the libbpf map
> structure.

Then we add a restriction that map size is fixed
in one object, but we don't have to. During relocating,
we can get exact map positioning information from
sym.st_value. I think finding a map with exact offset
from the map array would be better.

I'll write a patch to replace this one.

Thank you.

>   With this patch applied, libbpf will accept such ELFs.
>
> Fixes: 4708bbda5cb2 ("tools lib bpf: Fix maps resolution")
> Signed-off-by: Joe Stringer <joe@xxxxxxx>
> ---
>   tools/lib/bpf/libbpf.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 84e6b35da4bd..350ee4c59f85 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -201,6 +201,7 @@ struct bpf_object {
>   	size_t nr_programs;
>   	struct bpf_map *maps;
>   	size_t nr_maps;
> +	size_t map_len;
>   
>   	bool loaded;
>   
> @@ -379,6 +380,7 @@ static struct bpf_object *bpf_object__new(const char *path,
>   	obj->efile.maps_shndx = -1;
>   
>   	obj->loaded = false;
> +	obj->map_len = sizeof(struct bpf_map_def);
>   
>   	INIT_LIST_HEAD(&obj->list);
>   	list_add(&obj->list, &bpf_objects_list);
> @@ -602,6 +604,7 @@ bpf_object__init_maps(struct bpf_object *obj)
>   		return -ENOMEM;
>   	}
>   	obj->nr_maps = nr_maps;
> +	obj->map_len = data->d_size / nr_maps;
>   
>   	/*
>   	 * fill all fd with -1 so won't close incorrect
> @@ -829,7 +832,7 @@ bpf_program__collect_reloc(struct bpf_program *prog,
>   			return -LIBBPF_ERRNO__RELOC;
>   		}
>   
> -		map_idx = sym.st_value / sizeof(struct bpf_map_def);
> +		map_idx = sym.st_value / prog->obj->map_len;
>   		if (map_idx >= nr_maps) {
>   			pr_warning("bpf relocation: map_idx %d large than %d\n",
>   				   (int)map_idx, (int)nr_maps - 1);