linux-kernel - Re: [PATCH v2 6/8] KVM: Fix multiple races in gfn=>pfn cache refresh

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 20 May 2022 15:02:02 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, David Woodhouse <dwmw@...zon.co.uk>,
        Mingwei Zhang <mizhang@...gle.com>,
        Maxim Levitsky <mlevitsk@...hat.com>
Subject: Re: [PATCH v2 6/8] KVM: Fix multiple races in gfn=>pfn cache refresh

On Fri, May 20, 2022, Paolo Bonzini wrote:
> 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;

This can be

	return kvm->mmu_notifier_seq != mmu_seq;

Looks good otherwise.  It'll probably yield a smaller diff too.