Re: kvm splat in mmu_spte_clear_track_bits

[Linus Torvalds]

On Tue, Aug 29, 2017 at 12:13 PM, Jerome Glisse <jglisse@xxxxxxxxxx> wrote:
>
> Yes and i am fine with page traversal being under spinlock and not
> being able to sleep during that. I agree doing otherwise would be
> insane. It is just that the existing behavior of try_to_unmap_one()
> and page_mkclean_one() have been broken and that no mmu_notifier
> calls were added around the lock section.

Yeah, I'm actually surprised that ever worked. I'm surprised that
try_to_unmap_one didn't hold any locks earlier.

In fact, I think at least some of them *did* already hold the page
table locks: ptep_clear_flush_young_notify() and friends very much
should have always held them.

So it's literally just that mmu_notifier_invalidate_page() call that
used to be outside all the locks, but honestly, I think that was
always a bug. It means that you got notified of the page removal
*after* the page was already gone and all locks had been released, so
a completely *different* page could already have been mapped to that
address.

So I think the old code was always broken exactly because the callback
wasn't serialized with the actual action.

> I sent a patch that properly compute the range to invalidate and move
> to invalidate_range() but is lacking the invalidate_range_start()/
> end() so i am gonna respin that with range_start/end bracketing and
> assume the worse for the range of address.

So surrounding it with start/end _should_ make KVM happy.

KVM people, can you confirm?

But I do note that there's a number of other users of that
"invalidate_page" callback.

I think ib_umem_notifier_invalidate_page() the exact same blocking
issue, but changing to range_start/end should be good there too.

amdgpu_mn_invalidate_page() and the xen/gntdev also seem to be happy
being replaced with start/end.

In fact, I'm wondering if this actually means that we could get rid of
mmu_notifier_invalidate_page() entirely. There's only a couple of
callers, and the other one seems to be fs/dax.c, and it actually seems
to have the exact same issue that the try_to_unmap_one() code had: it
tried to invalidate an address too late - by the time it was called,
the page gad already been cleaned and locks had been released.

So the more I look at that "turn mmu_notifier_invalidate_page() into
invalidate_range_start/end()" the more I think that's fundamentally
the right thing to do.

                 Linus