Re: [PATCH bpf] bpf: Validate node_id in arena_alloc_pages()

On Fri, Apr 17, 2026 at 5:55 PM <[email protected]> wrote:
>
> Sashiko AI review found 2 potential issue(s):
> - [High] This problem wasn't introduced by this patch, but apply_to_page_range() is called within an atomic critical section with IRQs disabled.
> - [Medium] Validation of user-provided NUMA node ID uses node_online() instead of node_state(..., N_MEMORY).
> --
>
> commit 312a17d46d0e6285c5b29bc93a445829df8c62c7
> Author: Puranjay Mohan <[email protected]>
>
> bpf: Validate node_id in arena_alloc_pages()
>
> This commit validates the node_id parameter before passing it down the
> allocation chain in arena_alloc_pages().
>
> > diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> > index 9c68c9b0b24ad..523c3a61063bf 100644
> > --- a/kernel/bpf/arena.c
> > +++ b/kernel/bpf/arena.c
> > @@ -562,6 +562,10 @@ static long arena_alloc_pages(struct bpf_arena *arena, long uaddr, long page_cnt
> >       u32 uaddr32;
> >       int ret, i;
> >
> > +     if (node_id != NUMA_NO_NODE &&
> > +         ((unsigned int)node_id >= nr_node_ids || !node_online(node_id)))
> > +             return 0;
> > +
>
> Should this check use node_state(node_id, N_MEMORY) instead of
> node_online(node_id)?
>
> A node can be online but memory-less, such as CPU-only nodes. Passing a
> memory-less node down the allocation chain could ignore the user's
> placement intent and cause unexpected allocator fallback.

That is fine I think. it is just a hint.

>
> This issue isn't introduced by this patch, but looking at the surrounding
> code in arena_alloc_pages(), is it safe to call apply_to_page_range()
> while holding a raw spinlock?
>
> arena_alloc_pages()
>     if (raw_res_spin_lock_irqsave(&arena->spinlock, flags))
>         goto out_free_pages;
>     ...
>     while (remaining) {
>         ...
>         ret = apply_to_page_range(...);
>
> Since apply_to_page_range() can sleep when allocating page table entries,
> could this cause a sleeping function called from invalid context panic since
> the spinlock disables interrupts?

No, the page tables are already allocated we just do
set_pte_at(&init_mm, addr, pte, mk_pte(page, PAGE_KERNEL)); in the
callback, so it is fine.