On 9/14/23 2:28 PM, Song Liu wrote:
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?
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.
