Re: [PATCH v2] kvm: x86: fix stale mmio cache bug

[Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>]

On 08/06/2014 06:39 AM, David Matlack wrote:
> On Mon, Aug 4, 2014 at 8:36 PM, Xiao Guangrong
> <xiaoguangrong@linux.vnet.ibm.com> wrote:
>> On 08/05/2014 05:10 AM, David Matlack wrote:
>>>
>>> This patch fixes the issue by doing the following:
>>>   - Tag the mmio cache with the memslot generation and use it to
>>>     validate mmio cache lookups.
>>>   - Extend vcpu_clear_mmio_info to clear mmio_gfn in addition to
>>>     mmio_gva, since both can be used to fast path mmio faults.
>>>   - In mmu_sync_roots, unconditionally clear the mmio cache since
>>>     even direct_map (e.g. tdp) hosts use it.
>>
>> It's not needed.
>>
>> direct map only uses gpa (and never cache gva) and
>> vcpu_clear_mmio_info only clears gva.
> 
> Ok thanks for the clarification.
> 
>>> +static inline void vcpu_cache_mmio_info(struct kvm_vcpu *vcpu,
>>> +                                     gva_t gva, gfn_t gfn, unsigned access)
>>> +{
>>> +     vcpu->arch.mmio_gen = kvm_current_mmio_generation(vcpu->kvm);
>>> +
>>> +     /*
>>> +      * Ensure that the mmio_gen is set before the rest of the cache entry.
>>> +      * Otherwise we might see a new generation number attached to an old
>>> +      * cache entry if creating/deleting a memslot races with mmio caching.
>>> +      * The inverse case is possible (old generation number with new cache
>>> +      * info), but that is safe. The next access will just miss the cache
>>> +      * when it should have hit.
>>> +      */
>>> +     smp_wmb();
>>
>> The memory barrier can't help us, consider this scenario:
>>
>> CPU 0                                      CPU 1
>> page-fault
>> see gpa is not mapped in memslot
>>
>>                               create new memslot containing gpa from Qemu
>>                                   update the slots's generation number
>> cache mmio info
>>
>> !!! later when vcpu accesses gpa again
>> it will cause mmio-exit.
> 
> Ah! Thanks for catching my mistake.
> 
>> The easy way to fix this is that we update slots's generation-number
>> after synchronize_srcu_expedited when memslot is being updated that
>> ensures other sides can see the new generation-number only after
>> finishing update.
> 
> It would be possible for a vcpu to see an inconsistent kvm_memslots struct
> (new set of slots with an old generation number). Is that not an issue?

In this case, checking generation-number will fail, we will goto the slow path
to handle mmio access - that's very rare, so i think it's ok.

> 
> We could just use the generation number that is stored in the
> spte. The only downside (that I can see) is that handle_abnormal_pfn()
> calls vcpu_cache_mmio_info() but does not have access to the spte.
> So presumably we'd have to do a page table walk.

The issue is not only in vcpu_cache_mmio_info but also in
mark_mmio_spte() where we may cache new generation-number and old memslots
info.