Re: [PATCH bpf-next v7 3/4] bpf: arena: make arena kfuncs any context safe

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2855 bytes --]

> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> index 128efb68d47b..55d1e15725c7 100644

[ ... ]

> +static void arena_free_worker(struct work_struct *work)
> +{
> +	struct bpf_arena *arena = container_of(work, struct bpf_arena, free_work);
> +	struct llist_node *list, *pos, *t;
> +	struct arena_free_span *s;
> +	u64 arena_vm_start, user_vm_start;
> +	struct llist_head free_pages;
> +	struct page *page;
> +	unsigned long full_uaddr;
> +	long kaddr, page_cnt, pgoff;
> +	unsigned long flags;
> +
> +	if (raw_res_spin_lock_irqsave(&arena->spinlock, flags)) {
> +		schedule_work(work);
> +		return;
> +	}
> +
> +	init_llist_head(&free_pages);
> +	arena_vm_start = bpf_arena_get_kern_vm_start(arena);
> +	user_vm_start = bpf_arena_get_user_vm_start(arena);
> +
> +	list = llist_del_all(&arena->free_spans);
> +	llist_for_each(pos, list) {
> +		s = llist_entry(pos, struct arena_free_span, node);
> +		page_cnt = s->page_cnt;
> +		kaddr = arena_vm_start + s->uaddr;
> +		pgoff = compute_pgoff(arena, s->uaddr);
> +
> +		/* clear ptes and collect pages in free_pages llist */
> +		apply_to_existing_page_range(&init_mm, kaddr, page_cnt << PAGE_SHIFT,
> +					     apply_range_clear_cb, &free_pages);
> +
> +		range_tree_set(&arena->rt, pgoff, page_cnt);
> +	}
> +	raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
> +
> +	/* Iterate the list again without holding spinlock to do the tlb flush and zap_pages */
> +	llist_for_each_safe(pos, t, list) {
> +		s = llist_entry(pos, struct arena_free_span, node);
> +		page_cnt = s->page_cnt;
> +		full_uaddr = user_vm_start + s->uaddr;
                          ^^^^^^^^^^^^^^^^^^^^^^^

Is there a missing clear_lo32() call here? In arena_free_pages() the
sleepable path computes full_uaddr as:

    full_uaddr = clear_lo32(arena->user_vm_start) + uaddr;

But arena_free_worker() computes it as:

    full_uaddr = user_vm_start + s->uaddr;

Since s->uaddr is already the lower 32 bits (set from masked uaddr in
arena_free_pages()), and user_vm_start includes its own lower 32 bits,
this appears to produce the wrong address. For example, if user_vm_start
is 0x7f7d26200000 and s->uaddr is 0x26200000, this would give
0x7f7d4c400000 instead of the correct 0x7f7d26200000.

Should this be:

    full_uaddr = clear_lo32(user_vm_start) + s->uaddr;

to match the sleepable path?

> +		kaddr = arena_vm_start + s->uaddr;
> +
> +		/* ensure no stale TLB entries */
> +		flush_tlb_kernel_range(kaddr, kaddr + (page_cnt * PAGE_SIZE));
> +
> +		/* remove pages from user vmas */
> +		zap_pages(arena, full_uaddr, page_cnt);
> +
> +		kfree_nolock(s);
> +	}

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/20441598118