On Tue, Apr 13, 2021 at 05:36:41PM -0400, Peter Xu wrote: > This fixes a bug that can trigger with e.g. "taskset -c 0 ./dirty_log_test" or > when the testing host is very busy. > > The issue is when the vcpu thread got the dirty bit set but got preempted by > other threads _before_ the data is written, we won't be able to see the latest > data only until the vcpu threads do VMENTER. IOW, the guest write operation and > dirty bit set cannot guarantee atomicity. The race could look like: > > main thread vcpu thread > =========== =========== > iteration=X > *addr = X > (so latest data is X) > iteration=X+1 > ... > iteration=X+N > guest executes "*addr = X+N" > reg=READ_ONCE(iteration)=X+N > host page fault > set dirty bit for page "addr" > (_before_ VMENTER happens... > so *addr is still X!) > vcpu thread got preempted > get dirty log > verify data > detected dirty bit set, data is X > not X+N nor X+N-1, data too old! > > This patch closes this race by allowing the main thread to give the vcpu thread > chance to do a VMENTER to complete that write operation. It's done by adding a > vcpu loop counter (must be defined as volatile as main thread will do read > loop), then the main thread can guarantee the vcpu got at least another VMENTER > by making sure the guest_vcpu_loops increases by 2. > > Dirty ring does not need this since dirty_ring_last_page would already help > avoid this specific race condition. > > Cc: Andrew Jones <drjones@xxxxxxxxxx> > Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx> > Cc: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> > Cc: Sean Christopherson <seanjc@xxxxxxxxxx> > Signed-off-by: Peter Xu <peterx@xxxxxxxxxx> > --- > v2: > - drop one unnecessary check on "!matched" > --- > tools/testing/selftests/kvm/dirty_log_test.c | 53 +++++++++++++++++++- > 1 file changed, 52 insertions(+), 1 deletion(-) > Reviewed-by: Andrew Jones <drjones@xxxxxxxxxx>
