Re: [RFC bpf-next v2 1/3] libbpf: Create a shadow copy for each struct_ops map if necessary.

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

On 2/15/24 15:55, Andrii Nakryiko wrote:
> On Tue, Feb 13, 2024 at 6:08 PM <thinker.li@gmail.com> wrote:
>>
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> If the user has passed a shadow info for a struct_ops map along with struct
>> bpf_object_open_opts, a shadow copy will be created for the map and
>> returned from bpf_map__initial_value().
>>
>> The user can read and write shadow variables through the shadow copy, which
>> is placed in the struct pointed by skel->struct_ops.FOO, where FOO is the
>> map name.
>>
>> The value of a shadow variable will be used to update the value of the map
>> when loading the map to the kernel.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   tools/lib/bpf/libbpf.c          | 195 ++++++++++++++++++++++++++++++--
>>   tools/lib/bpf/libbpf.h          |  34 +++++-
>>   tools/lib/bpf/libbpf.map        |   1 +
>>   tools/lib/bpf/libbpf_internal.h |   1 +
>>   4 files changed, 220 insertions(+), 11 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 01f407591a92..ce9c4cdb2dc5 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -487,6 +487,14 @@ struct bpf_struct_ops {
>>           * from "data".
>>           */
>>          void *kern_vdata;
>> +       /* Description of the layout that a shadow copy should look like.
>> +        */
>> +       const struct bpf_struct_ops_map_info *shadow_info;
>> +       /* A shadow copy of the struct_ops data created according to the
>> +        * layout described by shadow_info.
>> +        */
>> +       void *shadow_data;
>> +       __u32 shadow_data_size;
> 
> what I mentioned on cover letter, just a few lines above, before
> kern_vdata we have just `void *data` which initially contains whatever
> was set in ELF. Just expose that through bpf_map__initial_value() and
> teach bpftool to generate section with variables for that memory and
> that should be all we need, no?

I am not sure if read your question correctly.
Padding & alignments can vary in different platforms. BPF and
user space programs are supposed to be in different platforms.
So, I can not expect that the same struct has the same layout in
BPF/x86/and ARM, right?

> 
>>          __u32 type_id;
>>   };
>>
>> @@ -1027,7 +1035,7 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
>>          struct module_btf *mod_btf;
>>          void *data, *kern_data;
>>          const char *tname;
>> -       int err;
>> +       int err, j;
>>
>>          st_ops = map->st_ops;
>>          type = st_ops->type;
> 
> [...]
> 
>>   void *bpf_map__initial_value(struct bpf_map *map, size_t *psize)
>>   {
>> +       if (bpf_map__is_struct_ops(map)) {
>> +               if (psize)
>> +                       *psize = map->st_ops->shadow_data_size;
>> +               return map->st_ops->shadow_data;
>> +       }
>> +
>>          if (!map->mmaped)
>>                  return NULL;
>>          *psize = map->def.value_size;
>> @@ -13462,3 +13632,8 @@ void bpf_object__destroy_skeleton(struct bpf_object_skeleton *s)
>>          free(s->progs);
>>          free(s);
>>   }
>> +
>> +__u32 bpf_map__struct_ops_type(const struct bpf_map *map)
>> +{
>> +       return map->st_ops->type_id;
>> +}
> 
> we can expose this st_ops->type_id as map->def.value_type_id so that
> existing bpf_map__btf_value_type_id() API can be used, no need to add
> more struct_ops-specific APIs

OK!

