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

[bot+bpf-ci@kernel.org]

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

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 09d5dc541..527921269 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h

[ ... ]

> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
> index 9d8a8eb44..f330b51de 100644
> --- a/kernel/bpf/arena.c
> +++ b/kernel/bpf/arena.c

[ ... ]

> @@ -327,12 +351,19 @@ static vm_fault_t arena_vm_fault(struct vm_fault *vmf)
>  	struct bpf_arena *arena = container_of(map, struct bpf_arena, map);
>  	struct page *page;
>  	long kbase, kaddr;
> +	unsigned long flags;
>  	int ret;
>
>  	kbase = bpf_arena_get_kern_vm_start(arena);
>  	kaddr = kbase + (u32)(vmf->address);
>
> -	guard(mutex)(&arena->lock);
> +	if (raw_res_spin_lock_irqsave(&arena->spinlock, flags))
> +		/*
> +		 * This is an impossible case and would only trigger if res_spin_lock is buggy or
> +		 * due to another kernel bug.
> +		 */
> +		return VM_FAULT_RETRY;
> +
>  	page = vmalloc_to_page((void *)kaddr);
>  	if (page)
>  		/* already have a page vmap-ed */
> @@ -344,26 +375,30 @@ static vm_fault_t arena_vm_fault(struct vm_fault *vmf)
>
>  	ret = range_tree_clear(&arena->rt, vmf->pgoff, 1);
>  	if (ret)
> -		return VM_FAULT_SIGSEGV;
> +		goto out_unlock_sigsegv;
>
>  	struct apply_range_data data = { .pages = &page, .i = 0 };
>  	/* Account into memcg of the process that created bpf_arena */
>  	ret = bpf_map_alloc_pages(map, NUMA_NO_NODE, 1, &page);
>  	if (ret) {
>  		range_tree_set(&arena->rt, vmf->pgoff, 1);
> -		return VM_FAULT_SIGSEGV;
> +		goto out_unlock_sigsegv;
>  	}
>
>  	ret = apply_to_page_range(&init_mm, kaddr, PAGE_SIZE, apply_range_set_cb, &data);
>  	if (ret) {
>  		range_tree_set(&arena->rt, vmf->pgoff, 1);
> -		__free_page(page);
> -		return VM_FAULT_SIGSEGV;
> +		free_pages_nolock(page, 0);
> +		goto out_unlock_sigsegv;
>  	}
>  out:
> +	raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
>  	page_ref_add(page, 1);
>  	vmf->page = page;
>  	return 0;
> +out_unlock_sigsegv:
> +	raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
> +	return VM_FAULT_SIGSEGV;
>  }

Can the page reference race with arena_free_pages()? The sequence:

  Thread A (arena_vm_fault):
    bpf_map_alloc_pages() -> page refcount=1
    apply_to_page_range() -> maps PTE
    raw_res_spin_unlock_irqrestore()
    page_ref_add(page, 1) -> tries to increment to 2

  Thread B (arena_free_pages):
    raw_res_spin_lock_irqsave()
    apply_to_existing_page_range()->apply_range_clear_cb() -> clears PTE
    raw_res_spin_unlock_irqrestore()
    __free_page(page) -> refcount 1->0, page freed

Between Thread A's unlock and page_ref_add(), Thread B can acquire the
lock, clear the PTE, and free the page. Then Thread A's page_ref_add()
operates on freed memory.

Should page_ref_add() happen before releasing the spinlock, or should
the initial allocation start with refcount=2?

[ ... ]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c

[ ... ]


---
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/19272481461