On Thu, Feb 15, 2024 at 6:35 PM Kui-Feng Lee <sinquersw@xxxxxxxxx> wrote:
>
>
>
> On 2/15/24 15:55, Andrii Nakryiko wrote:
> > On Tue, Feb 13, 2024 at 6:08 PM <thinker.li@xxxxxxxxx> wrote:
> >>
> >> From: Kui-Feng Lee <thinker.li@xxxxxxxxx>
> >>
> >> 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@xxxxxxxxx>
> >> ---
> >> 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?
We can constraint this functionality to 64-bit host architectures, and
then all these concerns will go away. It should be possible to make
all this work even if the host architecture is 64-bit, but I'm not
sure it's worth doing.
Either way, we need to keep this simple and minimal, no extra
descriptors and stuff like that.
>
> >
> >> __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
> >>