> 
>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>> index 5723cbbfcc41..b435cafefe7a 100644
>> --- a/tools/lib/bpf/libbpf.h
>> +++ b/tools/lib/bpf/libbpf.h
>> @@ -109,6 +109,27 @@ LIBBPF_API libbpf_print_fn_t libbpf_set_print(libbpf_print_fn_t fn);
>>   /* Hide internal to user */
>>   struct bpf_object;
>>
>> +/* Description of a member in the struct_ops type for a map. */
>> +struct bpf_struct_ops_member_info {
>> +       const char *name;
>> +       __u32 offset;
>> +       __u32 size;
>> +};
>> +
>> +/* Description of the layout of a shadow copy for a struct_ops map. */
>> +struct bpf_struct_ops_map_info {
>> +       /* The name of the struct_ops map */
>> +       const char *name;
>> +       const struct bpf_struct_ops_member_info *members;
>> +       __u32 cnt;
>> +       __u32 data_size;
>> +};
>> +
>> +struct bpf_struct_ops_shadow_info {
>> +       const struct bpf_struct_ops_map_info *maps;
>> +       __u32 cnt;
>> +};
>> +
>>   struct bpf_object_open_opts {
>>          /* size of this struct, for forward/backward compatibility */
>>          size_t sz;
>> @@ -197,9 +218,18 @@ struct bpf_object_open_opts {
>>           */
>>          const char *bpf_token_path;
>>
>> +       /* A list of shadow info for every struct_ops map.  A shadow info
>> +        * provides the information used by libbpf to map the offsets of
>> +        * struct members of a struct_ops type from BTF to the offsets of
>> +        * the corresponding members in the shadow copy in the user
>> +        * space. It ensures that the shadow copy provided by the libbpf
>> +        * can be accessed by the user space program correctly.
>> +        */
>> +       const struct bpf_struct_ops_shadow_info *struct_ops_shadow;
>> +
> 
> I still don't follow. bpftool will generate memory-layout compatible
> structure for user-space, they can just work directly with that
> memory. We shouldn't need all this extra info structs.
> 
> Libbpf can just check that fields that are supposed to be BPF prog
> references are correct `struct bpf_program *` pointers.

Check the explanation above.

> 
>>          size_t :0;
>>   };
>> -#define bpf_object_open_opts__last_field bpf_token_path
>> +#define bpf_object_open_opts__last_field struct_ops_shadow
>>
>>   /**
>>    * @brief **bpf_object__open()** creates a bpf_object by opening
>> @@ -839,6 +869,8 @@ struct bpf_map;
>>   LIBBPF_API struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map);
>>   LIBBPF_API int bpf_link__update_map(struct bpf_link *link, const struct bpf_map *map);
>>
>> +LIBBPF_API __u32 bpf_map__struct_ops_type(const struct bpf_map *map);
>> +
>>   struct bpf_iter_attach_opts {
>>          size_t sz; /* size of this struct for forward/backward compatibility */
>>          union bpf_iter_link_info *link_info;
>> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
>> index 86804fd90dd1..e0efc85114df 100644
>> --- a/tools/lib/bpf/libbpf.map
>> +++ b/tools/lib/bpf/libbpf.map
>> @@ -413,4 +413,5 @@ LIBBPF_1.4.0 {
>>                  bpf_token_create;
>>                  btf__new_split;
>>                  btf_ext__raw_data;
>> +               bpf_map__struct_ops_type;
>>   } LIBBPF_1.3.0;
>> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
>> index ad936ac5e639..aec6d57fe5d1 100644
>> --- a/tools/lib/bpf/libbpf_internal.h
>> +++ b/tools/lib/bpf/libbpf_internal.h
>> @@ -234,6 +234,7 @@ struct btf_type;
>>   struct btf_type *btf_type_by_id(const struct btf *btf, __u32 type_id);
>>   const char *btf_kind_str(const struct btf_type *t);
>>   const struct btf_type *skip_mods_and_typedefs(const struct btf *btf, __u32 id, __u32 *res_id);
>> +const struct btf_type *resolve_func_ptr(const struct btf *btf, __u32 id, __u32 *res_id);
>>
>>   static inline enum btf_func_linkage btf_func_linkage(const struct btf_type *t)
>>   {
>> --
>> 2.34.1
>>