On Thu, Jan 25, 2024 at 3:13 PM Kui-Feng Lee <sinquersw@xxxxxxxxx> wrote:
>
>
>
> On 1/25/24 14:21, Andrii Nakryiko wrote:
> > On Wed, Jan 24, 2024 at 2:41 PM <thinker.li@xxxxxxxxx> wrote:
> >>
> >> From: Kui-Feng Lee <thinker.li@xxxxxxxxx>
> >>
> >> Create shadow variables for the fields of struct_ops maps in a skeleton
> >> with the same name as the respective fields. For instance, if struct
> >> bpf_testmod_ops has a "data" field as following, you can access or modify
> >> its value through a shadow variable also named "data" in the skeleton.
> >>
> >> SEC(".struct_ops.link")
> >> struct bpf_testmod_ops testmod_1 = {
> >> .data = 0x1,
> >> };
> >>
> >> Then, you can change the value in the following manner as shown in the code
> >> below.
> >>
> >> skel->st_ops_vars.testmod_1.data = 13;
> >>
> >> It is helpful to configure a struct_ops map during the execution of a
> >> program. For instance, you can include a flag in a struct_ops type to
> >> modify the way the kernel handles an instance. By implementing this
> >> feature, a user space program can alter the flag's value prior to loading
> >> an instance into the kernel.
> >>
> >> The code generator for skeletons will produce code that copies values to
> >> shadow variables from the internal data buffer when a skeleton is
> >> opened. It will also copy values from shadow variables back to the internal
> >> data buffer before a skeleton is loaded into the kernel.
> >>
> >> The code generator will calculate the offset of a field and generate code
> >> that copies the value of the field from or to the shadow variable to or
> >> from the struct_ops internal data, with an offset relative to the beginning
> >> of the internal data. For instance, if the "data" field in struct
> >> bpf_testmod_ops is situated 16 bytes from the beginning of the struct, the
> >> address for the "data" field of struct bpf_testmod_ops will be the starting
> >> address plus 16.
> >>
> >> The offset is calculated during code generation, so it is always in line
> >> with the internal data in the skeleton. Even if the user space programs and
> >> the BPF program were not compiled together, any differences in versions
> >> would not impact correctness. This means that even if the BPF program and
> >> the user space program reference different versions of the "struct
> >> bpf_testmod_ops" and have different offsets for "data", these offsets
> >> computed by the code generator would still function correctly.
> >>
> >> The user space programs can only modify values by using these shadow
> >> variables when the skeleton is open, but before being loaded. Once the
> >> skeleton is loaded, the value is copied to the kernel, and any future
> >> changes only affect the shadow variables in the user space memory and do
> >> not update the kernel space. The shadow variables are not initialized
> >> before opening a skeleton, so you cannot update values through them before
> >> opening.
> >>
> >> For function pointers (operators), you can change/replace their values with
> >> other BPF programs. For example, the test case in test_struct_ops_module.c
> >> points .test_2 to test_3() while it was pointed to test_2() by assigning a
> >> new value to the shadow variable.
> >>
> >> skel->st_ops_vars.testmod_1.test_2 = skel->progs.test_3;
> >>
> >> However, you need to turn off autoload for test_2() since it is not
> >> assigned to any struct_ops map anymore. Or, it will fails to load test_2().
> >>
> >> bpf_program__set_autoload(skel->progs.test_2, false);
> >>
> >> You can define more struct_ops programs than necessary. However, you need
> >> to turn autoload off for unused ones.
> >
> > Overall I like the idea, it seems like a pretty natural and powerful
> > interface. Few things I'd do a bit differently:
> >
> > - less code gen in the skeleton. It feels like it's better to teach
> > libbpf to allow getting/setting intial struct_ops map "image" using
> > standard bpf_map__initial_value() and bpf_map__set_initial_value().
> > That fits with other global data maps.
>
> Right, it is doable to move some logic from the code generator to
> libbpf. The only thing should keep in the code generator should be
> generating shadow variable fields in the skeleton.
>
> >
> > - I think all struct ops maps should be in skel->struct_ops.<name>,
> > not struct_ops_vars. I'd probably also combine struct_ops.link and
> > no-link struct ops in one section for simplicity
>
> agree!
>
> >
> > - getting underlying struct_ops BTF type should be possible to do
> > through bpf_map__btf_value_type_id(), no need for struct_ops-specific
> > one
>
> AFAIK, libbpf doesn't set def.value_type_id for struct_ops maps.
> bpf_map__btf_value_type_id() doesn't return the ID of a struct_ops type.
> I will check what the side effects are if def.value_type_id is set for
> struct_ops maps.
Yes, it doesn't right now, not sure why, though. At least we can fix
that on libbpf side.
>
> >
> > - you have a bunch of specific logic to dump INT/ENUM/PTR, I wonder if
> > we can just reuse libbpf's BTF dumping API to dump everything? Though
> > for prog pointers we'd want to rewrite them to `struct bpf_program *`
> > field, not sure yet how hard it is.
>
> I am not quite sure what part you are talking about.
> Do you mean the code skipping type modifiers?
> The implementation skips all const (and static/volatile/... etc) to make
> sure the user space programs can change these values without any
> tricky type casting.
>
No, I'm talking about gen_st_ops_shadow_vars and gen_st_ops_func_one,
which don't handle members of type STRUCT/UNION, for example. I didn't
look too deeply into details of the implementation in those parts, but
I don't see any reason why we shouldn't support embedded struct
members there?
> >
> > The above is brief, so please ask questions where it's not clear what
> > I propose, thanks!
> >
> > [...]
>
> Thank you for the comments.
>
