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

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

On 10/30/23 18:53, Martin KaFai Lau wrote:
> 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.


I will rephrase this part as the following words.

Every kernel module has its BTF, comprising information on types defined
in the module. The BTF fd (attr->value_type_btf_obj_fd) passed from
userspace helps the bpf_struct_ops to lookup type information and
description of the struct_ops type, which is necessary for parsing the
layout of map element values and registering maps. The descriptions are
looked up by matching a type id (attr->btf_vmlinux_value_type_id)
against bpf_struct_ops_desc(s) defined in a BTF. If a struct_ops type
is defined in a module, the bpf_struct_ops needs to know the module BTF
to lookup the bpf_struct_ops_desc.

> 
>>
>> 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?

aux->attach_btf is from attach_btf_obj_fd, being set by bpf_prog_load().
I will rephrase it to make it clear.

   The bpf_prog includes attach_btf in aux which is passed along with the
   bpf_attr (attr->attach_btf_obj_fd) when loading the program.

> 
> [ ... ]
> 
>> 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?

This topic has been discussed several times.
I will add a pointer to btf in st_map to make
people happy :)

> 
> 
>> +        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;
>