[PATCH v2] objpool: fix choosing allocation for percpu slots

[PATCH v2] objpool: fix choosing allocation for percpu slots

[Viktor Malik]

objpool intends to use vmalloc for default (non-atomic) allocations of
percpu slots and objects. However, the condition checking if GFP flags
are equal to GFP_ATOMIC is wrong b/c GFP_ATOMIC is a combination of bits
(__GFP_HIGH|__GFP_KSWAPD_RECLAIM) and so `pool->gfp & GFP_ATOMIC` will
be true if either bit is set. Since GFP_ATOMIC and GFP_KERNEL share the
___GFP_KSWAPD_RECLAIM bit, kmalloc will be used in cases when GFP_KERNEL
is specified, i.e. in all current usages of objpool.

This may lead to unexpected OOM errors since kmalloc cannot allocate
large amounts of memory.

For instance, objpool is used by fprobe rethook which in turn is used by
BPF kretprobe.multi and kprobe.session probe types. Trying to attach
these to all kernel functions with libbpf using

    SEC("kprobe.session/*")
    int kprobe(struct pt_regs *ctx)
    {
        [...]
    }

fails on objpool slot allocation with ENOMEM.

Fix the condition to truly use vmalloc by default.

Fixes: b4edb8d2d464 ("lib: objpool added: ring-array based lockless MPMC")
Signed-off-by: Viktor Malik <vmalik@redhat.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Reviewed-by: Matt Wu <wuqiang.matt@bytedance.com>
---
 lib/objpool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/objpool.c b/lib/objpool.c
