On 15/05/17 18:43, Christoffer Dall wrote:
On Mon, May 15, 2017 at 02:36:58PM +0100, Suzuki K Poulose wrote:
On 15/05/17 11:00, Christoffer Dall wrote:
Hi Suzuki,
So I don't think this change is wrong, but I wonder if it's sufficient.
For example, I can see that this function is called from
stage2_unmsp_vm
-> stage2_unmap_memslot
-> unmap_stage2_range
and
kvm_arch_flush_shadow_memslot
-> unmap_stage2_range
which never check if the pgd pointer is valid,
You are right. Those two callers do not check it. We could fix all of this by simply
moving the check to the beginning of the loop.
i.e, something like this :
@@ -295,6 +295,12 @@ static void unmap_stage2_range(struct kvm *kvm, phys_addr_t start, u64 size)
assert_spin_locked(&kvm->mmu_lock);
pgd = kvm->arch.pgd + stage2_pgd_index(addr);
do {
+ /*
+ * Make sure the page table is still active, as we could
+ * another thread could have possibly freed the page table.
+ */
+ if (!READ_ONCE(kvm->arch.pgd))
+ break;
next = stage2_pgd_addr_end(addr, end);
if (!stage2_pgd_none(*pgd))
unmap_stage2_puds(kvm, pgd, addr, next);
and finally, kvm_free_stage2_pgd also checks the pgd pointer outside of holding the
kvm->mmu_lock so why is this not racy?
This has been fixed by patch 1 in the series. So should be fine.
I can respin the patch with the changes if you are OK with it.
Yes, absolutely. I've already applied patch 1 so no need to include
that in your respin.
I have made a minor change to the 1st patch, to make use of READ_ONCE/WRITE_ONCE,
to make sure we don't use the cached value of the kvm->arch.pgd. Something like :
@@ -829,22 +829,22 @@ void stage2_unmap_vm(struct kvm *kvm)
* Walks the level-1 page table pointed to by kvm->arch.pgd and frees all
* underlying level-2 and level-3 tables before freeing the actual level-1 table
* and setting the struct pointer to NULL.
- *
- * Note we don't need locking here as this is only called when the VM is
- * destroyed, which can only be done once.
*/
void kvm_free_stage2_pgd(struct kvm *kvm)
{
- if (kvm->arch.pgd == NULL)
- return;
+ void *pgd = NULL;
spin_lock(&kvm->mmu_lock);
- unmap_stage2_range(kvm, 0, KVM_PHYS_SIZE);
+ if (kvm->arch.pgd) {
+ unmap_stage2_range(kvm, 0, KVM_PHYS_SIZE);
+ pgd = READ_ONCE(kvm->arch.pgd);
+ WRITE_ONCE(kvm->arch.pgd, NULL);
+ }
spin_unlock(&kvm->mmu_lock);
Let me know if you could fix it up or I could send a fresh series. Sorry about that.
Suzuki
