Re: [PATCH bpf-next v5 3/6] libbpf: Convert st_ops->data to shadow type.

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

On 2/28/24 09:58, Andrii Nakryiko wrote:
> On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <thinker.li@gmail.com> wrote:
>>
>> Convert st_ops->data to the shadow type of the struct_ops map. The shadow
>> type of a struct_ops type is a variant of the original struct type
>> providing a way to access/change the values in the maps of the struct_ops
>> type.
>>
>> bpf_map__initial_value() will return st_ops->data for struct_ops types. The
>> skeleton is going to use it as the pointer to the shadow type of the
>> original struct type.
>>
>> One of the main differences between the original struct type and the shadow
>> type is that all function pointers of the shadow type are converted to
>> pointers of struct bpf_program. Users can replace these bpf_program
>> pointers with other BPF programs. The st_ops->progs[] will be updated
>> before updating the value of a map to reflect the changes made by users.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   tools/lib/bpf/libbpf.c | 21 ++++++++++++++++++++-
>>   1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 465b50235a01..2d22344fb127 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -1102,6 +1102,9 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
>>                  if (btf_is_ptr(mtype)) {
>>                          struct bpf_program *prog;
>>
>> +                       /* Update the value from the shadow type */
>> +                       st_ops->progs[i] = *(struct bpf_program **)mdata;
>> +
> 
> it's unsettling to just cast a pointer like this without any
> validation. It's too easy for users to set either some garbage there
> or struct bpf_program * pointer from some other skeleton.
> 
> Luckily, validation is pretty simple, we can just iterate over all
> bpf_object's programs and check if any of them matches the value of
> the mdata pointer. If not, error out with meaningful error.

Make sense to me.

> 
> Also, even if the bpf_program pointer is correct, it could be a
> program of the wrong type, so I think we should add a bit more
> validation here, given these pointers are set by users directly after
> bpf_object is opened.


Agree!
Although this will be checked by the kernel, it makes sense to check at
the user space to provide a more meaningful error.

> 
>>                          prog = st_ops->progs[i];
>>                          if (!prog)
>>                                  continue;
>> @@ -9308,7 +9311,9 @@ static struct bpf_map *find_struct_ops_map_by_offset(struct bpf_object *obj,
>>          return NULL;
>>   }
>>
>> -/* Collect the reloc from ELF and populate the st_ops->progs[] */
>> +/* Collect the reloc from ELF, populate the st_ops->progs[], and update
>> + * st_ops->data for shadow type.
>> + */
>>   static int bpf_object__collect_st_ops_relos(struct bpf_object *obj,
>>                                              Elf64_Shdr *shdr, Elf_Data *data)
>>   {
>> @@ -9422,6 +9427,14 @@ static int bpf_object__collect_st_ops_relos(struct bpf_object *obj,
>>                  }
>>
>>                  st_ops->progs[member_idx] = prog;
>> +
>> +               /* st_ops->data will be expose to users, being returned by
> 
> typo: exposed
> 
>> +                * bpf_map__initial_value() as a pointer to the shadow
>> +                * type. All function pointers in the original struct type
>> +                * should be converted to a pointer to struct bpf_program
>> +                * in the shadow type.
>> +                */
>> +               *((struct bpf_program **)(st_ops->data + moff)) = prog;
>>          }
>>
>>          return 0;
>> @@ -9880,6 +9893,12 @@ int bpf_map__set_initial_value(struct bpf_map *map,
>>
>>   void *bpf_map__initial_value(struct bpf_map *map, size_t *psize)
>>   {
>> +       if (bpf_map__is_struct_ops(map)) {
>> +               if (psize)
>> +                       *psize = map->def.value_size;
>> +               return map->st_ops->data;
>> +       }
>> +
>>          if (!map->mmaped)
>>                  return NULL;
>>          *psize = map->def.value_size;
>> --
>> 2.34.1
>>