linux-kernel - RE: Q. about KVM and CPU hotplug

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] [day] [month] [year] [list]

Message-ID: <BN9PR11MB543302C3841AED15AF52B2A28C6B9@BN9PR11MB5433.namprd11.prod.outlook.com>
Date:   Sat, 4 Dec 2021 03:57:06 +0000
From:   "Tian, Kevin" <kevin.tian@...el.com>
To:     Thomas Gleixner <tglx@...utronix.de>,
        Paolo Bonzini <pbonzini@...hat.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "Yamahata, Isaku" <isaku.yamahata@...el.com>,
        "Huang, Kai" <kai.huang@...el.com>,
        "Nakajima, Jun" <jun.nakajima@...el.com>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        "Gao, Chao" <chao.gao@...el.com>
Subject: RE: Q. about KVM and CPU hotplug

> From: Thomas Gleixner <tglx@...utronix.de>
> Sent: Wednesday, December 1, 2021 6:31 PM
> 
> On Wed, Dec 01 2021 at 06:59, Kevin Tian wrote:
> >> From: Paolo Bonzini <paolo.bonzini@...il.com>
> >> It should fail the first vmptrld instruction.  It will result in a few
> >> WARN_ONCE and pr_warn_ratelimited (see vmx_insn_failed).  For VMX
> this
> >> should be a pretty bad firmware bug, and it has never been reported.
> >> KVM did find some undocumented errata but not this one!
> >>
> >
> > or it may be caused by incompatible CPU capabilities, which is currently
> > missing a check in kvm_starting_cpu(). So far the compatibility check is
> > done only once before registering cpu hotplug state machine:
> >
> >         for_each_online_cpu(cpu) {
> >                 smp_call_function_single(cpu, check_processor_compat, &c, 1);
> >                 if (r < 0)
> >                         goto out_free_2;
> >         }
> >
> >         r = cpuhp_setup_state_nocalls(CPUHP_AP_KVM_STARTING,
> "kvm/cpu:starting",
> >                                       kvm_starting_cpu, kvm_dying_cpu);
> 
> Duh. This is silly _and_ broken.
> 
> Using for_each_inline_cpu() without holding cpus_read_lock() is racy
> against concurrent hotplug. But even if the locking is added then
> nothing prevents a CPU from being plugged _after_ the lock is dropped.
> 
> The right solution is to move the hotplug state into the threaded
> section as I pointed out and do:
> 
>          r = cpuhp_setup_state(CPUHP_AP_KVM_STARTING, "kvm/cpu:starting",
>                                kvm_starting_cpu, kvm_dying_cpu);
> 
> which will do the right thing automatically. Checking for compatibility
> would just be part of the kvm_starting_cpu() callback.
> 

Yes, this sounds the right thing to do. We'll work on a fix.

And as said in another reply to Paolo, future TDX compatibility check
will also be added to this callback.

Thanks
Kevin