[PATCH] objpool: fix choosing allocation for percpu slots

[PATCH] 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 and causes kmalloc to be used in most
cases (even if GFP_KERNEL is requested). Since kmalloc cannot allocate
large amounts of memory, this may lead to unexpected OOM errors.

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>
---
 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] objpool: fix choosing allocation for percpu slots

[Andrii Nakryiko]

On Thu, Aug 22, 2024 at 1:27 AM 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 and causes kmalloc to be used in most

I was confused by this, because original code has no equality and it
looks like correct code. But in reality GFP_ATOMIC is a collection of
bits (__GFP_HIGH|__GFP_KSWAPD_RECLAIM), and so `pool->gfp &
GFP_ATOMIC` will be true if either bit is set, hence your change.
Also, GFP_ATOMIC and GFP_KERNEL share ___GFP_KSWAPD_RECLAIM bit
specifically, which is what causes the use of kmalloc_node(), always.

It would be nice to expand on that in the commit. Other than that LGTM

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> cases (even if GFP_KERNEL is requested). Since kmalloc cannot allocate
> large amounts of memory, this may lead to unexpected OOM errors.
>
> 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>
> ---
>  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] objpool: fix choosing allocation for percpu slots

[Viktor Malik]

On 8/22/24 11:30 PM, Andrii Nakryiko wrote:
> On Thu, Aug 22, 2024 at 1:27 AM 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 and causes kmalloc to be used in most
> 
> I was confused by this, because original code has no equality and it
> looks like correct code. But in reality GFP_ATOMIC is a collection of
> bits (__GFP_HIGH|__GFP_KSWAPD_RECLAIM), and so `pool->gfp &
> GFP_ATOMIC` will be true if either bit is set, hence your change.
> Also, GFP_ATOMIC and GFP_KERNEL share ___GFP_KSWAPD_RECLAIM bit
> specifically, which is what causes the use of kmalloc_node(), always.
> 
> It would be nice to expand on that in the commit. Other than that LGTM

Right, the commit message could use a better explanation, thanks!

I'll update it, add the acks, and send v2.

Viktor

> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
>> cases (even if GFP_KERNEL is requested). Since kmalloc cannot allocate
>> large amounts of memory, this may lead to unexpected OOM errors.
>>
>> 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>
>> ---
>>  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] objpool: fix choosing allocation for percpu slots

[wuqiang.matt]

On 2024/8/22 16:25, Viktor Malik 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 and causes kmalloc to be used in most
> cases (even if GFP_KERNEL is requested). Since kmalloc cannot allocate
> large amounts of memory, this may lead to unexpected OOM errors.

Sure, good catch. Don't notice that GFP_ATOMIC is not atomic. My original
intention is using kmalloc only if GFP_ATOMIC is specified and other flags
should go with vmalloc, but (pool->gfp == GFP_ATOMIC) is not accurate.

Masami, please help review and include this patch into your patch set if
it's appropriate to you. Thanks.

Reviewed-by: Matt Wu <wuqiang.matt@bytedance.com>

> 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>
> ---
>   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,