Re: [PATCH bpf-next v2] bpf: Add kernel-doc for arena page kfuncs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] The commit message incorrectly claims that the BPF arena kfuncs have a "sleepable-only context" contract.
- [Low] The newly added kernel-doc comments omit several failure conditions and error returns for the BPF arena kfuncs.

Pre-existing issues:
- [High] `bpf_arena_reserve_pages()` silently returns 0 (success) when passed a non-page-aligned address, failing to actually reserve the pages.
- [High] `range_tree_clear()` corrupts the range tree and leaks free space if memory allocation fails.
--

commit 4e0172dd044fa3635f4bf80032c1bc3a4a5ac292
Author: Dhiraj Shah <find.dhiraj@gmail.com>

bpf: Add kernel-doc for arena page kfuncs

This commit adds kernel-doc comments for BPF arena page-management kfuncs
and renders them under a new subsection in Documentation/bpf/kfuncs.rst.
No functional changes are introduced.

> The page-management kfuncs exposed by BPF arena -
> bpf_arena_alloc_pages(), bpf_arena_free_pages() and
> bpf_arena_reserve_pages() - are part of the BPF kfunc ABI but lack
> rendered documentation. Their contracts (valid argument ranges,
> sleepable-only context, and the set of error returns) are today only

[Severity: Medium]
Is the description "sleepable-only context" accurate here?

The BPF verifier supports calling bpf_arena_alloc_pages() and
bpf_arena_free_pages() in non-sleepable contexts by dynamically replacing
the calls with their non-sleepable variants
(bpf_arena_alloc_pages_non_sleepable and
bpf_arena_free_pages_non_sleepable).

Additionally, the BTF_ID_FLAGS for these kfuncs do not appear to include
the KF_SLEEPABLE flag.

> discoverable by reading kernel/bpf/arena.c.

[ ... ]

> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> index 49a8f7b1beef5..948a431591063 100644
> --- a/kernel/bpf/arena.c
> +++ b/kernel/bpf/arena.c

[ ... ]

> + * Return:
> + * * The user-space virtual address of the start of the allocated region on
> + *   success. The BPF JIT translates this address for accesses from the BPF
> + *   program.
> + * * %NULL if @p__map is not an arena, @flags is non-zero, @page_cnt is zero
> + *   or exceeds the arena size, @addr__ign is misaligned or outside the
> + *   arena, @node_id is invalid, or the kernel is out of memory.
> + */
>  __bpf_kfunc void *bpf_arena_alloc_pages(void *p__map, void *addr__ign, u32 page_cnt,

[Severity: Low]
Are there missing failure conditions in this documentation?

It appears bpf_arena_alloc_pages() can also return %NULL if the requested
address is already allocated, or if there is no contiguous free space of
the requested size in the arena.

[ ... ]

> + * Return:
> + * * 0 on success, or when @page_cnt is zero.
> + * * -EINVAL if @p__map is not an arena or the requested range falls outside
> + *   the arena's user VMA.
> + * * -EBUSY if any page in the requested range is already allocated, or if
> + *   contention on the arena's internal spinlock prevents the operation from
> + *   completing.
> + */
>  __bpf_kfunc int bpf_arena_reserve_pages(void *p__map, void *ptr__ign, u32 page_cnt)

[Severity: Low]
Does this also need to document -ENOMEM?

If range_tree_clear() fails to allocate a new node during a split operation,
it seems this can return -ENOMEM.

[Severity: High]
This isn't a bug introduced by this patch, but does arena_reserve_pages()
silently fail when passed a non-page-aligned address?

In arena_reserve_pages(), if the user-provided address is not page-aligned:

kernel/bpf/arena.c:arena_reserve_pages() {
    ...
    if (uaddr & ~PAGE_MASK)
        return 0;
    ...
}

Since 0 indicates success for this kfunc, doesn't this mislead the caller
into believing the pages were successfully reserved? No pages are actually
reserved in the arena's range tree, which could allow subsequent
bpf_arena_alloc_pages() calls to allocate that exact memory.

[Severity: High]
This is a pre-existing issue, but does range_tree_clear() corrupt the range
tree and leak free space if memory allocation fails?

When range_tree_clear() clears a range that strictly overlaps the middle of
an existing range node, it splits the node. It removes the original node,
reinserts the left portion, and then calls kmalloc_nolock() to allocate a
new node for the right portion.

If kmalloc_nolock() fails (returning -ENOMEM), the function returns
immediately and the right portion of the original node is never reinserted.
Since this tree tracks free space in the arena, does losing a node
permanently leak that free space and make it unavailable for future
allocations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260524114243.259916-1-find.dhiraj@gmail.com?part=1