linux-kernel - RE: [PATCH 5/6] KVM: x86: Remove WARN_ON in kvm_arch_check_processor

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 for Android: free password hash cracker in your pocket

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

Message-ID: <BN9PR11MB5276DEA925C72AF585E7472C8C519@BN9PR11MB5276.namprd11.prod.outlook.com>
Date:   Tue, 11 Jan 2022 02:15:11 +0000
From:   "Tian, Kevin" <kevin.tian@...el.com>
To:     "Christopherson,, Sean" <seanjc@...gle.com>,
        "Gao, Chao" <chao.gao@...el.com>
CC:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "Vitaly Kuznetsov" <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        "Jim Mattson" <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, Ingo Molnar <mingo@...hat.com>,
        Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 5/6] KVM: x86: Remove WARN_ON in
 kvm_arch_check_processor_compat

> From: Sean Christopherson <seanjc@...gle.com>
> Sent: Tuesday, January 11, 2022 7:00 AM
> 
> On Mon, Dec 27, 2021, Chao Gao wrote:
> > kvm_arch_check_processor_compat() needn't be called with interrupt
> > disabled, as it only reads some CRs/MSRs which won't be clobbered
> > by interrupt handlers or softirq.
> >
> > What really needed is disabling preemption. No additional check is
> > added because if CONFIG_DEBUG_PREEMPT is enabled, smp_processor_id()
> > (right above the WARN_ON()) can help to detect any violation.
> 
> Hrm, IIRC, the assertion that IRQs are disabled was more about detecting
> improper
> usage with respect to KVM doing hardware enabling than it was about
> ensuring the
> current task isn't migrated.  E.g. as exhibited by patch 06, extra protections
> (disabling of hotplug in that case) are needed if this helper is called outside
> of the core KVM hardware enabling flow since hardware_enable_all() does
> its thing
> via SMP function call.

Looks the WARN_ON() was added by you. 😊

commit f1cdecf5807b1a91829a2dc4f254bfe6bafd4776
Author: Sean Christopherson <sean.j.christopherson@...el.com>
Date:   Tue Dec 10 14:44:14 2019 -0800

    KVM: x86: Ensure all logical CPUs have consistent reserved cr4 bits

    Check the current CPU's reserved cr4 bits against the mask calculated
    for the boot CPU to ensure consistent behavior across all CPUs.

    Signed-off-by: Sean Christopherson <sean.j.christopherson@...el.com>
    Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>

But it's unclear to me how this WARN_ON() is related to what the commit
msg tries to explain. When I read this code it's more like a sanity check on
the assumption that it is currently called in SMP function call which runs 
the said function with interrupt disabled.

> 
> Is there CPU onlining state/metadata that we could use to handle that
> specific case?
> It'd be nice to preserve the paranoid check, but it's not a big deal if we can't.
> 
> If we can't preserve the WARN, can you rework the changelog to explain the
> motivation
> for removing the WARN?
> 
> Thanks!
> 
> > Signed-off-by: Chao Gao <chao.gao@...el.com>
> > ---
> >  arch/x86/kvm/x86.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index aa09c8792134..a80e3b0c11a8 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -11384,8 +11384,6 @@ int kvm_arch_check_processor_compat(void)
> >  {
> >  	struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
> >
> > -	WARN_ON(!irqs_disabled());
> > -
> >  	if (__cr4_reserved_bits(cpu_has, c) !=
> >  	    __cr4_reserved_bits(cpu_has, &boot_cpu_data))
> >  		return -EIO;
> > --
> > 2.25.1
> >