Re: [PATCH bpf-next v6 4/4] selftests/bpf: Add a test for arena fault-in under memory.max

[Khawar Ahemad <ahemadkhawar123@gmail.com>]

Addressing the three CI review comments on the v6 series:

---
[Patch 4/4] max_entries comment
---

> could the 50000 carry a short note that it just has to outrun
> ARENA_BUDGET on the smallest page size

Accepted. Will be fixed in v7 as a proper preceding block comment:

  /*
   * 50000 pages must exceed ARENA_BUDGET / PAGE_SIZE (64M / 4k = 16384)
   * so the fault loop hits memory.max before exhausting the arena itself.
   */
  __uint(max_entries, 50000);

---
[Patch 1/4] bpf_map_alloc_page_sleepable() reentrancy documentation
---

> Michal Hocko asked how bpf_map_alloc_page_sleepable() achieves safety
> from mm reentrancy, noting that sleepable context alone doesn't
> guarantee safety.

Accepted. Michal's concern is correct: "sleepable" does not automatically
imply "non-reentrant with respect to mm". The comment will be expanded in
v7 to document the reentrancy contract explicitly:

  The only current caller is arena_vm_fault(), which is invoked from
  handle_mm_fault() before taking arena->spinlock and before any BPF
  subsystem lock. mmap_lock is held shared by the fault path, but the
  allocator only needs it for vma lookup which it does not perform here.
  No BPF-internal lock is held at the call site, so allocator reentrancy
  through mm is not possible.

---
[Patch 2/4] Lockless probe race and non-blocking fallback
---

> Doesn't this reintroduce the exact problem the patch aims to solve?

No. The residual race is intentional, bounded, and qualitatively different
from the bug being fixed. Here is why:

The original bug was on the NORMAL, non-racy path: every arena fault where
no page existed would unconditionally use the non-blocking allocator, so
any routine memory.max event killed the process. The fix eliminates that
by preallocating with the sleepable allocator before the spinlock.

The fallback path (!new_page under the lock) is reached only when ALL of
the following are simultaneously true:

  (a) The lockless probe saw a page (a BPF program allocated one).
  (b) That page was freed between the probe and the lock acquisition.
  (c) The resulting non-blocking allocation also fails (memory.max hit
      in that same narrow window).

For (c) to occur independently of (a)+(b), the system must be under
memory pressure severe enough that a non-blocking atomic allocation fails
at the exact same moment the BPF program is freeing a page. A BPF program
that frees arena pages by definition had successfully allocated them
moments earlier, so available memory exists nearby. The likelihood of the
non-blocking allocator failing in this window is therefore extremely low.

More importantly, moving the probe inside the locked region is not
architecturally possible: the purpose of the probe is to decide WHETHER to
preallocate with the sleepable allocator. That decision must happen before
the lock is taken, because sleeping is not allowed inside the spinlock. The
probe-then-preallocate-then-lock sequence is the same pattern used by
do_anonymous_page() and do_cow_fault() in mm/memory.c.

On failure in the race path, VM_FAULT_SIGBUS is returned (not
VM_FAULT_SIGSEGV), which is the correct signal for a resource failure as
opposed to an addressing violation. The commit message's claim ("prevents
a routine memory.max into a fake segfault") refers to the normal path and
remains accurate.

Will add an additional sentence to the !new_page comment in v7 making
clear that this path cannot be sleepable by design.

Khawar Ahemad <ahemadkhawar123@gmail.com>