On Tue, 22 Oct 2024 13:45:08 +0200
Viktor Malik <vmalik@xxxxxxxxxx> wrote:
> 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.
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@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
> >>
> >
> >
>
--
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