index 234f9d0bd081..fd108fe0d095 100644
--- a/lib/objpool.c
+++ b/lib/objpool.c
@@ -76,7 +76,7 @@ objpool_init_percpu_slots(struct objpool_head *pool, int nr_objs,
 		 * mimimal size of vmalloc is one page since vmalloc would
 		 * always align the requested size to page size
 		 */
-		if (pool->gfp & GFP_ATOMIC)
+		if ((pool->gfp & GFP_ATOMIC) == GFP_ATOMIC)
 			slot = kmalloc_node(size, pool->gfp, cpu_to_node(i));
 		else
 			slot = __vmalloc_node(size, sizeof(void *), pool->gfp,
-- 
2.46.0

Re: [PATCH v2] objpool: fix choosing allocation for percpu slots

[Steven Rostedt]

On Mon, 26 Aug 2024 08:07:18 +0200
Viktor Malik <vmalik@redhat.com> wrote:

> objpool intends to use vmalloc for default (non-atomic) allocations of
> percpu slots and objects. However, the condition checking if GFP flags
> are equal to GFP_ATOMIC is wrong b/c GFP_ATOMIC is a combination of bits
> (__GFP_HIGH|__GFP_KSWAPD_RECLAIM) and so `pool->gfp & GFP_ATOMIC` will
> be true if either bit is set. Since GFP_ATOMIC and GFP_KERNEL share the
> ___GFP_KSWAPD_RECLAIM bit, kmalloc will be used in cases when GFP_KERNEL
> is specified, i.e. in all current usages of objpool.
> 
> This may lead to unexpected OOM errors since kmalloc cannot allocate
> large amounts of memory.
> 
> For instance, objpool is used by fprobe rethook which in turn is used by
> BPF kretprobe.multi and kprobe.session probe types. Trying to attach
> these to all kernel functions with libbpf using
> 
>     SEC("kprobe.session/*")
>     int kprobe(struct pt_regs *ctx)
>     {
>         [...]
>     }
> 
> fails on objpool slot allocation with ENOMEM.
> 
> Fix the condition to truly use vmalloc by default.
> 
> Fixes: b4edb8d2d464 ("lib: objpool added: ring-array based lockless MPMC")
> Signed-off-by: Viktor Malik <vmalik@redhat.com>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> Reviewed-by: Matt Wu <wuqiang.matt@bytedance.com>

Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve

> ---
>  lib/objpool.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/objpool.c b/lib/objpool.c
> index 234f9d0bd081..fd108fe0d095 100644
> --- a/lib/objpool.c
> +++ b/lib/objpool.c
> @@ -76,7 +76,7 @@ objpool_init_percpu_slots(struct objpool_head *pool, int nr_objs,
>  		 * mimimal size of vmalloc is one page since vmalloc would
>  		 * always align the requested size to page size
>  		 */
> -		if (pool->gfp & GFP_ATOMIC)
> +		if ((pool->gfp & GFP_ATOMIC) == GFP_ATOMIC)
>  			slot = kmalloc_node(size, pool->gfp, cpu_to_node(i));
>  		else
>  			slot = __vmalloc_node(size, sizeof(void *), pool->gfp,

Re: [PATCH v2] objpool: fix choosing allocation for percpu slots

[Masami Hiramatsu]

On Mon, 26 Aug 2024 08:07:18 +0200
Viktor Malik <vmalik@redhat.com> wrote:

> objpool intends to use vmalloc for default (non-atomic) allocations of
> percpu slots and objects. However, the condition checking if GFP flags
> are equal to GFP_ATOMIC is wrong b/c GFP_ATOMIC is a combination of bits

You meant "whether GFP flags sets any bit of GFP_ATOMIC is wrong"?

> (__GFP_HIGH|__GFP_KSWAPD_RECLAIM) and so `pool->gfp & GFP_ATOMIC` will
> be true if either bit is set. Since GFP_ATOMIC and GFP_KERNEL share the
> ___GFP_KSWAPD_RECLAIM bit, kmalloc will be used in cases when GFP_KERNEL
> is specified, i.e. in all current usages of objpool.
> 
> This may lead to unexpected OOM errors since kmalloc cannot allocate
> large amounts of memory.
> 
> For instance, objpool is used by fprobe rethook which in turn is used by
> BPF kretprobe.multi and kprobe.session probe types. Trying to attach
> these to all kernel functions with libbpf using
> 
>     SEC("kprobe.session/*")
>     int kprobe(struct pt_regs *ctx)
>     {
>         [...]
>     }
> 
> fails on objpool slot allocation with ENOMEM.
> 
> Fix the condition to truly use vmalloc by default.
> 

Anyway, this looks good to me.

Thank you,

> Fixes: b4edb8d2d464 ("lib: objpool added: ring-array based lockless MPMC")
> Signed-off-by: Viktor Malik <vmalik@redhat.com>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> Reviewed-by: Matt Wu <wuqiang.matt@bytedance.com>
> ---
>  lib/objpool.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/objpool.c b/lib/objpool.c
> index 234f9d0bd081..fd108fe0d095 100644
> --- a/lib/objpool.c
> +++ b/lib/objpool.c
> @@ -76,7 +76,7 @@ objpool_init_percpu_slots(struct objpool_head *pool, int nr_objs,
>  		 * mimimal size of vmalloc is one page since vmalloc would
>  		 * always align the requested size to page size
>  		 */
> -		if (pool->gfp & GFP_ATOMIC)
> +		if ((pool->gfp & GFP_ATOMIC) == GFP_ATOMIC)
>  			slot = kmalloc_node(size, pool->gfp, cpu_to_node(i));
>  		else
>  			slot = __vmalloc_node(size, sizeof(void *), pool->gfp,
> -- 
> 2.46.0
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v2] objpool: fix choosing allocation for percpu slots

[Viktor Malik]

On 10/22/24 07:17, Masami Hiramatsu (Google) wrote:
> On Mon, 26 Aug 2024 08:07:18 +0200
> Viktor Malik <vmalik@redhat.com> wrote:
> 
>> objpool intends to use vmalloc for default (non-atomic) allocations of
>> percpu slots and objects. However, the condition checking if GFP flags
>> are equal to GFP_ATOMIC is wrong b/c GFP_ATOMIC is a combination of bits
> 
> You meant "whether GFP flags sets any bit of GFP_ATOMIC is wrong"?

Well, I meant that the condition is wrong w.r.t. what is supposedly its
original purpose. But feel free to rephrase as you seem fit or I can
send v3 if you prefer.

Thanks.
Viktor

> 
>> (__GFP_HIGH|__GFP_KSWAPD_RECLAIM) and so `pool->gfp & GFP_ATOMIC` will
>> be true if either bit is set. Since GFP_ATOMIC and GFP_KERNEL share the
>> ___GFP_KSWAPD_RECLAIM bit, kmalloc will be used in cases when GFP_KERNEL
>> is specified, i.e. in all current usages of objpool.
>>
>> This may lead to unexpected OOM errors since kmalloc cannot allocate
>> large amounts of memory.
>>
>> For instance, objpool is used by fprobe rethook which in turn is used by
>> BPF kretprobe.multi and kprobe.session probe types. Trying to attach
>> these to all kernel functions with libbpf using
>>
>>     SEC("kprobe.session/*")
>>     int kprobe(struct pt_regs *ctx)
>>     {
>>         [...]
>>     }
>>
>> fails on objpool slot allocation with ENOMEM.
>>
>> Fix the condition to truly use vmalloc by default.
>>
> 
> Anyway, this looks good to me.
> 
> Thank you,
> 
>> Fixes: b4edb8d2d464 ("lib: objpool added: ring-array based lockless MPMC")
>> Signed-off-by: Viktor Malik <vmalik@redhat.com>
>> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>> Reviewed-by: Matt Wu <wuqiang.matt@bytedance.com>
>> ---
>>  lib/objpool.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/objpool.c b/lib/objpool.c
>> index 234f9d0bd081..fd108fe0d095 100644
>> --- a/lib/objpool.c
>> +++ b/lib/objpool.c
>> @@ -76,7 +76,7 @@ objpool_init_percpu_slots(struct objpool_head *pool, int nr_objs,
>>  		 * mimimal size of vmalloc is one page since vmalloc would
>>  		 * always align the requested size to page size
>>  		 */
>> -		if (pool->gfp & GFP_ATOMIC)
>> +		if ((pool->gfp & GFP_ATOMIC) == GFP_ATOMIC)
>>  			slot = kmalloc_node(size, pool->gfp, cpu_to_node(i));
>>  		else
>>  			slot = __vmalloc_node(size, sizeof(void *), pool->gfp,
>> -- 
>> 2.46.0
>>
> 
> 

Re: [PATCH v2] objpool: fix choosing allocation for percpu slots

[Masami Hiramatsu]

On Tue, 22 Oct 2024 13:45:08 +0200
Viktor Malik <vmalik@redhat.com> wrote:

> On 10/22/24 07:17, Masami Hiramatsu (Google) wrote:
> > On Mon, 26 Aug 2024 08:07:18 +0200
> > Viktor Malik <vmalik@redhat.com> wrote:
> > 
> >> objpool intends to use vmalloc for default (non-atomic) allocations of
> >> percpu slots and objects. However, the condition checking if GFP flags
> >> are equal to GFP_ATOMIC is wrong b/c GFP_ATOMIC is a combination of bits
> > 
> > You meant "whether GFP flags sets any bit of GFP_ATOMIC is wrong"?
> 
> Well, I meant that the condition is wrong w.r.t. what is supposedly its
> original purpose. But feel free to rephrase as you seem fit or I can
> send v3 if you prefer.

No problem :) let me rephrase that part.

Thank you!

> 
> Thanks.
> Viktor
> 
> > 
> >> (__GFP_HIGH|__GFP_KSWAPD_RECLAIM) and so `pool->gfp & GFP_ATOMIC` will
> >> be true if either bit is set. Since GFP_ATOMIC and GFP_KERNEL share the
> >> ___GFP_KSWAPD_RECLAIM bit, kmalloc will be used in cases when GFP_KERNEL
> >> is specified, i.e. in all current usages of objpool.
> >>
> >> This may lead to unexpected OOM errors since kmalloc cannot allocate
> >> large amounts of memory.
> >>
> >> For instance, objpool is used by fprobe rethook which in turn is used by
> >> BPF kretprobe.multi and kprobe.session probe types. Trying to attach
> >> these to all kernel functions with libbpf using
> >>
> >>     SEC("kprobe.session/*")
> >>     int kprobe(struct pt_regs *ctx)
> >>     {
> >>         [...]
> >>     }
> >>
> >> fails on objpool slot allocation with ENOMEM.
> >>
> >> Fix the condition to truly use vmalloc by default.
> >>
> > 
> > Anyway, this looks good to me.
> > 
> > Thank you,
> > 
> >> Fixes: b4edb8d2d464 ("lib: objpool added: ring-array based lockless MPMC")
> >> Signed-off-by: Viktor Malik <vmalik@redhat.com>
> >> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> >> Reviewed-by: Matt Wu <wuqiang.matt@bytedance.com>
> >> ---
> >>  lib/objpool.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/lib/objpool.c b/lib/objpool.c
> >> index 234f9d0bd081..fd108fe0d095 100644
> >> --- a/lib/objpool.c
> >> +++ b/lib/objpool.c
> >> @@ -76,7 +76,7 @@ objpool_init_percpu_slots(struct objpool_head *pool, int nr_objs,
> >>  		 * mimimal size of vmalloc is one page since vmalloc would
> >>  		 * always align the requested size to page size
> >>  		 */
> >> -		if (pool->gfp & GFP_ATOMIC)
> >> +		if ((pool->gfp & GFP_ATOMIC) == GFP_ATOMIC)
> >>  			slot = kmalloc_node(size, pool->gfp, cpu_to_node(i));
> >>  		else
> >>  			slot = __vmalloc_node(size, sizeof(void *), pool->gfp,
> >> -- 
> >> 2.46.0
> >>
> > 
> > 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>