On 10/22/24 07:17, Masami Hiramatsu (Google) wrote:
> On Mon, 26 Aug 2024 08:07:18 +0200
> Viktor Malik <vmalik@xxxxxxxxxx> 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@xxxxxxxxxx>
>> Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
>> Reviewed-by: Matt Wu <wuqiang.matt@xxxxxxxxxxxxx>
>> ---
>> 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
>>
>
>
