Re: [PATCH v5 11/13] KVM: Keep memslots in tree-based structures instead of array-based ones

From: Sean Christopherson <seanjc@google.com>
To: "Maciej S. Szmigiero" <mail@maciej.szmigiero.name>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>,
	Igor Mammedov <imammedo@redhat.com>,
	Marc Zyngier <maz@kernel.org>, James Morse <james.morse@arm.com>,
	Julien Thierry <julien.thierry.kdev@gmail.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Huacai Chen <chenhuacai@kernel.org>,
	Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>,
	Paul Mackerras <paulus@ozlabs.org>,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	Janosch Frank <frankja@linux.ibm.com>,
	David Hildenbrand <david@redhat.com>,
	Cornelia Huck <cohuck@redhat.com>,
	Claudio Imbrenda <imbrenda@linux.ibm.com>,
	Joerg Roedel <joro@8bytes.org>,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 11/13] KVM: Keep memslots in tree-based structures instead of array-based ones
Date: Wed, 27 Oct 2021 00:36:03 +0000	[thread overview]
Message-ID: <YXie8z7w4AiFx4bP@google.com> (raw)
In-Reply-To: <0c35c60b524dd264cc6abb6a48bc253958f99673.1632171479.git.maciej.szmigiero@oracle.com>

On Mon, Sep 20, 2021, Maciej S. Szmigiero wrote:
> @@ -1607,68 +1506,145 @@ static int kvm_set_memslot(struct kvm *kvm,
> +	if (change != KVM_MR_CREATE) {
>  		/*
> -		 * The arch-specific fields of the memslots could have changed
> -		 * between releasing the slots_arch_lock in
> -		 * install_new_memslots and here, so get a fresh copy of these
> -		 * fields.
> +		 * The arch-specific fields of the memslot could have changed
> +		 * between reading them and taking slots_arch_lock in one of two
> +		 * places above.
> +		 * That includes old and new which were read in __kvm_set_memory_region.
>  		 */
> -		kvm_copy_memslots_arch(slots, __kvm_memslots(kvm, as_id));
> +		old->arch = new->arch = slotina->arch = slotact->arch;

Fudge.  This subtly and silently fixes an existing bug where @old and @new can
have stale arch specific data due to x86's godawful slots_arch_lock behavior.
If a flags-only update collides with alloc_all_memslots_rmaps(), @old and @new
may have stale (NULL) data if the rmaps activation happens after the old slot is
snapshotted.

It can be fixed by doing exactly this, but that is so, so gross (not your fault
at all, I'm complaining about the existing mess).

I think we can opportunistically prep for this series to make the end result
(mostly this patch), a bit cleaner while fixing that snafu.  Specifically, I
think I see a path to avoiding bikeshedding slotina, slotact, etc...

I'll get a series for the fix posted tomorrow, and hopefully reply with my thoughts
for this patch too.