Re: [PATCH v1 1/5] KVM: arm64: Enable ring-based dirty memory tracking

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 8/24/22 00:47, Marc Zyngier wrote:
I definitely don't think I 100% understand all the ordering things since
they're complicated.. but my understanding is that the reset procedure
didn't need memory barrier (unlike pushing, where we have explicit wmb),
because we assumed the userapp is not hostile so logically it should only
modify the flags which is a 32bit field, assuming atomicity guaranteed.
Atomicity doesn't guarantee ordering, unfortunately. Take the
following example: CPU0 is changing a bunch of flags for GFNs A, B, C,
D that exist in the ring in that order, and CPU1 performs an ioctl to
reset the page state.

CPU0:
     write_flag(A, KVM_DIRTY_GFN_F_RESET)
     write_flag(B, KVM_DIRTY_GFN_F_RESET)
     write_flag(C, KVM_DIRTY_GFN_F_RESET)
     write_flag(D, KVM_DIRTY_GFN_F_RESET)
     [...]

CPU1:
    ioctl(KVM_RESET_DIRTY_RINGS)

Since CPU0 writes do not have any ordering, CPU1 can observe the
writes in a sequence that have nothing to do with program order, and
could for example observe that GFN A and D have been reset, but not B
and C. This in turn breaks the logic in the reset code (B, C, and D
don't get reset), despite userspace having followed the spec to the
letter. If each was a store-release (which is the case on x86), it
wouldn't be a problem, but nothing calls it in the documentation.

Maybe that's not a big deal if it is expected that each CPU will issue
a KVM_RESET_DIRTY_RINGS itself, ensuring that it observe its own
writes. But expecting this to work across CPUs without any barrier is
wishful thinking.

Agreed, but that's a problem for userspace to solve. If userspace wants to reset the fields in different CPUs, it has to synchronize with its own invoking of the ioctl.

That is, CPU0 must ensure that a ioctl(KVM_RESET_DIRTY_RINGS) is done after (in the memory-ordering sense) its last write_flag(D, KVM_DIRTY_GFN_F_RESET). If there's no such ordering, there's no guarantee that the write_flag will have any effect.

The main reason why I preferred a global KVM_RESET_DIRTY_RINGS ioctl was because it takes kvm->slots_lock so the execution would be serialized anyway. Turning slots_lock into an rwsem would be even worse because it also takes kvm->mmu_lock (since slots_lock is a mutex, at least two concurrent invocations won't clash with each other on the mmu_lock).

Paolo




[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux