* Re: [PATCH v3] mm/mmu_notifier: Add async OOM cleanup via call_srcu()
2026-07-22 14:08 [PATCH v3] mm/mmu_notifier: Add async OOM cleanup via call_srcu() shaikh.kamal
@ 2026-07-22 14:22 ` sashiko-bot
2026-07-24 8:59 ` Lorenzo Stoakes (ARM)
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-07-22 14:22 UTC (permalink / raw)
To: shaikh.kamal; +Cc: kvm, linux-rt-devel, linux-kernel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] mm/mmu_notifier: Add async OOM cleanup via call_srcu()
2026-07-22 14:08 [PATCH v3] mm/mmu_notifier: Add async OOM cleanup via call_srcu() shaikh.kamal
2026-07-22 14:22 ` sashiko-bot
@ 2026-07-24 8:59 ` Lorenzo Stoakes (ARM)
1 sibling, 0 replies; 3+ messages in thread
From: Lorenzo Stoakes (ARM) @ 2026-07-24 8:59 UTC (permalink / raw)
To: shaikh.kamal
Cc: Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
Suren Baghdasaryan, Michal Hocko, David Rientjes, Shakeel Butt,
Paolo Bonzini, linux-mm, linux-kernel, kvm, linux-rt-devel,
seanjc, syzbot+c3178b6b512446632bac, kernel test robot
This approach was previously NAK'd so I'm not sure why it's at v3? :) see
below about this, but maybe that was address, doesn't seem so however.
(I'd also suggest you don't use gmail, it bounces kernel mail as spam a lot
now and may result in you not receiving responses - see
https://korg.docs.kernel.org/linuxdev.html for a free linux.dev account as
an alternative).
On Wed, Jul 22, 2026 at 07:38:03PM +0530, shaikh.kamal wrote:
> When an mm undergoes OOM kill, the OOM reaper unmaps memory while
> holding the mmap_lock in a non-blocking context set up by
> mmu_notifier_invalidate_range_start_nonblock(). MMU notifier
> subscribers (notably KVM) acquire sleeping locks in their
> invalidate callbacks, which deadlocks on PREEMPT_RT where
> spinlock_t is a sleeping rt_mutex:
>
> BUG: sleeping function called from invalid context at
> kernel/locking/spinlock_rt.c:48
> in_atomic(): 0, irqs_disabled(): 0, non_block: 1, pid: 40,
> name: oom_reaper
> Call Trace:
> rt_spin_lock
> kvm_mmu_notifier_invalidate_range_start
> __mmu_notifier_invalidate_range_start
> zap_vma_for_reaping
> __oom_reap_task_mm
>
> Implement the asynchronous cleanup design proposed by Paolo
> Bonzini in v1 review: a new optional after_oom_unregister
> callback in struct mmu_notifier_ops, invoked after the SRCU grace
> period via call_srcu() so that no readers can still reference the
> subscription when cleanup runs.
>
> The flow is:
>
> 1. The OOM reaper calls mmu_notifier_oom_enter() from
> __oom_reap_task_mm(), before the non-blocking VMA zap loop.
> 2. mmu_notifier_oom_enter() walks the subscription list and, for
> each subscriber that provides after_oom_unregister, detaches
> the subscription from the active list and schedules a
> call_srcu() callback. The reaper's subsequent invalidations
> therefore never invoke the subscriber's callbacks.
> 3. The deferred callback invokes after_oom_unregister once the
> grace period has elapsed and all in-flight readers have
> finished.
> 4. Subsystems waiting to free structures referenced by the
> callback can call the new mmu_notifier_barrier() helper, which
> wraps srcu_barrier() to wait for all outstanding callbacks
> scheduled this way.
>
> Paolo's original sketch called synchronize_srcu() from
> mmu_notifier_oom_enter() before invoking the callbacks. That
> blocks the OOM reaper waiting for a grace period and can deadlock:
> the reaper holds mmap_lock while waiting for SRCU readers to
> drain, while an in-flight reader can itself be blocked on
> mmap_lock. Use call_srcu() instead so the reaper never waits; the
> callback runs asynchronously once the grace period elapses, and
> mmu_notifier_barrier() provides the synchronization point for
> teardown paths that need to wait.
Hmm I don't think mmu notifier callbacks can make assumptions about mmap
lock, do you mean there's an implicit dependency on the lock somehow?
Please be a lot more specific about the deadlock scenario.
>
> after_oom_unregister is mutually exclusive with alloc_notifier
> because allocated notifiers can have additional outstanding
> references that the OOM path cannot safely drop.
>
> Update KVM to provide after_oom_unregister, which clears
> mn_active_invalidate_count, and to detect via hlist_unhashed() in
> kvm_destroy_vm() when its subscription was already detached by the
> OOM path; in that case call mmu_notifier_barrier() and drop the mm
> reference rather than calling mmu_notifier_unregister().
>
> Tested under virtme-ng with PREEMPT_RT, KASAN, and lockdep
> enabled, using a minimal KVM test program (opens /dev/kvm, creates
> a VM, registers memory, creates a vCPU, and sleeps) driven through
> a CONFIG_DEBUG_VM-only debugfs trigger (not part of this patch)
> that invokes __oom_reap_task_mm() on the target task. With the
> patch applied, mmu_notifier_oom_enter() detaches the KVM
> subscription, the call_srcu() callback runs after the SRCU grace
> period, KVM's after_oom_unregister clears
> mn_active_invalidate_count, and mmu_notifier_barrier() returns
> cleanly in kvm_destroy_vm(), with no KASAN reports, kernel BUGs,
> or lockdep splats across 20 stress iterations. The same setup
> reproduces the syzbot-reported warning on an unpatched PREEMPT_RT
> kernel; it is no longer observed with this patch applied.
It'd be good if you could provide a minimal reproducer, especially since
syzbot wasn't able to come up with one.
>
> If the GFP_ATOMIC allocation in mmu_notifier_oom_enter() fails,
> the after_oom_unregister callback for that subscription is
> skipped rather than retried, since retrying could sleep and
> reintroduce the deadlock this patch fixes. The subscription is
> still cleaned up later via the normal unregister path.
>
> Fixes: 52ac8b358b0c ("KVM: Block memslot updates across range_start() and range_end()")
You need a Cc: stable in mm for backports.
But hmm backporting an entirely new mmu notifier mechanism like this is
really really questionable.
Weren't you also told that this predates that commit anyway and should be
commit e930bffe95e1 ("KVM: Synchronize guest physical memory map to host
virtual memory map")?...
https://lore.kernel.org/all/aYyhfvC_2s000P7H@google.com/
I'm a little concerned that review is not being addressed here.
> Reported-by: syzbot+c3178b6b512446632bac@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=c3178b6b512446632bac
This isn't a report it's a syzbot control panel?
You mean https://lore.kernel.org/all/6936812a.a70a0220.38f243.0090.GAE@google.com/ right?
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Link: https://lore.kernel.org/all/CABgObfZQM0Eq1=vzm812D+CAcjOaE1f1QAUqGo5rTzXgLnR9cQ@mail.gmail.com/
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202605031109.uxckW5L3-lkp@intel.com/
We don't do Reported-by/Closes on stuff that's not upstream especially not
on previous revisions of a not yet accepted patch.
>
> Signed-off-by: shaikh.kamal <shaikhkamal2012@gmail.com>
Hmm, this approach was NAK'd before but you're pressing ahead with it
unless I'm missing something:
https://lore.kernel.org/all/aasD2OHkQN3kdRba@google.com/
You never responded to that - could you address that please?
I'd also like to hear from Michal and David on this also ideally. But my
2p:
You're allocating GFP_ATOMIC when the OOM killer reaper runs (!!) using reserves
that are intended for absolutely critical atomic context stuff (IRQ etc.) for an
MMU notifier event.
This seems nuts to me, and you're letting it be unbounded on MMU notifier
registrations AFAICT.
I also don't think something like this is backportable esp. to a 2021 (!) Fixes
tag.
I would suggest trying a completely different approach.
In general gating things on whether the oom_reap (renamed...!) op is
defined then having mmu_notifier pre-allocate the callback stuff (or reuse
a place that's already allocated) seems a better way.
But since it's intended for a fix (+ therefore backport) is there not some
simpler resolution that doesn't require actively extending the mmu_notifier
logic to do completely new stuff?
I'm not really comfortable with something like that being backported.
You can always do a 'dumb' fix for backport and a more forward thinking one
for upstream.
> ---
> Changes in v3:
> - Rebase onto v7.2-rc3; v2 was based on stale v7.0 which the
> kernel test robot could not apply to current trees
> - Add missing static inline stub for mmu_notifier_oom_enter() in
> the !CONFIG_MMU_NOTIFIER section of include/linux/mmu_notifier.h,
> fixing allnoconfig build failures reported by the kernel test
> robot
> - Use kmalloc_obj() per current allocation idiom
> - Add Fixes: tag identifying the commit that introduced
> mn_invalidate_lock
>
> Changes in v2:
> - Complete redesign per Paolo's v1 review: moved from a
> KVM-internal locking change to a new mm/mmu_notifier
> after_oom_unregister callback with call_srcu() async cleanup
> (hence the subject prefix change from KVM: to mm/)
> - Add mmu_notifier_barrier() (srcu_barrier wrapper) for teardown
> synchronization in kvm_destroy_vm()
> - Move call site to __oom_reap_task_mm(); use hlist_del_init() to
> keep hlist_unhashed() correct and avoid use-after-free on the
> stack-allocated oom_list head
>
> v2: https://lore.kernel.org/all/20260429222548.25475-1-shaikhkamal2012@gmail.com/
> v1: https://lore.kernel.org/all/20260209161527.31978-1-shaikhkamal2012@gmail.com/
>
> include/linux/mmu_notifier.h | 14 ++++
> mm/mmu_notifier.c | 122 +++++++++++++++++++++++++++++++++++
> mm/oom_kill.c | 3 +
> virt/kvm/kvm_main.c | 27 +++++++-
> 4 files changed, 165 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/mmu_notifier.h b/include/linux/mmu_notifier.h
> index a11a44eef521..a1820a487854 100644
> --- a/include/linux/mmu_notifier.h
> +++ b/include/linux/mmu_notifier.h
> @@ -88,6 +88,14 @@ struct mmu_notifier_ops {
> void (*release)(struct mmu_notifier *subscription,
> struct mm_struct *mm);
>
> + /*
> + * Any mmu notifier that defines this is automatically unregistered
> + * when its mm is the subject of an OOM kill. after_oom_unregister()
> + * is invoked after all other outstanding callbacks have terminated.
> + */
> + void (*after_oom_unregister)(struct mmu_notifier *subscription,
> + struct mm_struct *mm);
This is really confusing. You subscribe to an event that then causes that event
to actually happen? And you are defining a new event specifically for it?
It definitely needs renaming, I read the comment and the name and think
'what?' :)
How about simply 'oom_reaped' or something like that?
> +
> /*
> * clear_flush_young is called after the VM is
> * test-and-clearing the young/accessed bitflag in the
> @@ -424,6 +432,8 @@ bool __mmu_notifier_clear_young(struct mm_struct *mm,
> unsigned long start, unsigned long end);
> bool __mmu_notifier_test_young(struct mm_struct *mm,
> unsigned long address);
> +void mmu_notifier_oom_enter(struct mm_struct *mm);
This seems like a bad name. Enter implies exit, and this is at the point of the
reaper being invoked also, seems better to put that in the name.
Good also to have it resemble the event name it triggers?
> +void mmu_notifier_barrier(void);
> extern int __mmu_notifier_invalidate_range_start(struct mmu_notifier_range *r);
> extern void __mmu_notifier_invalidate_range_end(struct mmu_notifier_range *r);
> extern void __mmu_notifier_arch_invalidate_secondary_tlbs(struct mm_struct *mm,
> @@ -643,6 +653,10 @@ static inline void mmu_notifier_synchronize(void)
> {
> }
>
> +static inline void mmu_notifier_oom_enter(struct mm_struct *mm)
> +{
> +}
> +
> #endif /* CONFIG_MMU_NOTIFIER */
>
> #endif /* _LINUX_MMU_NOTIFIER_H */
> diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c
> index 245b74f39f91..f30bf95f17c7 100644
> --- a/mm/mmu_notifier.c
> +++ b/mm/mmu_notifier.c
> @@ -49,6 +49,37 @@ struct mmu_notifier_subscriptions {
> struct hlist_head deferred_list;
> };
>
> +/*
> + * Callback structure for asynchronous OOM cleanup.
> + * Used with call_srcu() to defer after_oom_unregister callbacks
> + * until after SRCU grace period completes.
> + */
> +struct mmu_notifier_oom_callback {
> + struct rcu_head rcu;
> + struct mmu_notifier *subscription;
> + struct mm_struct *mm;
> +};
> +
> +/*
> + * Callback function invoked after SRCU grace period.
> + * Safely calls after_oom_unregister once all readers have finished.
> + */
> +static void mmu_notifier_oom_callback_fn(struct rcu_head *rcu)
> +{
> + struct mmu_notifier_oom_callback *cb =
> + container_of(rcu, struct mmu_notifier_oom_callback, rcu);
> +
> + /* Safe - all SRCU readers have finished */
> + cb->subscription->ops->after_oom_unregister(cb->subscription, cb->mm);
> +
> + /* Release mm reference taken when callback was scheduled */
> + WARN_ON_ONCE(atomic_read(&cb->mm->mm_count) <= 0);
I'm quite concerned that you think this is likely enough for a
WARN_ON_ONCE()... You really shouldn't be accessing this field directly. And if
you're seeing a 0 refcount you're already effectively doing a UAF so what's the
point?
> + mmdrop(cb->mm);
Hmm but you're also mmdrop()'ing in the KVM callback?
> +
> + /* Free callback structure */
> + kfree(cb);
> +}
> +
> /*
> * This is a collision-retry read-side/write-side 'lock', a lot like a
> * seqcount, however this allows multiple write-sides to hold it at
> @@ -385,6 +416,84 @@ void __mmu_notifier_release(struct mm_struct *mm)
> mn_hlist_release(subscriptions, mm);
> }
>
> +void mmu_notifier_oom_enter(struct mm_struct *mm)
This function badly needs a comment, kdoc please.
> +{
> + struct mmu_notifier_subscriptions *subscriptions =
> + mm->notifier_subscriptions;
This indentation is absolutely wild :) please find a better way.
If you call the var 'subs' it can fit on one line ;)
> + struct mmu_notifier *subscription;
> + struct hlist_node *tmp;
> + HLIST_HEAD(oom_list);
> + int id;
> +
> + if (!subscriptions)
> + return;
> +
> + id = srcu_read_lock(&srcu);
> +
> + /*
> + * Prevent further calls to the MMU notifier, except for
> + * release and after_oom_unregister.
> + */
> + spin_lock(&subscriptions->lock);
> + hlist_for_each_entry_safe(subscription, tmp,
> + &subscriptions->list, hlist) {
> + if (!subscription->ops->after_oom_unregister)
> + continue;
> +
> + /*
> + * after_oom_unregister and alloc_notifier are incompatible,
> + * because there could be other references to allocated
> + * notifiers.
> + */
> + if (WARN_ON(subscription->ops->alloc_notifier))
> + continue;
Probably better to check this at registration time no?
> +
> + hlist_del_init_rcu(&subscription->hlist);
> + hlist_add_head(&subscription->hlist, &oom_list);
> + }
> + spin_unlock(&subscriptions->lock);
> + hlist_for_each_entry(subscription, &oom_list, hlist)
> + if (subscription->ops->release)
> + subscription->ops->release(subscription, mm);
> +
> + srcu_read_unlock(&srcu, id);
> +
> + if (hlist_empty(&oom_list))
> + return;
> +
> + hlist_for_each_entry_safe(subscription, tmp,
> + &oom_list, hlist) {
> + struct mmu_notifier_oom_callback *cb;
> + /*
> + * Remove from stack-based oom_list and reset hlist to unhashed state.
> + * This sets subscription->hlist.pprev = NULL, so future callers of
> + * mmu_notifier_unregister() (e.g. kvm_destroy_vm) will see
> + * hlist_unhashed() == true and take the safe path, avoiding
> + * use-after-free on the stack-allocated oom_list head.
> + */
This seems really fragile and 'just so' to suit a use case.
> + hlist_del_init(&subscription->hlist);
> +
> + /*
> + * GFP_ATOMIC failure is exceedingly rare. We cannot sleep
Really? Under memory pressure enough that the OOM killer has been invoked
and when only emergency memory reserves (which you're now depleting) are
available?
Allocating on an OOM path is deeply questionable in general.
Allocating _for each subscription_ seems nuts?
Also aren't you definitely using additional reserves this way because the OOM
killer is underway, reserves intended for _absolutely critical_ tasks to do with
OOM, not MMU notifier events?
Wouldn't it just be better to have pre-allocated this at registration
time or embed this somewhere that's already allocated?
> + * here (would reintroduce the deadlock this patch fixes)
> + * and cannot call after_oom_unregister synchronously
> + * without first waiting for SRCU readers. The subscriber
> + * will not receive after_oom_unregister but cleanup will
> + * eventually happen via the unregister path.
> + */
> + cb = kmalloc_obj(*cb, GFP_ATOMIC);
See above re: the craziness of this.
Couldn't you use mmu_notifier->rcu_head?
It's only used if alloc_notifier is specified which you explicitly disallow.
That way you don't need an allocation at all.
> + if (!cb)
> + continue;
If you can't allocate, loop around and try another allocation afterwards?
That also seems crazy? You should bail out. But really you shouldn't be
allocating like this here at all as per above.
But in general - you're resolving a bug by doing this but it's ok to just
give up and allow the deadlock if you can't allocate?
Then it's not a fix at all?
> +
> + cb->subscription = subscription;
> + cb->mm = mm;
> + mmgrab(mm);
> +
> + /* Schedule callback - returns immediately */
We don't need to have comments explaining how core RCU functions work :)
> + call_srcu(&srcu, &cb->rcu, mmu_notifier_oom_callback_fn);
> + }
> +}
> +
> /*
> * If no young bitflag is supported by the hardware, ->clear_flush_young can
> * unmap the address and return 1 or 0 depending if the mapping previously
> @@ -1144,3 +1253,16 @@ void mmu_notifier_synchronize(void)
> synchronize_srcu(&srcu);
> }
> EXPORT_SYMBOL_GPL(mmu_notifier_synchronize);
> +
> +/**
> + * mmu_notifier_barrier - Wait for all pending MMU notifier callbacks
> + *
> + * Waits for all call_srcu() callbacks scheduled by mmu_notifier_oom_enter()
> + * to complete. Used by subsystems during cleanup to prevent use-after-free
> + * when destroying structures accessed by the callbacks.
> + */
> +void mmu_notifier_barrier(void)
> +{
> + srcu_barrier(&srcu);
> +}
> +EXPORT_SYMBOL_GPL(mmu_notifier_barrier);
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 5f372f6e26fa..66adcd03f36a 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -516,6 +516,9 @@ static bool __oom_reap_task_mm(struct mm_struct *mm)
> bool ret = true;
> MA_STATE(mas, &mm->mm_mt, ULONG_MAX, ULONG_MAX);
>
> + /* Notify MMU notifiers about the OOM event */
This comment is superfluous.
> + mmu_notifier_oom_enter(mm);
> +
> /*
> * Tell all users of get_user/copy_from_user etc... that the content
> * is no longer stable. No barriers really needed because unmapping
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index e44c20c04961..79a4df8a337a 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -875,6 +875,24 @@ static void kvm_mmu_notifier_release(struct mmu_notifier *mn,
> srcu_read_unlock(&kvm->srcu, idx);
> }
>
> +static void kvm_mmu_notifier_after_oom_unregister(struct mmu_notifier *mn,
> + struct mm_struct *mm)
> +{
> + struct kvm *kvm;
> +
> + kvm = mmu_notifier_to_kvm(mn);
> +
> + /*
> + * At this point the unregister has completed and all other callbacks
> + * have terminated. Clean up any unbalanced invalidation counts.
> + */
The comment seems unrelated to the WARN_ON()?
> + WARN_ON(rcuwait_active(&kvm->mn_memslots_update_rcuwait));
Is this actually possible?
It's not great to leave WARN_ON()'s in a patch that's intended to be
backported that seem to be 'oh maybe I didn't get this patch right'.
We don't want that kind of thing as part of a _fix_ you should be
absolutely certain the fix works.
Also WARN_ON_ONCE() maybe?
> + if (kvm->mn_active_invalidate_count)
> + kvm->mn_active_invalidate_count = 0;
> + else
> + WARN_ON(kvm->mmu_invalidate_in_progress);
Why is this separate and predicated on !mn_active_invalidate_count?
Seems simpler to just group the warnings together with comments explaining
_why_.
But as per above the fact you're putting 'maybe my patch is wrong' warnings
here worries me.
> +}
> +
> static const struct mmu_notifier_ops kvm_mmu_notifier_ops = {
> .invalidate_range_start = kvm_mmu_notifier_invalidate_range_start,
> .invalidate_range_end = kvm_mmu_notifier_invalidate_range_end,
> @@ -882,6 +900,7 @@ static const struct mmu_notifier_ops kvm_mmu_notifier_ops = {
> .clear_young = kvm_mmu_notifier_clear_young,
> .test_young = kvm_mmu_notifier_test_young,
> .release = kvm_mmu_notifier_release,
> + .after_oom_unregister = kvm_mmu_notifier_after_oom_unregister,
> };
>
> static int kvm_init_mmu_notifier(struct kvm *kvm)
> @@ -1273,7 +1292,13 @@ static void kvm_destroy_vm(struct kvm *kvm)
> kvm->buses[i] = NULL;
> }
> kvm_coalesced_mmio_free(kvm);
> - mmu_notifier_unregister(&kvm->mmu_notifier, kvm->mm);
> + if (hlist_unhashed(&kvm->mmu_notifier.hlist)) {
I really hate that you're accessing internals like this to figure out
state. Is it also safe to access it like this without a barrier of any
kind?
You'd at least want a helper wrapped around this...
And is KVM the only place that needs updating here? You've created a whole
new paradigm here surely needs to be reflected elsewhere?
> + /* Subscription removed by OOM. Wait for async callback. */
> + mmu_notifier_barrier();
It seems like you've decided 'why not both' on synchronisation vs. callback :)
> + mmdrop(kvm->mm);
I really don't like that you expose mm internals here.
It seems to me that this logic should be part of the standard unregister
procedure, gated on the 'after_oom_unregister' (preferably renamed) op
being defined.
> + } else {
> + mmu_notifier_unregister(&kvm->mmu_notifier, kvm->mm);
This code should _just be_ mmu_notifier_unregister() as before.
> + }
> /*
> * At this point, pending calls to invalidate_range_start()
> * have completed but no more MMU notifiers will run, so
>
> base-commit: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
> --
> 2.43.0
>
I also asked AI to have a look at this and it points out process_mrelease()
has no capability check, so you can trigger this from an unprivileged
process (on its own killed child process for instance).
Is that intended? That seems possibly problematic?
Cheers, Lorenzo
^ permalink raw reply [flat|nested] 3+ messages in thread