Re: [RFC bpf-next v3 08/11] bpf: pass attached BTF to find correct type info of struct_ops progs.

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

On 9/25/23 15:58, Andrii Nakryiko wrote:
> On Wed, Sep 20, 2023 at 9:00 AM <thinker.li@gmail.com> wrote:
>>
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> The type info of a struct_ops type may be in a module.  So, we need to know
>> which module BTF to look for type information.  The later patches will make
>> libbpf to attach module BTFs to programs. This patch passes attached BTF
>> from syscall to bpf_struct_ops subsystem to make sure attached BTF is
>> available when the bpf_struct_ops subsystem is ready to use it.
>>
>> bpf_prog has attach_btf in aux from attach_btf_obj_fd, that is pass along
>> with the bpf_attr loading the program. attach_btf is used to find the btf
>> type of attach_btf_id. attach_btf_id is used to identify the traced
>> function for a trace program.  For struct_ops programs, it is used to
>> identify the struct_ops type of the struct_ops object a program attached
>> to.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   include/uapi/linux/bpf.h       |  4 ++++
>>   kernel/bpf/bpf_struct_ops.c    | 12 +++++++++++-
>>   kernel/bpf/syscall.c           |  2 +-
>>   kernel/bpf/verifier.c          |  4 +++-
>>   tools/include/uapi/linux/bpf.h |  4 ++++
>>   5 files changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 73b155e52204..178d6fa45fa0 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -1390,6 +1390,10 @@ union bpf_attr {
>>                   * to using 5 hash functions).
>>                   */
>>                  __u64   map_extra;
>> +
>> +               __u32   mod_btf_fd;     /* fd pointing to a BTF type data
>> +                                        * for btf_vmlinux_value_type_id.
>> +                                        */
> 
> we have attach_btf_obj_fd for BPF_PROG_LOAD command, so I guess
> consistent naming would be "<something>_btf_obj_fd" where <something>
> would make it more-or-less clear that this is BTF for
> btf_vmlinux_value_type_id?

Got it! I will rename it to value_type_btf_obj_fd.

> 
>>          };
>>
>>          struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index 8b5c859377e9..d5600d9ad302 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -765,9 +765,19 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>>          struct bpf_struct_ops_map *st_map;
>>          const struct btf_type *t, *vt;
>>          struct bpf_map *map;
>> +       struct btf *btf;
>>          int ret;
>>
>> -       st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf_vmlinux);
>> +       /* XXX: We need a module name or ID to find a BTF type. */
>> +       /* XXX: should use btf from attr->btf_fd */
> 
> Do we need these XXX: comments? I think you had some more in previous patches

Will be removed.

> 
>> +       if (attr->mod_btf_fd) {
>> +               btf = btf_get_by_fd(attr->mod_btf_fd);
>> +               if (IS_ERR(btf))
>> +                       return ERR_PTR(PTR_ERR(btf));
>> +       } else {
>> +               btf = btf_vmlinux;
>> +       }
>> +       st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf);
>>          if (!st_ops)
>>                  return ERR_PTR(-ENOTSUPP);
> 
> should we make sure that module's BTF is put properly on error?

Yes, this issue has been addressed locally.

> 
>>
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index 85c1d908f70f..fed3870fec7a 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -1097,7 +1097,7 @@ static int map_check_btf(struct bpf_map *map, const struct btf *btf,
>>          return ret;
>>   }
>>
>> -#define BPF_MAP_CREATE_LAST_FIELD map_extra
>> +#define BPF_MAP_CREATE_LAST_FIELD mod_btf_fd
>>   /* called via syscall */
>>   static int map_create(union bpf_attr *attr)
>>   {
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 99b45501951c..11f85dbc911b 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -19623,6 +19623,7 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
>>          const struct btf_member *member;
>>          struct bpf_prog *prog = env->prog;
>>          u32 btf_id, member_idx;
>> +       struct btf *btf;
>>          const char *mname;
>>
>>          if (!prog->gpl_compatible) {
>> @@ -19630,8 +19631,9 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
>>                  return -EINVAL;
>>          }
>>
>> +       btf = prog->aux->attach_btf;
>>          btf_id = prog->aux->attach_btf_id;
>> -       st_ops = bpf_struct_ops_find(btf_id, btf_vmlinux);
>> +       st_ops = bpf_struct_ops_find(btf_id, btf);
>>          if (!st_ops) {
>>                  verbose(env, "attach_btf_id %u is not a supported struct\n",
>>                          btf_id);
>> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
>> index 73b155e52204..178d6fa45fa0 100644
>> --- a/tools/include/uapi/linux/bpf.h
>> +++ b/tools/include/uapi/linux/bpf.h
>> @@ -1390,6 +1390,10 @@ union bpf_attr {
>>                   * to using 5 hash functions).
>>                   */
>>                  __u64   map_extra;
>> +
>> +               __u32   mod_btf_fd;     /* fd pointing to a BTF type data
>> +                                        * for btf_vmlinux_value_type_id.
>> +                                        */
>>          };
>>
>>          struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
>> --
>> 2.34.1
>>