Messages in this thread Patch in this message | | | Date | Fri, 20 May 2022 16:58:27 +0200 | Subject | Re: [PATCH v2 6/8] KVM: Fix multiple races in gfn=>pfn cache refresh | From | Paolo Bonzini <> |
| |
On 5/20/22 16:49, Paolo Bonzini wrote: > On 4/27/22 03:40, Sean Christopherson wrote: >> + * Wait for mn_active_invalidate_count, not mmu_notifier_count, >> + * to go away, as the invalidation in the mmu_notifier event >> + * occurs_before_ mmu_notifier_count is elevated. >> + * >> + * Note, mn_active_invalidate_count can change at any time as >> + * it's not protected by gpc->lock. But, it is guaranteed to >> + * be elevated before the mmu_notifier acquires gpc->lock, and >> + * isn't dropped until after mmu_notifier_seq is updated. So, >> + * this task may get a false positive of sorts, i.e. see an >> + * elevated count and wait even though it's technically safe to >> + * proceed (becase the mmu_notifier will invalidate the cache >> + *_after_ it's refreshed here), but the cache will never be >> + * refreshed with stale data, i.e. won't get false negatives. > > I am all for lavish comments, but I think this is even too detailed. > What about:
And in fact this should be moved to a separate function.
diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c index 50ce7b78b42f..321964ff42e1 100644 --- a/virt/kvm/pfncache.c +++ b/virt/kvm/pfncache.c @@ -112,6 +112,36 @@ static void gpc_release_pfn_and_khva(struct kvm *kvm, kvm_pfn_t pfn, void *khva) } } + +static inline bool mmu_notifier_retry_cache(struct kvm *kvm, unsigned long mmu_seq) +{ + /* + * mn_active_invalidate_count acts for all intents and purposes + * like mmu_notifier_count here; but we cannot use the latter + * because the invalidation in the mmu_notifier event occurs + * _before_ mmu_notifier_count is elevated. + * + * Note, it does not matter that mn_active_invalidate_count + * is not protected by gpc->lock. It is guaranteed to + * be elevated before the mmu_notifier acquires gpc->lock, and + * isn't dropped until after mmu_notifier_seq is updated. + */ + if (kvm->mn_active_invalidate_count) + return true; + + /* + * Ensure mn_active_invalidate_count is read before + * mmu_notifier_seq. This pairs with the smp_wmb() in + * mmu_notifier_invalidate_range_end() to guarantee either the + * old (non-zero) value of mn_active_invalidate_count or the + * new (incremented) value of mmu_notifier_seq is observed. + */ + smp_rmb(); + if (kvm->mmu_notifier_seq != mmu_seq) + return true; + return false; +} + static kvm_pfn_t hva_to_pfn_retry(struct kvm *kvm, struct gfn_to_pfn_cache *gpc) { /* Note, the new page offset may be different than the old! */ @@ -129,7 +159,7 @@ static kvm_pfn_t hva_to_pfn_retry(struct kvm *kvm, struct gfn_to_pfn_cache *gpc) */ gpc->valid = false; - for (;;) { + do { mmu_seq = kvm->mmu_notifier_seq; smp_rmb(); @@ -188,32 +218,7 @@ static kvm_pfn_t hva_to_pfn_retry(struct kvm *kvm, struct gfn_to_pfn_cache *gpc) * attempting to refresh. */ WARN_ON_ONCE(gpc->valid); - - /* - * mn_active_invalidate_count acts for all intents and purposes - * like mmu_notifier_count here; but we cannot use the latter - * because the invalidation in the mmu_notifier event occurs - * _before_ mmu_notifier_count is elevated. - * - * Note, it does not matter that mn_active_invalidate_count - * is not protected by gpc->lock. It is guaranteed to - * be elevated before the mmu_notifier acquires gpc->lock, and - * isn't dropped until after mmu_notifier_seq is updated. - */ - if (kvm->mn_active_invalidate_count) - continue; - - /* - * Ensure mn_active_invalidate_count is read before - * mmu_notifier_seq. This pairs with the smp_wmb() in - * mmu_notifier_invalidate_range_end() to guarantee either the - * old (non-zero) value of mn_active_invalidate_count or the - * new (incremented) value of mmu_notifier_seq is observed. - */ - smp_rmb(); - if (kvm->mmu_notifier_seq == mmu_seq) - break; - } + } while (mmu_notifier_retry_cache(kvm, mmu_seq); gpc->valid = true; gpc->pfn = new_pfn;
| |