* [PATCH v2] mm/mmu_notifiers: Esnure range_end() is paired with range_start()
@ 2021-03-11 18:00 Sean Christopherson
2021-03-11 23:28 ` Jason Gunthorpe
0 siblings, 1 reply; 2+ messages in thread
From: Sean Christopherson @ 2021-03-11 18:00 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, linux-mm, Jason Gunthorpe, David Rientjes,
Ben Gardon, Michal Hocko, Jérôme Glisse,
Andrea Arcangeli, Johannes Weiner, Dimitri Sivanich,
Sean Christopherson
If one or more notifiers fails .invalidate_range_start(), invoke
.invalidate_range_end() for "all" notifiers. If there are multiple
notifiers, those that did not fail are expecting _start() and _end() to
be paired, e.g. KVM's mmu_notifier_count would become imbalanced.
Disallow notifiers that can fail _start() from implementing _end() so
that it's unnecessary to either track which notifiers rejected _start(),
or had already succeeded prior to a failed _start().
Note, the existing behavior of calling _start() on all notifiers even
after a previous notifier failed _start() was an unintented "feature".
Make it canon now that the behavior is depended on for correctness.
As of today, the bug is likely benign:
1. The only caller of the non-blocking notifier is OOM kill.
2. The only notifiers that can fail _start() are the i915 and Nouveau
drivers.
3. The only notifiers that utilize _end() are the SGI UV GRU driver
and KVM.
4. The GRU driver will never coincide with the i195/Nouveau drivers.
5. An imbalanced kvm->mmu_notifier_count only causes soft lockup in the
_guest_, and the guest is already doomed due to being an OOM victim.
Fix the bug now to play nice with future usage, e.g. KVM has a potential
use case for blocking memslot updates in KVM while an invalidation is
in-progress, and failure to unblock would result in said updates being
blocked indefinitely and hanging.
Found by inspection. Verified by adding a second notifier in KVM that
periodically returns -EAGAIN on non-blockable ranges, triggering OOM,
and observing that KVM exits with an elevated notifier count.
Fixes: 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu notifiers")
Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
Cc: stable@vger.kernel.org
Cc: David Rientjes <rientjes@google.com>
Cc: Ben Gardon <bgardon@google.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: "Jérôme Glisse" <jglisse@redhat.com>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Dimitri Sivanich <dimitri.sivanich@hpe.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
v2: Reimplemented as suggested by Jason. Only functional change relative
to Jason's suggestion is to check invalidate_range_end before calling to
avoid a NULL pointer dereference. I also added more comments, hopefully
they're helpful...
v1: https://lkml.kernel.org/r/20210310213117.1444147-1-seanjc@google.com
include/linux/mmu_notifier.h | 10 +++++-----
mm/mmu_notifier.c | 23 +++++++++++++++++++++++
2 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/include/linux/mmu_notifier.h b/include/linux/mmu_notifier.h
index b8200782dede..1a6a9eb6d3fa 100644
--- a/include/linux/mmu_notifier.h
+++ b/include/linux/mmu_notifier.h
@@ -169,11 +169,11 @@ struct mmu_notifier_ops {
* the last refcount is dropped.
*
* If blockable argument is set to false then the callback cannot
- * sleep and has to return with -EAGAIN. 0 should be returned
- * otherwise. Please note that if invalidate_range_start approves
- * a non-blocking behavior then the same applies to
- * invalidate_range_end.
- *
+ * sleep and has to return with -EAGAIN if sleeping would be required.
+ * 0 should be returned otherwise. Please note that notifiers that can
+ * fail invalidate_range_start are not allowed to implement
+ * invalidate_range_end, as there is no mechanism for informing the
+ * notifier that its start failed.
*/
int (*invalidate_range_start)(struct mmu_notifier *subscription,
const struct mmu_notifier_range *range);
diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c
index 61ee40ed804e..459d195d2ff6 100644
--- a/mm/mmu_notifier.c
+++ b/mm/mmu_notifier.c
@@ -501,10 +501,33 @@ static int mn_hlist_invalidate_range_start(
"");
WARN_ON(mmu_notifier_range_blockable(range) ||
_ret != -EAGAIN);
+ /*
+ * We call all the notifiers on any EAGAIN,
+ * there is no way for a notifier to know if
+ * its start method failed, thus a start that
+ * does EAGAIN can't also do end.
+ */
+ WARN_ON(ops->invalidate_range_end);
ret = _ret;
}
}
}
+
+ if (ret) {
+ /*
+ * Must be non-blocking to get here. If there are multiple
+ * notifiers and one or more failed start, any that succeeded
+ * start are expecting their end to be called. Do so now.
+ */
+ hlist_for_each_entry_rcu(subscription, &subscriptions->list,
+ hlist, srcu_read_lock_held(&srcu)) {
+ if (!subscription->ops->invalidate_range_end)
+ continue;
+
+ subscription->ops->invalidate_range_end(subscription,
+ range);
+ }
+ }
srcu_read_unlock(&srcu, id);
return ret;
--
2.31.0.rc2.261.g7f71774620-goog
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v2] mm/mmu_notifiers: Esnure range_end() is paired with range_start()
2021-03-11 18:00 [PATCH v2] mm/mmu_notifiers: Esnure range_end() is paired with range_start() Sean Christopherson
@ 2021-03-11 23:28 ` Jason Gunthorpe
0 siblings, 0 replies; 2+ messages in thread
From: Jason Gunthorpe @ 2021-03-11 23:28 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andrew Morton, linux-kernel, linux-mm, David Rientjes,
Ben Gardon, Michal Hocko, Jérôme Glisse,
Andrea Arcangeli, Johannes Weiner, Dimitri Sivanich
On Thu, Mar 11, 2021 at 10:00:57AM -0800, Sean Christopherson wrote:
> If one or more notifiers fails .invalidate_range_start(), invoke
> .invalidate_range_end() for "all" notifiers. If there are multiple
> notifiers, those that did not fail are expecting _start() and _end() to
> be paired, e.g. KVM's mmu_notifier_count would become imbalanced.
> Disallow notifiers that can fail _start() from implementing _end() so
> that it's unnecessary to either track which notifiers rejected _start(),
> or had already succeeded prior to a failed _start().
>
> Note, the existing behavior of calling _start() on all notifiers even
> after a previous notifier failed _start() was an unintented "feature".
> Make it canon now that the behavior is depended on for correctness.
>
> As of today, the bug is likely benign:
>
> 1. The only caller of the non-blocking notifier is OOM kill.
> 2. The only notifiers that can fail _start() are the i915 and Nouveau
> drivers.
> 3. The only notifiers that utilize _end() are the SGI UV GRU driver
> and KVM.
> 4. The GRU driver will never coincide with the i195/Nouveau drivers.
> 5. An imbalanced kvm->mmu_notifier_count only causes soft lockup in the
> _guest_, and the guest is already doomed due to being an OOM victim.
>
> Fix the bug now to play nice with future usage, e.g. KVM has a potential
> use case for blocking memslot updates in KVM while an invalidation is
> in-progress, and failure to unblock would result in said updates being
> blocked indefinitely and hanging.
>
> Found by inspection. Verified by adding a second notifier in KVM that
> periodically returns -EAGAIN on non-blockable ranges, triggering OOM,
> and observing that KVM exits with an elevated notifier count.
>
> Fixes: 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu notifiers")
> Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: stable@vger.kernel.org
> Cc: David Rientjes <rientjes@google.com>
> Cc: Ben Gardon <bgardon@google.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: "Jérôme Glisse" <jglisse@redhat.com>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Dimitri Sivanich <dimitri.sivanich@hpe.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
>
> v2: Reimplemented as suggested by Jason. Only functional change relative
> to Jason's suggestion is to check invalidate_range_end before calling to
> avoid a NULL pointer dereference. I also added more comments, hopefully
> they're helpful...
>
> v1: https://lkml.kernel.org/r/20210310213117.1444147-1-seanjc@google.com
Looks fine, thanks. I think you need some commit message remark to
discourage backporting, the added WARN_ON will trigger on older
kernels that have many more things implementing
invalidate_range_end(). It should not be backported to anything that
has more invalidate_range_ends()'s than today's kernel.
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-03-11 23:29 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-11 18:00 [PATCH v2] mm/mmu_notifiers: Esnure range_end() is paired with range_start() Sean Christopherson
2021-03-11 23:28 ` Jason Gunthorpe
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.