Re: [PATCH v2 bpf-next 1/2] libbpf: load global data maps lazily on legacy kernels

[Alan Maguire <alan.maguire@oracle.com>]

On Tue, 23 Nov 2021, Andrii Nakryiko wrote:

> Load global data maps lazily, if kernel is too old to support global
> data. Make sure that programs are still correct by detecting if any of
> the to-be-loaded programs have relocation against any of such maps.
> 
> This allows to solve the issue ([0]) with bpf_printk() and Clang
> generating unnecessary and unreferenced .rodata.strX.Y sections, but it
> also goes further along the CO-RE lines, allowing to have a BPF object
> in which some code can work on very old kernels and relies only on BPF
> maps explicitly, while other BPF programs might enjoy global variable
> support. If such programs are correctly set to not load at runtime on
> old kernels, bpf_object will load and function correctly now.
> 
>   [0] https://lore.kernel.org/bpf/CAK-59YFPU3qO+_pXWOH+c1LSA=8WA1yabJZfREjOEXNHAqgXNg@mail.gmail.com/
> 
> Fixes: aed659170a31 ("libbpf: Support multiple .rodata.* and .data.* BPF maps")
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>

Thanks for fixing this! I ran into it today on a 4.14 kernel and verified 
that with the patch applied to latest libbpf, BPF objects with legacy maps 
loaded and ran where previously loading failed.

Tested-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  tools/lib/bpf/libbpf.c | 34 ++++++++++++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index af405c38aadc..27695bf31250 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -5006,6 +5006,24 @@ bpf_object__create_maps(struct bpf_object *obj)
>  	for (i = 0; i < obj->nr_maps; i++) {
>  		map = &obj->maps[i];
>  
> +		/* To support old kernels, we skip creating global data maps
> +		 * (.rodata, .data, .kconfig, etc); later on, during program
> +		 * loading, if we detect that at least one of the to-be-loaded
> +		 * programs is referencing any global data map, we'll error
> +		 * out with program name and relocation index logged.
> +		 * This approach allows to accommodate Clang emitting
> +		 * unnecessary .rodata.str1.1 sections for string literals,
> +		 * but also it allows to have CO-RE applications that use
> +		 * global variables in some of BPF programs, but not others.
> +		 * If those global variable-using programs are not loaded at
> +		 * runtime due to bpf_program__set_autoload(prog, false),
> +		 * bpf_object loading will succeed just fine even on old
> +		 * kernels.
> +		 */
> +		if (bpf_map__is_internal(map) &&
> +		    !kernel_supports(obj, FEAT_GLOBAL_DATA))
> +			continue;
> +
>  		retried = false;
>  retry:
>  		if (map->pin_path) {
> @@ -5605,6 +5623,14 @@ bpf_object__relocate_data(struct bpf_object *obj, struct bpf_program *prog)
>  				insn[0].src_reg = BPF_PSEUDO_MAP_IDX_VALUE;
>  				insn[0].imm = relo->map_idx;
>  			} else {
> +				const struct bpf_map *map = &obj->maps[relo->map_idx];
> +
> +				if (bpf_map__is_internal(map) &&
> +				    !kernel_supports(obj, FEAT_GLOBAL_DATA)) {
> +					pr_warn("prog '%s': relo #%d: kernel doesn't support global data\n",
> +						prog->name, i);
> +					return -ENOTSUP;
> +				}
>  				insn[0].src_reg = BPF_PSEUDO_MAP_VALUE;
>  				insn[0].imm = obj->maps[relo->map_idx].fd;
>  			}
> @@ -6139,6 +6165,8 @@ bpf_object__relocate(struct bpf_object *obj, const char *targ_btf_path)
>  		 */
>  		if (prog_is_subprog(obj, prog))
>  			continue;
> +		if (!prog->load)
> +			continue;
>  
>  		err = bpf_object__relocate_calls(obj, prog);
>  		if (err) {
> @@ -6152,6 +6180,8 @@ bpf_object__relocate(struct bpf_object *obj, const char *targ_btf_path)
>  		prog = &obj->programs[i];
>  		if (prog_is_subprog(obj, prog))
>  			continue;
> +		if (!prog->load)
> +			continue;
>  		err = bpf_object__relocate_data(obj, prog);
>  		if (err) {
>  			pr_warn("prog '%s': failed to relocate data references: %d\n",
> @@ -6939,10 +6969,6 @@ static int bpf_object__sanitize_maps(struct bpf_object *obj)
>  	bpf_object__for_each_map(m, obj) {
>  		if (!bpf_map__is_internal(m))
>  			continue;
> -		if (!kernel_supports(obj, FEAT_GLOBAL_DATA)) {
> -			pr_warn("kernel doesn't support global data\n");
> -			return -ENOTSUP;
> -		}
>  		if (!kernel_supports(obj, FEAT_ARRAY_MMAP))
>  			m->def.map_flags ^= BPF_F_MMAPABLE;
>  	}
> -- 
> 2.30.2
> 
>