Re: [PATCH bpf-next v8 06/10] bpf: pass attached BTF to the bpf_struct_ops subsystem

[Martin KaFai Lau <martin.lau@linux.dev>]

On 10/30/23 12:28 PM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> Giving a BTF, the bpf_struct_ops knows the right place to look up type info
> associated with a type ID. This enables a user space program to load a
> struct_ops object linked to a struct_ops type defined by a module, by
> providing the module BTF (fd).

This describes about the struct_ops map creation change (by adding 
value_type_btf_obj_fd)? It could be described more clearly in the commit 
message, like specify the value_type_btf_obj_fd addition and how it is used in 
the struct_ops map creation.

> 
> The bpf_prog includes attach_btf in aux which is passed along with the
> bpf_attr when loading the program. The purpose of attach_btf is to
> determine the btf type of attach_btf_id. The attach_btf_id is then used to
> identify the traced function for a trace program. In the case of struct_ops
> programs, it is used to identify the struct_ops type of the struct_ops
> object that a program is attached to.

Does attach_btf_obj_fd also work?

[ ... ]

> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 256516aba632..db2bbba50e38 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -694,6 +694,7 @@ static void __bpf_struct_ops_map_free(struct bpf_map *map)
>   		bpf_jit_uncharge_modmem(PAGE_SIZE);
>   	}
>   	bpf_map_area_free(st_map->uvalue);
> +	btf_put(st_map->st_ops_desc->btf);
>   	bpf_map_area_free(st_map);
>   }
>   
> @@ -735,16 +736,31 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>   	const struct btf_type *t, *vt;
>   	struct module *mod = NULL;
>   	struct bpf_map *map;
> +	struct btf *btf;
>   	int ret;
>   
> -	st_ops_desc = bpf_struct_ops_find_value(btf_vmlinux, attr->btf_vmlinux_value_type_id);
> -	if (!st_ops_desc)
> -		return ERR_PTR(-ENOTSUPP);
> +	if (attr->value_type_btf_obj_fd) {
> +		/* The map holds btf for its whole life time. */

It took me a while to parse this comment and connect it with the 
btf_put(st_map->st_ops_desc->btf) in the __bpf_struct_ops_map_free() above.

It is now like "btf" owns "struct_ops_desc" which also stores a pointer pointing 
back to itself, like "btf->struct_ops_desc->btf". The struct_ops_desc->btf was 
not initialized by st_map but st_map will increment its refcount much later.

Can btf be directly stored in the st_map->btf instead and map_alloc holds the 
refcnt of st_map->btf and btf_put(st_map->btf) in map_free?


> +		btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
> +		if (IS_ERR(btf))
> +			return ERR_PTR(PTR_ERR(btf));
> +
> +		if (btf != btf_vmlinux) {
> +			mod = btf_try_get_module(btf);
> +			if (!mod) {
> +				ret = -EINVAL;
> +				goto errout;
> +			}
> +		}
> +	} else {
> +		btf = btf_vmlinux;
> +		btf_get(btf);
> +	}
>   
> -	if (st_ops_desc->btf != btf_vmlinux) {
> -		mod = btf_try_get_module(st_ops_desc->btf);
> -		if (!mod)
> -			return ERR_PTR(-EINVAL);
> +	st_ops_desc = bpf_struct_ops_find_value(btf, attr->btf_vmlinux_value_type_id);
> +	if (!st_ops_desc) {
> +		ret = -ENOTSUPP;
> +		goto errout;
>   	}
>   
>   	vt = st_ops_desc->value_type;