* [PATCH bpf-next] bpf: Charge modmem for struct_ops trampoline
@ 2023-09-13 22:26 Song Liu
2023-09-14 21:14 ` Martin KaFai Lau
0 siblings, 1 reply; 4+ messages in thread
From: Song Liu @ 2023-09-13 22:26 UTC (permalink / raw)
To: bpf; +Cc: ast, daniel, andrii, martin.lau, kernel-team, Song Liu
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);
+ }
+
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;
}
-
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] bpf: Charge modmem for struct_ops trampoline
2023-09-13 22:26 [PATCH bpf-next] bpf: Charge modmem for struct_ops trampoline Song Liu
@ 2023-09-14 21:14 ` Martin KaFai Lau
2023-09-14 21:28 ` Song Liu
0 siblings, 1 reply; 4+ messages in thread
From: Martin KaFai Lau @ 2023-09-14 21:14 UTC (permalink / raw)
To: Song Liu; +Cc: ast, daniel, andrii, kernel-team, bpf
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.
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().
> +
> 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;
> }
> -
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] bpf: Charge modmem for struct_ops trampoline
2023-09-14 21:14 ` Martin KaFai Lau
@ 2023-09-14 21:28 ` Song Liu
2023-09-14 22:16 ` Martin KaFai Lau
0 siblings, 1 reply; 4+ messages in thread
From: Song Liu @ 2023-09-14 21:28 UTC (permalink / raw)
To: Martin KaFai Lau; +Cc: ast, daniel, andrii, kernel-team, bpf
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?
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;
> > }
> > -
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] bpf: Charge modmem for struct_ops trampoline
2023-09-14 21:28 ` Song Liu
@ 2023-09-14 22:16 ` Martin KaFai Lau
0 siblings, 0 replies; 4+ messages in thread
From: Martin KaFai Lau @ 2023-09-14 22:16 UTC (permalink / raw)
To: Song Liu; +Cc: ast, daniel, andrii, kernel-team, bpf
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-09-14 22:16 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-13 22:26 [PATCH bpf-next] bpf: Charge modmem for struct_ops trampoline Song Liu
2023-09-14 21:14 ` Martin KaFai Lau
2023-09-14 21:28 ` Song Liu
2023-09-14 22:16 ` Martin KaFai Lau
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).