On Thu, Sep 14, 2023 at 2:14 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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?
Thanks,
Song
>
> > +
> > st_map->uvalue = bpf_map_area_alloc(vt->size, NUMA_NO_NODE);
> > st_map->links =
> > bpf_map_area_alloc(btf_type_vlen(t) * sizeof(struct bpf_links *),
> > @@ -907,4 +917,3 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
> > kfree(link);
> > return err;
> > }
> > -
>
