Re: [PATCH v3] mm/mmu_notifier: Add async OOM cleanup via call_srcu()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Reusing an RCU-protected hlist node for a stack-local list overwrites the RCU pointer, corrupting concurrent SRCU readers.
- [Critical] Disconnecting KVM from the MMU notifier during OOM breaks the KVM retry protocol, allowing guests to access freed host memory.
- [High] The patch synchronously invokes the release callback from the OOM reaper, reintroducing the PREEMPT_RT deadlock it intended to fix.
- [High] Skipping mmu_notifier_unregister on GFP_ATOMIC failure bypasses SRCU synchronization, leading to a Use-After-Free of KVM by active readers.
- [High] Concurrent unregistration races with mmu_notifier_oom_enter's lockless iteration over the local oom_list, causing stack corruption.
- [High] A TOCTOU race exists between kvm_destroy_vm() checking hlist_unhashed() and mmu_notifier_oom_enter() scheduling the SRCU callback, leading to a Use-After-Free.
- [Medium] Duplicated cleanup logic in kvm_destroy_vm triggers a false positive WARN_ON after an OOM event.
--

--- Patch [1]: [PATCH v3] mm/mmu_notifier: Add async OOM cleanup via call_srcu() ---
Note: The format of this review has been altered to use a free-form summary
rather than inline patch quoting due to recitation restrictions.

commit 3eae3d8df937f01d0f1b5da1f0f3daf339692174
Author: shaikh.kamal <shaikhkamal2012@gmail.com>
mm/mmu_notifier: Add async OOM cleanup via call_srcu()

This commit implements asynchronous cleanup of MMU notifiers during OOM kills
to prevent deadlocks on PREEMPT_RT kernels. It uses call_srcu to defer
after_oom_unregister callbacks until after the SRCU grace period.

Link: https://lore.kernel.org/all/CABgObfZQM0Eq1=vzm812D+CAcjOaE1f1QAUqGo5rTzXgLnR9cQ@mail.gmail.com/

[Severity: Critical]
In mmu_notifier_oom_enter(), the code uses hlist_add_head() to move the
subscription to a stack-local list immediately after calling
hlist_del_init_rcu(). Does this overwrite the next pointer of the
RCU-protected node before the grace period elapses?

Concurrent SRCU readers traversing the global list might follow the
overwritten pointer into the stack-allocated list and read garbage data,
potentially causing a panic.

[Severity: Critical]
By removing KVM from the subscriptions list during mmu_notifier_oom_enter(),
does this break the KVM retry protocol?

The OOM reaper will unmap pages while holding mmap_lock for read, but KVM
will miss the invalidate_range_start callbacks. This means mmu_invalidate_seq
won't be incremented. If a concurrent KVM page fault resolves a PFN, it will
pass the sequence retry check and could install a shadow mapping just before
the OOM reaper frees the physical page, which might allow a guest to access
host memory reallocated to other processes.

[Severity: High]
The code iterates over the local list in mmu_notifier_oom_enter() and
synchronously calls the release callback. For KVM,
kvm_mmu_notifier_release() acquires the KVM mmu_lock.

Since the OOM reaper holds the mmap_lock and calls this release callback,
could this recreate the exact AB-BA deadlock between mmap_lock and mmu_lock
on PREEMPT_RT that this patch was intended to fix?

[Severity: High]
If the GFP_ATOMIC allocation fails in mmu_notifier_oom_enter(), the code
continues without scheduling a call_srcu() callback, leaving the node
unhashed.

In kvm_destroy_vm(), if hlist_unhashed() is true, it calls
mmu_notifier_barrier() and then mmdrop(kvm->mm). Because no callback was
queued, srcu_barrier() returns immediately. Could this lead to a
use-after-free of KVM structures by concurrent SRCU readers that are still
traversing the unhashed node?

[Severity: High]
There appears to be a race condition when iterating over the stack-allocated
list in mmu_notifier_oom_enter().

Concurrently, mmu_notifier_unregister() checks if the node is hashed,
acquires the subscriptions lock, and calls hlist_del_init_rcu(). Because the
stack list is iterated locklessly, won't this concurrent deletion modify the
stack-local list pointers, causing data races and potential stack corruption?

[Severity: High]
Is there a time-of-check to time-of-use race between kvm_destroy_vm() and
mmu_notifier_oom_enter()?

The code in mmu_notifier_oom_enter() unhashes the node before calling
call_srcu(). If kvm_destroy_vm() runs in this small window, it will observe
the unhashed state, call the barrier which won't wait since no callback is
queued yet, and then free the KVM structure.

The OOM reaper thread would then queue the callback pointing to the freed
subscription, leading to a use-after-free when the deferred callback
executes.

[Severity: Medium]
In kvm_destroy_vm(), the cleanup logic checks if mn_active_invalidate_count
is non-zero, and if not, triggers a WARN_ON if mmu_invalidate_in_progress is
set.

However, kvm_mmu_notifier_after_oom_unregister() already zeroes
mn_active_invalidate_count. Since the OOM event detached KVM during an
active invalidation, the in-progress count remains positive. Will this
duplicated cleanup logic consistently trigger a false positive WARN_ON during
VM teardown after an OOM event?

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