Re: [PATCH bpf-next v1 2/8] libbpf: tie struct_ops programs to kernel BTF ids, not to local ids

[David Vernet <void@manifault.com>]

[-- Attachment #1: Type: text/plain, Size: 4178 bytes --]

On Tue, Feb 27, 2024 at 10:45:50PM +0200, Eduard Zingerman wrote:
> Enforce the following existing limitation on struct_ops programs based
> on kernel BTF id instead of program-local BTF id:
> 
>     struct_ops BPF prog can be re-used between multiple .struct_ops &
>     .struct_ops.link as long as it's the same struct_ops struct
>     definition and the same function pointer field

Am I correct in understanding the code that the prog also has to be at the same
offset in the new type? So if we have for example:

SEC("struct_ops/test")
int BPF_PROG(foo) { ... }

struct some_ops___v1 {
	int (*test)(void);
};

struct some_ops___v2 {
	int (*init)(void);
	int (*test)(void);
};

Then this wouldn't work? If so, would it be possible for libbpf to do something
like invisibly duplicate the prog and create a separate one for each struct_ops
map where it's encountered? It feels like a rather awkward restriction to
impose given that the idea behind the feature is to enable loading one of
multiple possible definitions of a struct_ops type.

> 
> This allows reusing same BPF program for versioned struct_ops map
> definitions, e.g.:
> 
>     SEC("struct_ops/test")
>     int BPF_PROG(foo) { ... }
> 
>     struct some_ops___v1 { int (*test)(void); };
>     struct some_ops___v2 { int (*test)(void); };
> 
>     SEC(".struct_ops.link") struct some_ops___v1 a = { .test = foo }
>     SEC(".struct_ops.link") struct some_ops___v2 b = { .test = foo }
> 
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  tools/lib/bpf/libbpf.c | 44 ++++++++++++++++++++----------------------
>  1 file changed, 21 insertions(+), 23 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index abe663927013..c239b75d5816 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1134,8 +1134,27 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
>  
>  			if (mod_btf)
>  				prog->attach_btf_obj_fd = mod_btf->fd;
> -			prog->attach_btf_id = kern_type_id;
> -			prog->expected_attach_type = kern_member_idx;
> +
> +			/* if we haven't yet processed this BPF program, record proper
> +			 * attach_btf_id and member_idx
> +			 */
> +			if (!prog->attach_btf_id) {
> +				prog->attach_btf_id = kern_type_id;
> +				prog->expected_attach_type = kern_member_idx;
> +			}
> +
> +			/* struct_ops BPF prog can be re-used between multiple
> +			 * .struct_ops & .struct_ops.link as long as it's the
> +			 * same struct_ops struct definition and the same
> +			 * function pointer field
> +			 */
> +			if (prog->attach_btf_id != kern_type_id ||
> +			    prog->expected_attach_type != kern_member_idx) {
> +				pr_warn("struct_ops reloc %s: cannot use prog %s in sec %s with type %u attach_btf_id %u expected_attach_type %u for func ptr %s\n",
> +					map->name, prog->name, prog->sec_name, prog->type,
> +					prog->attach_btf_id, prog->expected_attach_type, mname);
> +				return -EINVAL;
> +			}
>  
>  			st_ops->kern_func_off[i] = kern_data_off + kern_moff;
>  
> @@ -9409,27 +9428,6 @@ static int bpf_object__collect_st_ops_relos(struct bpf_object *obj,
>  			return -EINVAL;
>  		}
>  
> -		/* if we haven't yet processed this BPF program, record proper
> -		 * attach_btf_id and member_idx
> -		 */
> -		if (!prog->attach_btf_id) {
> -			prog->attach_btf_id = st_ops->type_id;
> -			prog->expected_attach_type = member_idx;
> -		}
> -
> -		/* struct_ops BPF prog can be re-used between multiple
> -		 * .struct_ops & .struct_ops.link as long as it's the
> -		 * same struct_ops struct definition and the same
> -		 * function pointer field
> -		 */
> -		if (prog->attach_btf_id != st_ops->type_id ||
> -		    prog->expected_attach_type != member_idx) {
> -			pr_warn("struct_ops reloc %s: cannot use prog %s in sec %s with type %u attach_btf_id %u expected_attach_type %u for func ptr %s\n",
> -				map->name, prog->name, prog->sec_name, prog->type,
> -				prog->attach_btf_id, prog->expected_attach_type, name);
> -			return -EINVAL;
> -		}
> -
>  		st_ops->progs[member_idx] = prog;
>  	}
>  
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]