linux-kernel - Re: [PATCH v2] KVM x86/xen: add an override for PVCLOCK_TSC_STABLE

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: <028f629d16377f9a7e9fd87ef9564846b0ab4ed9.camel@infradead.org>
Date:   Tue, 31 Oct 2023 22:48:41 +0000
From:   David Woodhouse <dwmw2@...radead.org>
To:     Sean Christopherson <seanjc@...gle.com>,
        Paul Durrant <paul@....org>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Jonathan Corbet <corbet@....net>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>, kvm@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] KVM x86/xen: add an override for
 PVCLOCK_TSC_STABLE_BIT

On Tue, 2023-10-31 at 15:39 -0700, Sean Christopherson wrote:
> On Tue, Oct 31, 2023, Paul Durrant wrote:
> > 
> > +       if (force_tsc_unstable)
> > +               guest_hv_clock->flags &= ~PVCLOCK_TSC_STABLE_BIT;
> 
> I don't see how this works.  This clears the bit in the guest copy, then clobbers
> all of guest_hv_clock with a memcpy().

Agreed, that seems wrong.

>         if (force_tsc_unstable)
>                 guest_hv_clock->flags &= ~PVCLOCK_TSC_STABLE_BIT;
> 
>         smp_wmb();
> 
>         /* retain PVCLOCK_GUEST_STOPPED if set in guest copy */
>         vcpu->hv_clock.flags |= (guest_hv_clock->flags & PVCLOCK_GUEST_STOPPED);
> 
>         if (vcpu->pvclock_set_guest_stopped_request) {
>                 vcpu->hv_clock.flags |= PVCLOCK_GUEST_STOPPED;
>                 vcpu->pvclock_set_guest_stopped_request = false;
>         }
> 
>         memcpy(guest_hv_clock, &vcpu->hv_clock, sizeof(*guest_hv_clock)); <= sets PVCLOCK_TSC_STABLE_BIT again, no?
>         smp_wmb();
> 
> Any reason not to make this a generic "capability" instead of a Xen specific flag?
> E.g. I assume these problematic guests would mishandle PVCLOCK_TSC_STABLE_BIT if
> it showed up in kvmclock, but they don't use kvmclock so it's not a problem in
> practice.

No, those guests are just fine with kvmclock. It's the *Xen* page they
forgot to map to userspace for the vDSO to use. And it's Xen (true Xen)
which made you jump through hoops to use the TSC that way, such that it
would actually expose the PVCLOCK_TSC_STABLE_BIT. We don't expect, and
have never seen, such issues with native KVM guests.

> I doubt there's a real need or use case, but it'd require less churn and IMO is
> simpler overall, e.g.
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e3eb608b6692..731b201bfd5a 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3225,7 +3225,7 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
>  
>         /* If the host uses TSC clocksource, then it is stable */
>         pvclock_flags = 0;
> -       if (use_master_clock)
> +       if (use_master_clock && !vcpu->kvm.force_tsc_unstable)
>                 pvclock_flags |= PVCLOCK_TSC_STABLE_BIT;
>  
>         vcpu->hv_clock.flags = pvclock_flags;
> 
> I also assume this is a "set and forget" thing?  I.e. KVM can require the flag
> to be set before any vCPUs are created.

Hrm, not sure we have previously required that the KVM_XEN_HVM_CONFIG
setup be done before any vCPUs were created. I tend to prefer *not* to
push ordering requirements onto userspace. Does it need to be a per-
vcpu thing? 


Download attachment "smime.p7s" of type "application/pkcs7-signature" (5965 bytes)