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

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

On 1/18/24 14:18, Martin KaFai Lau wrote:
> 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));

Got it!
> 
>> +            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);
>