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

[Viktor Malik <vmalik@redhat.com>]

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