Re: [PATCH bpf-next] bpf: Charge modmem for struct_ops trampoline

[Martin KaFai Lau <martin.lau@linux.dev>]

On 9/14/23 2:28 PM, Song Liu wrote:
> On Thu, Sep 14, 2023 at 2:14 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> On 9/13/23 3:26 PM, Song Liu wrote:
>>> Current code charges modmem for regular trampoline, but not for struct_ops
>>> trampoline. Add bpf_jit_[charge|uncharge]_modmem() to struct_ops so the
>>> trampoline is charged in both cases.
>>>
>>> Signed-off-by: Song Liu <song@kernel.org>
>>> ---
>>>    kernel/bpf/bpf_struct_ops.c | 13 +++++++++++--
>>>    1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>>> index fdc3e8705a3c..ea6ca87a2ed9 100644
>>> --- a/kernel/bpf/bpf_struct_ops.c
>>> +++ b/kernel/bpf/bpf_struct_ops.c
>>> @@ -615,7 +615,10 @@ static void __bpf_struct_ops_map_free(struct bpf_map *map)
>>>        if (st_map->links)
>>>                bpf_struct_ops_map_put_progs(st_map);
>>>        bpf_map_area_free(st_map->links);
>>> -     bpf_jit_free_exec(st_map->image);
>>> +     if (st_map->image) {
>>> +             bpf_jit_free_exec(st_map->image);
>>> +             bpf_jit_uncharge_modmem(PAGE_SIZE);
>>> +     }
>>>        bpf_map_area_free(st_map->uvalue);
>>>        bpf_map_area_free(st_map);
>>>    }
>>> @@ -657,6 +660,7 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>>>        struct bpf_struct_ops_map *st_map;
>>>        const struct btf_type *t, *vt;
>>>        struct bpf_map *map;
>>> +     int ret;
>>>
>>>        st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id);
>>>        if (!st_ops)
>>> @@ -681,6 +685,12 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>>>        st_map->st_ops = st_ops;
>>>        map = &st_map->map;
>>>
>>> +     ret = bpf_jit_charge_modmem(PAGE_SIZE);
>>> +     if (ret) {
>>> +             __bpf_struct_ops_map_free(map);
>>> +             return ERR_PTR(ret);
>>> +     }
>>
>>
>> This just came to my mind when reading it again.
>>
>> It will miss a bpf_jit_uncharge_modmem() if the bpf_jit_alloc_exec() at a few
>> lines below did fail (meaning st_map->image is NULL). It is because the
>> __bpf_struct_ops_map_free() only uncharge if st_map->image is not NULL.
> 
> Indeed. This is a problem.
> 
>>
>> How above moving the bpf_jit_alloc_exec() to here (immediately after
>> bpf_jit_charge_modem succeeded). Like,
>>
>>          st_map->image = bpf_jit_alloc_exec(PAGE_SIZE);
>>          if (!st_map->image) {
>>                  bpf_jit_uncharge_modmem(PAGE_SIZE);
>>                  __bpf_struct_ops_map_free(map);
>>                  return ERR_PTR(-ENOMEM);
>>          }
>>
>> Then there is also no need to test 'if (st_map->image)' in
>> __bpf_struct_ops_map_free().
> 
> I think we still need this test for uncharge, no?

You are right, somehow I thought the above bpf_jit_charge_modmem's failure path 
could directly use the bpf_map_area_free() instead of 
__bpf_struct_ops_map_free(). Agree, lets keep it consistent, call 
__bpf_struct_ops_map_free() on all failure paths and keep the 'if 
(st_map->image)' check there. Thanks.