Re: [RFC bpf-next v3 05/11] bpf: hold module for bpf_struct_ops_map.

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

On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> Ensure a module doesn't go away when a struct_ops object is still alive,
> being a struct_ops type that is registered by the module.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>   include/linux/bpf.h         | 1 +
>   kernel/bpf/bpf_struct_ops.c | 6 ++++++
>   2 files changed, 7 insertions(+)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 0776cb584b3f..faaec20156f1 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1627,6 +1627,7 @@ struct bpf_struct_ops {
>   	int (*update)(void *kdata, void *old_kdata);
>   	int (*validate)(void *kdata);
>   	const struct btf *btf;
> +	struct module *owner;
>   	const struct btf_type *type;
>   	const struct btf_type *value_type;
>   	const char *name;
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 7c2ef53687ef..ef8a1edec891 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -632,6 +632,8 @@ static void __bpf_struct_ops_map_free(struct bpf_map *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;
> +
>   	/* The struct_ops's function may switch to another struct_ops.
>   	 *
>   	 * For example, bpf_tcp_cc_x->init() may switch to
> @@ -649,6 +651,7 @@ static void bpf_struct_ops_map_free(struct bpf_map *map)
>   	 */
>   	synchronize_rcu_mult(call_rcu, call_rcu_tasks);
>   
> +	module_put(st_map->st_ops->owner);
>   	__bpf_struct_ops_map_free(map);
>   }
>   
> @@ -673,6 +676,9 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>   	if (!st_ops)
>   		return ERR_PTR(-ENOTSUPP);
>   
> +	if (!try_module_get(st_ops->owner))
> +		return ERR_PTR(-EINVAL);

The module can be gone at this point?
I don't think try_module_get is safe. btf_try_get_module should be used instead.

> +
>   	vt = st_ops->value_type;
>   	if (attr->value_size != vt->size)
>   		return ERR_PTR(-EINVAL);