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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.






[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux