linux-kernel - Re: [PATCH v4] KVM: x86/tsc: Don't sync user changes to TSC with KVM-initiated change

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZQHSJ9Epx1oNTZGE@google.com>
Date:   Wed, 13 Sep 2023 15:15:51 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     David Woodhouse <dwmw2@...radead.org>
Cc:     Like Xu <like.xu.linux@...il.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Oliver Upton <oliver.upton@...ux.dev>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] KVM: x86/tsc: Don't sync user changes to TSC with
 KVM-initiated change

On Wed, Sep 13, 2023, David Woodhouse wrote:
> On Wed, 2023-09-13 at 07:50 -0700, Sean Christopherson wrote:
> > On Wed, Sep 13, 2023, David Woodhouse wrote:
> > > Userspace used to be able to force a sync by writing zero. You are
> > > removing that from the ABI without any explanation about why;
> > 
> > No, my suggestion did not remove that from the ABI.  A @user_value of '0' would
> > still force synchronization.
> 
> Ah, OK. Yes, you're right. Thanks.
> 
> > It's necessary for "user_set_tsc" to be an accurate name.  The code in v6 yields
> > "user_set_tsc_to_non_zero_value".  And I don't think it's just a naming issue,
> 
> In another thread, you said that the sync code doesn't differentiate
> between userspace initializing the TSC And userspace attempting to
> synchronize the TSC. I responded that *I* don't differentiate the two
> and couldn't see the difference.
> 
> I think we were both wrong. Userspace does *explicitly* synchronize the
> TSC by writing zero, and the sync code *does* explicitly handle that,
> yes?

Doh, by "sync code" I meant the "conditionally sync" code, i.e. the data != 0 path.

> And the reason I mention it here is that we could perhaps reasonable
> say that userspace *syncing* the TSC like that is not the same as
> userspace *setting* the TSC, and that it's OK for user_set_tsc to
> remain false? It saves adding another argument to kvm_synchronize_tsc()
> making it even more complex for a use case that just doesn't make sense
> anyway...
> 
> > e.g. if userspace writes '0' immediately after creating, and then later writes a
> > small delta, the v6 code wouldn't trigger synchronization because "user_set_tsc"
> > would be left unseft by the write of '0'.
> 
> True, but that's the existing behaviour,

No?  The existing code will fall into the "conditionally sync" logic for any
non-zero value.

		if (data == 0) {
			/*
			 * detection of vcpu initialization -- need to sync
			 * with other vCPUs. This particularly helps to keep
			 * kvm_clock stable after CPU hotplug
			 */
			synchronizing = true;
		} else {
			u64 tsc_exp = kvm->arch.last_tsc_write +
						nsec_to_cycles(vcpu, elapsed);
			u64 tsc_hz = vcpu->arch.virtual_tsc_khz * 1000LL;
			/*
			 * Special case: TSC write with a small delta (1 second)
			 * of virtual cycle time against real time is
			 * interpreted as an attempt to synchronize the CPU.
			 */
			synchronizing = data < tsc_exp + tsc_hz &&
					data + tsc_hz > tsc_exp;
		}

> and it doesn't make much sense for the user to write 0 to trigger a sync
> immediately after creating, because the *kernel* does that anyway.

I don't care (in the Tommy Lee Jones[*] sense).  All I care about is minimizing
the probability of breaking userspace, which means making the smallest possible
change to KVM's ABI.  For me, whether or not userspace is doing something sensible
doesn't factor into that equation.

[*] https://www.youtube.com/watch?v=OoTbXu1qnbc