Re: [PATCH bpf-next v16 09/14] bpf: hold module refcnt in bpf_struct_ops map creation and prog verification.

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

On 1/17/24 5:49 PM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> To ensure that a module remains accessible whenever a struct_ops object of
> a struct_ops type provided by the module is still in use.
> 
> struct bpf_struct_ops_map doesn't hold a refcnt to btf anymore since a
> module will hold a refcnt to it's btf already. But, struct_ops programs are
> different. They hold their associated btf, not the module since they need
> only btf to assure their types (signatures).
> 
> However, verifier holds the refcnt of the associated module of a struct_ops
> type temporarily when verify a struct_ops prog. Verifier needs the help
> from the verifier operators (struct bpf_verifier_ops) provided by the owner
> module to verify data access of a prog, provide information, and generate
> code.
> 
> This patch also add a count of links (links_cnt) to bpf_struct_ops_map. It
> avoids bpf_struct_ops_map_put_progs() from accessing btf after calling
> module_put() in bpf_struct_ops_map_free().

Good catch in v16.

> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>   include/linux/bpf.h          |  1 +
>   include/linux/bpf_verifier.h |  1 +
>   kernel/bpf/bpf_struct_ops.c  | 31 +++++++++++++++++++++++++------
>   kernel/bpf/verifier.c        | 10 ++++++++++
>   4 files changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 3d1c1014fdb2..a977ed75288c 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1674,6 +1674,7 @@ struct bpf_struct_ops {
>   	int (*update)(void *kdata, void *old_kdata);
>   	int (*validate)(void *kdata);
>   	void *cfi_stubs;
> +	struct module *owner;
>   	const char *name;
>   	struct btf_func_model func_models[BPF_STRUCT_OPS_MAX_NR_MEMBERS];
>   };
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index d07d857ca67f..e6cf025c9446 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -662,6 +662,7 @@ struct bpf_verifier_env {
>   	u32 prev_insn_idx;
>   	struct bpf_prog *prog;		/* eBPF program being verified */
>   	const struct bpf_verifier_ops *ops;
> +	struct module *attach_btf_mod;	/* The owner module of prog->aux->attach_btf */
>   	struct bpf_verifier_stack_elem *head; /* stack of verifier states to be processed */
>   	int stack_size;			/* number of states to be processed */
>   	bool strict_alignment;		/* perform strict pointer alignment checks */
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 3b8d689ece5d..61486f6595ea 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -40,6 +40,7 @@ struct bpf_struct_ops_map {
>   	 * (in kvalue.data).
>   	 */
>   	struct bpf_link **links;
> +	u32 links_cnt;
>   	/* image is a page that has all the trampolines
>   	 * that stores the func args before calling the bpf_prog.
>   	 * A PAGE_SIZE "image" is enough to store all trampoline for
> @@ -306,10 +307,9 @@ static void *bpf_struct_ops_map_lookup_elem(struct bpf_map *map, void *key)
>   
>   static void bpf_struct_ops_map_put_progs(struct bpf_struct_ops_map *st_map)
>   {
> -	const struct btf_type *t = st_map->st_ops_desc->type;
>   	u32 i;
>   
> -	for (i = 0; i < btf_type_vlen(t); i++) {
> +	for (i = 0; i < st_map->links_cnt; i++) {
>   		if (st_map->links[i]) {
>   			bpf_link_put(st_map->links[i]);
>   			st_map->links[i] = NULL;
> @@ -641,12 +641,20 @@ 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->btf);
>   	bpf_map_area_free(st_map);
>   }
>   
>   static void bpf_struct_ops_map_free(struct bpf_map *map)
>   {
> +	struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map *)map;
> +
> +	/* st_ops->owner was acquired during map_alloc to implicitly holds
> +	 * the btf's refcnt. The acquire was only done when btf_is_module()
> +	 * st_map->btf cannot be NULL here.
> +	 */
> +	if (btf_is_module(st_map->btf))
> +		module_put(st_map->st_ops_desc->st_ops->owner);
> +
>   	/* The struct_ops's function may switch to another struct_ops.
>   	 *
>   	 * For example, bpf_tcp_cc_x->init() may switch to
> @@ -682,6 +690,7 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>   	size_t st_map_size;
>   	struct bpf_struct_ops_map *st_map;
>   	const struct btf_type *t, *vt;
> +	struct module *mod = NULL;
>   	struct bpf_map *map;
>   	struct btf *btf;
>   	int ret;
> @@ -695,11 +704,20 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>   			btf_put(btf);
>   			return ERR_PTR(-EINVAL);
>   		}
> +
> +		mod = btf_try_get_module(btf);

nit. btf_put(btf) here.

> +		if (!mod) {
> +			btf_put(btf);
> +			return ERR_PTR(-EINVAL);
> +		}
> +		/* mod holds a refcnt to btf. We don't need an extra refcnt
> +		 * here.
> +		 */
> +		btf_put(btf);
>   	} else {
>   		btf = bpf_get_btf_vmlinux();
>   		if (IS_ERR(btf))
>   			return ERR_CAST(btf);
> -		btf_get(btf);
>   	}
>   
>   	st_ops_desc = bpf_struct_ops_find_value(btf, attr->btf_vmlinux_value_type_id);
> @@ -746,8 +764,9 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>   		goto errout_free;
>   	}
>   	st_map->uvalue = bpf_map_area_alloc(vt->size, NUMA_NO_NODE);
> +	st_map->links_cnt = btf_type_vlen(t);
>   	st_map->links =
> -		bpf_map_area_alloc(btf_type_vlen(t) * sizeof(struct bpf_links *),
> +		bpf_map_area_alloc(st_map->links_cnt * sizeof(struct bpf_links *),
>   				   NUMA_NO_NODE);
>   	if (!st_map->uvalue || !st_map->links) {
>   		ret = -ENOMEM;
> @@ -763,7 +782,7 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>   errout_free:
>   	__bpf_struct_ops_map_free(map);
>   errout:
> -	btf_put(btf);
> +	module_put(mod);
>   
>   	return ERR_PTR(ret);
>   }
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index ff41f7736618..60f08f468399 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -20243,6 +20243,14 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
>   	}
>   
>   	btf = prog->aux->attach_btf ?: bpf_get_btf_vmlinux();
> +	if (btf_is_module(btf)) {
> +		/* Make sure st_ops is valid through the lifetime of env */
> +		env->attach_btf_mod = btf_try_get_module(btf);
> +		if (!env->attach_btf_mod) {
> +			verbose(env, "owner module of btf is not found\n");

nit. A better message, something like:

			verbose(env, "struct_ops module %s is not found\n",
				btf_get_name(btf));

> +			return -ENOTSUPP;
> +		}
> +	}
>   
>   	btf_id = prog->aux->attach_btf_id;
>   	st_ops_desc = bpf_struct_ops_find(btf, btf_id);
> @@ -20968,6 +20976,8 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3
>   		env->prog->expected_attach_type = 0;
>   
>   	*prog = env->prog;
> +
> +	module_put(env->attach_btf_mod);
>   err_unlock:
>   	if (!is_priv)
>   		mutex_unlock(&bpf_verifier_lock);