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

[Kui-Feng Lee <sinquersw@gmail.com>]

On 9/25/23 16:23, Martin KaFai Lau wrote:
> 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.

At this point, it holds btf, but not module. Module can go away while
some one still holding a refcount to the btf.

And, you are right, I should use btf_try_get_module().

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