Received-SPF: pass (google.com: domain of linux-kernel+bounces-67455-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240215010004.1456078-1-seanjc@google.com> <20240215010004.1456078-2-seanjc@google.com>
In-Reply-To: <20240215010004.1456078-2-seanjc@google.com>
From: David Matlack <dmatlack@google.com>
Date: Thu, 15 Feb 2024 09:57:29 -0800
Message-ID: <CALzav=c0MFB7UG7yaXB3bAFampYO_xN=5Pjao6La55wy4cwjSw@mail.gmail.com>
Subject: Re: [PATCH 1/2] KVM: x86: Mark target gfn of emulated atomic
 instruction as dirty
To: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Pasha Tatashin <tatashin@google.com>, Michael Krebs <mkrebs@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Feb 14, 2024 at 5:00=E2=80=AFPM Sean Christopherson <seanjc@google.=
com> wrote:
>
> When emulating an atomic access on behalf of the guest, mark the target
> gfn dirty if the CMPXCHG by KVM is attempted and doesn't fault.  This
> fixes a bug where KVM effectively corrupts guest memory during live
> migration by writing to guest memory without informing userspace that the
> page is dirty.
>
> Marking the page dirty got unintentionally dropped when KVM's emulated
> CMPXCHG was converted to do a user access.  Before that, KVM explicitly
> mapped the guest page into kernel memory, and marked the page dirty durin=
g
> the unmap phase.
>
> Mark the page dirty even if the CMPXCHG fails, as the old data is written
> back on failure, i.e. the page is still written.  The value written is
> guaranteed to be the same because the operation is atomic, but KVM's ABI
> is that all writes are dirty logged regardless of the value written.  And
> more importantly, that's what KVM did before the buggy commit.
>
> Huge kudos to the folks on the Cc list (and many others), who did all the
> actual work of triaging and debugging.
>
> Fixes: 1c2361f667f3 ("KVM: x86: Use __try_cmpxchg_user() to emulate atomi=
c accesses")

I'm only half serious but... Should we just revert this commit?

This commit claims that kvm_vcpu_map() is unsafe because it can race
with mremap(). But there are many other places where KVM uses
kvm_vcpu_map() (e.g. nested VMX). It seems like KVM is just not
compatible with mremap() until we address all the users of
kvm_vcpu_map(). Patching _just_ emulator_cmpxchg_emulated() seems
silly but maybe I'm missing some context on what led to commit
1c2361f667f3 being written.

kvm_vcpu_map/unmap() might not be the best interface, but it serves as
a common choke-point for mapping guest memory to access in KVM. This
is helpful for avoiding missed dirty logging updates (obviously) and
will be even more helpful if we add support for freezing guest memory
and "KVM Userfault" (as discussed in the 1/3 PUCK). I think we all
agree we should do more of this (common choke points), not less. If
there's a usecase for mremap()ing guest memory, we should make
kvm_vcpu_map() play nice with mmu_notifiers.