Re: [PATCH bpf-next v4 7/8] bpf: Synchronize arena page faults with page freeing

["Emil Tsalapatis" <emil@etsalapatis.com>]

On Fri Oct 16, 2026 at 2:02 AM UTC, Alexei Starovoitov wrote:
> On Wed, Oct 14, 2026 at 11:34 PM Emil Tsalapatis <emil@etsalapatis.com> wrote:
>> Prevent the arena VM fault handler from running in parallel with arena free
>> operations. This is similar to the policy for file-backed mappings. Use a
>> per-arena reference counter to exclude faults from synchronous and deferred
>> page teardown, and vice versa. Faults and frees can each run concurrently
>> with operations of the same type.
>
> In v3 I suggested to lock the page, recheck vmalloc_to_page() and
> return VM_FAULT_LOCKED. The recheck is missing and opref is there
> instead of it.
> The free path clears the pte under arena->spinlock and then does
> lock_page(); unlock_page();
> If arena_vm_fault() locked the page first the free waits for
> finish_fault() and zap_pages() removes that pte.
> If the free was first arena_vm_fault() sees different vmalloc_to_page()
> under the page lock. Unlock, put and start over.
> The fault holds a page ref, so the same page cannot come back.
> The free path holds nothing when it gets to zap_pages(), so
> mmap_read_lock() there is fine and patch 1 is not needed.

Makes sense to remove the opref (and a lot of the rest of the machinery)
since it's causing problems. There's caveats with the recommended locking,
while I think it'll be easier to just discuss it in the next version
more concretely but it's still worth pointing out some the caveats here.
If any of the below is wrong we can continue discussing on v5 that is
likely to be closer to where we want this patchset to look like.

1) The free path holds nothing, but the program calling it can. A
sleepable iter/task_vma program runs with the iterated task's mmap_lock
held for read, and so does a bpf_find_vma() callback in a sleepable
program. If either frees pages that this mm maps, zap_pages() takes the
same mmap_lock again. A writer that queues in between waits for the
first read lock, and the second one ends up waiting for the writer.
zap_pages() has to use a trylock.

With a trylock, a free can fail after it has cleared the ptes. It then
has to defer the zap, and the range stays allocated without ptes until
the worker is done. In your 0/8 reply you suggested returning the
scratch_page error for a page that is being freed, so every user fault
on the range gets SIGBUS for that whole time. If allocating the deferred
span fails, the pages cannot be freed, because user ptes may still map
them, and the page table walk in arena_map_free() no longer finds them.

The SIGBUS itself is a problem too. A fault on a page that is being
freed has never raised a signal. User space cannot tell that a free is
in progress, and freeing and reallocating is normal arena use.

I have a next version that takes the trylocks before it touches the
range, and defers before clearing any pte if one fails.

2) As you mentioned the range tree already shows when a page is reserved
or being freed, but does not distinguish between the two. Only a reserved
page should fail. A fault could find out by waiting for arena->lock,
which a free holds while retiring, and checking again. But it would have
to decide with the fault lock held, so every fault on a reserved page would
sleep through any free in progress with mmap_lock held.

The next version avoids this by recording the range it tears down as a page
offset + count. It sets the record under arena->spinlock in the same
section where it calls range_tree_clear() and clears the ptes, and drops
it when it calls range_tree_set(). Frees are serialized on arena->lock,
so there is at most one such range, and recording it allocates nothing.

A fault inside that range waits for the free to finish, and can drop the
fault lock first. Any other fault on an allocated range without a page
is on a reserved range and gets the scratch_page error right away, as
you suggested.

A range without kernel ptes is released right away under
arena->spinlock, without arena->lock or mmap_lock. buddy_chunk_get()
frees a reserved range before reallocating it, so its free is never
deferred in sleepable context. A fault on the range being freed does not
take arena->lock either, so it cannot make another free fail its
trylock. It sleeps until the free bumps a counter as it releases the
range and wakes it with wake_up_var(). 

A fault that cannot retry sleeps with mmap_lock held, which is safe
because a free only trylocks mmap_lock and never waits fault. Other
faults are not affected. A sleepable free of a populated range is
deferred only when one of its trylocks fails: while another free, an
mmap() or munmap() of the arena, or the worker holds arena->lock, or
while a writer holds or waits for one of the mms' mmap_lock.
Non-sleepable frees are always deferred, as before.

> See
> commit bb9b9eb0ae2e ("scsi: target: tcmu: Fix possible data corruption").
> Same bug.
>
> [...]
>>  	if (!sleepable)
>>  		goto defer;
>> +	if (!arena_opref_get(arena, ARENA_OPREF_FREE))
>> +		goto defer;
>
> One user thread faulting on any page of the arena turns sleepable
> bpf_arena_free_pages() of unrelated page into deferred one.
> buddy_chunk_get() in libarena does
> bpf_arena_free_pages(vaddr); bpf_arena_alloc_pages(vaddr);
> The alloc fails and that vaddr is lost.
> The other way around any free makes all faults in the arena spin in
> VM_FAULT_RETRY, and the worker spins in schedule_work() while a fault
> sleeps in alloc_pages_node(GFP_KERNEL).
> Giving frees a priority over faults, as you replied to bpf-ci, makes it
> even heavier.