Re: [PATCH bpf-next v2 2/3] bpf: struct_ops supports more than one page for trampolines.

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

On 2/22/24 18:16, Martin KaFai Lau wrote:
> On 2/22/24 5:35 PM, Kui-Feng Lee wrote:
>>
>>
>> On 2/22/24 16:33, Martin KaFai Lau wrote:
>>> On 2/21/24 2:59 PM, thinker.li@gmail.com wrote:
>>>> @@ -531,10 +567,10 @@ static long 
>>>> bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
>>>>       const struct btf_type *module_type;
>>>>       const struct btf_member *member;
>>>>       const struct btf_type *t = st_ops_desc->type;
>>>> +    void *image = NULL, *image_end = NULL;
>>>>       struct bpf_tramp_links *tlinks;
>>>>       void *udata, *kdata;
>>>>       int prog_fd, err;
>>>> -    void *image, *image_end;
>>>>       u32 i;
>>>>       if (flags)
>>>> @@ -573,15 +609,14 @@ static long 
>>>> bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
>>>>       udata = &uvalue->data;
>>>>       kdata = &kvalue->data;
>>>> -    image = st_map->image;
>>>> -    image_end = st_map->image + PAGE_SIZE;
>>>>       module_type = btf_type_by_id(btf_vmlinux, 
>>>> st_ops_ids[IDX_MODULE_ID]);
>>>>       for_each_member(i, t, member) {
>>>>           const struct btf_type *mtype, *ptype;
>>>>           struct bpf_prog *prog;
>>>>           struct bpf_tramp_link *link;
>>>> -        u32 moff;
>>>> +        u32 moff, tflags;
>>>> +        int tsize;
>>>>           moff = __btf_member_bit_offset(t, member) / 8;
>>>>           ptype = btf_type_resolve_ptr(st_map->btf, member->type, 
>>>> NULL);
>>>> @@ -653,10 +688,38 @@ static long 
>>>> bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
>>>>                     &bpf_struct_ops_link_lops, prog);
>>>>           st_map->links[i] = &link->link;
>>>> -        err = bpf_struct_ops_prepare_trampoline(tlinks, link,
>>>> -                            &st_ops->func_models[i],
>>>> -                            *(void **)(st_ops->cfi_stubs + moff),
>>>> -                            image, image_end);
>>>> +        tflags = BPF_TRAMP_F_INDIRECT;
>>>> +        if (st_ops->func_models[i].ret_size > 0)
>>>> +            tflags |= BPF_TRAMP_F_RET_FENTRY_RET;
>>>> +
>>>> +        /* Compute the size of the trampoline */
>>>> +        tlinks[BPF_TRAMP_FENTRY].links[0] = link;
>>>> +        tlinks[BPF_TRAMP_FENTRY].nr_links = 1;
>>>> +        tsize = arch_bpf_trampoline_size(&st_ops->func_models[i],
>>>> +                         tflags, tlinks, NULL);
>>>> +        if (tsize < 0) {
>>>> +            err = tsize;
>>>> +            goto reset_unlock;
>>>> +        }
>>>> +
>>>> +        /* Allocate pages */
>>>> +        if (tsize > (unsigned long)image_end - (unsigned long)image) {
>>>> +            if (tsize > PAGE_SIZE) {
>>>> +                err = -E2BIG;
>>>> +                goto reset_unlock;
>>>> +            }
>>>> +            image = bpf_struct_ops_map_inc_image(st_map);
>>>> +            if (IS_ERR(image)) {
>>>> +                err = PTR_ERR(image);
>>>> +                goto reset_unlock;
>>>> +            }
>>>> +            image_end = image + PAGE_SIZE;
>>>> +        }
>>>> +
>>>> +        err = arch_prepare_bpf_trampoline(NULL, image, image_end,
>>>> +                          &st_ops->func_models[i],
>>>> +                          tflags, tlinks,
>>>> +                          *(void **)(st_ops->cfi_stubs + moff));
>>>
>>> I don't prefer to copy the BPF_TRAMP_F_* setting on tflags, tlinks, 
>>> and the arch_*_trampoline_*() logic from 
>>> bpf_struct_ops_prepare_trampoline() which is used by the 
>>> bpf_dummy_ops for testing also. Considering struct_ops supports 
>>> kernel module now, in the future, it is better to move bpf_dummy_ops 
>>> out to the bpf_testmod somehow and avoid its 
>>> bpf_struct_ops_prepare_trampoline() usage. For now, it is still 
>>> better to keep bpf_struct_ops_prepare_trampoline() to be reusable by 
>>> both.
>>>
>>> Have you thought about the earlier suggestion in v1 to do 
>>> arch_alloc_bpf_trampoline() in bpf_struct_ops_prepare_trampoline() 
>>> instead of copying codes from bpf_struct_ops_prepare_trampoline() to 
>>> bpf_struct_ops_map_update_elem()?
>>>
>>> Something like this (untested code):
>>>
>>> void *bpf_struct_ops_prepare_trampoline(struct bpf_tramp_links *tlinks,
>>>                      struct bpf_tramp_link *link,
>>>                      const struct btf_func_model *model,
>>>                      void *stub_func, void *image,
>>>                      u32 *image_off,
>>>                      bool allow_alloc)
> 
> To be a little more specific, the changes in 
> bpf_struct_ops_map_update_elem()
> could be mostly like this (untested):
> 
>          ret_image = bpf_struct_ops_prepare_trampoline(tlinks, link,
>                                    &st_ops->func_models[i],
>                                    *(void **)(st_ops->cfi_stubs + moff),
>                                    image, &image_off,
>                                    st_map->image_pages_cnt < 
> MAX_TRAMP_IMAGE_PAGES);
>          if (IS_ERR(ret_image))
>              goto reset_unlock;
> 
>          if (image != ret_image) {
>              image = ret_image;
>              st_map->image_pages[st_map->image_pages_cnt++] = image;
>          }
> 

What I don't like is the memory management code was in two named
functions, bpf_struct_ops_map_free_image() and
bpf_struct_ops_map_inc_image().
Now, it falls apart.  Allocate in one place, keep accounting in another
place, and free yet at the 3rd place.

>>
>>
>> How about pass a struct bpf_struct_ops_map to
>> bpf_struct_ops_prepare_trampoline(). If the pointer of struct
>> bpf_struct_ops_map is not NULL, try to allocate new pages for the map?
>>
>> For example,
>>
>> static int
>> _bpf_struct_ops_prepare_trampoline(struct bpf_tramp_links *tlinks,
>>
>>                                     struct bpf_tramp_link *link,
>>
>>                                     const struct btf_func_model *model,
>>
>>                                     void *stub_func, void *image,
>>                                     void *image_end,
>>                                     struct bpf_struct_ops_map *st_map)
>> {
>>
>> ...
>>
>>      if (!image || size > PAGE_SIZE - *image_off) {
>>          if (!st_map)
> 
> Why only limit to st_map != NULL?
> 
> arch_alloc_bpf_trampoline() is also called in bpf_dummy_ops.
> If bpf_struct_ops_prepare_trampoline() can do the alloc, it may as well 
> simplify
> bpf_dummy_ops and just use bpf_struct_ops_prepare_trampoline() to alloc.


Yes, it can save a few lines from bpf_dummy_ops. But, bpf_dummy_ops
still need to free the memory. And, it doesn't pair alloc and free in
the same function. Usually, paring alloc and free in the same function,
nearby, or the same module is easier to understand.


[ skip ]