* [PATCH 0/2] VM: Fix a benign race in kicking vCPUs @ 2021-08-21 0:04 Sean Christopherson 2021-08-21 0:05 ` [PATCH 1/2] KVM: Clean up benign vcpu->cpu data races when " Sean Christopherson 2021-08-21 0:05 ` [PATCH 2/2] KVM: Guard cpusmask NULL check with CONFIG_CPUMASK_OFFSTACK Sean Christopherson 0 siblings, 2 replies; 8+ messages in thread From: Sean Christopherson @ 2021-08-21 0:04 UTC (permalink / raw) To: Paolo Bonzini Cc: kvm, linux-kernel, Venkatesh Srinivas, Vitaly Kuznetsov, Sean Christopherson Fix benign races when kicking vCPUs where the task doing the kicking can consume a stale vcpu->cpu. The races are benign because of the impliciations of task migration with respect to interrupts and being in guest mode, but IMO they're worth fixing if only as an excuse to document the flows. Patch 2 is a tangentially related cleanup to prevent future me from trying to get rid of the NULL check on the cpumask parameters, which _looks_ like it can't ever be NULL, but has a subtle edge case due to the way CONFIG_CPUMASK_OFFSTACK=y handles cpumasks. Sean Christopherson (2): KVM: Clean up benign vcpu->cpu data races when kicking vCPUs KVM: Guard cpusmask NULL check with CONFIG_CPUMASK_OFFSTACK virt/kvm/kvm_main.c | 46 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 9 deletions(-) -- 2.33.0.rc2.250.ged5fa647cd-goog ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] KVM: Clean up benign vcpu->cpu data races when kicking vCPUs 2021-08-21 0:04 [PATCH 0/2] VM: Fix a benign race in kicking vCPUs Sean Christopherson @ 2021-08-21 0:05 ` Sean Christopherson 2021-08-23 7:49 ` Vitaly Kuznetsov 2021-08-21 0:05 ` [PATCH 2/2] KVM: Guard cpusmask NULL check with CONFIG_CPUMASK_OFFSTACK Sean Christopherson 1 sibling, 1 reply; 8+ messages in thread From: Sean Christopherson @ 2021-08-21 0:05 UTC (permalink / raw) To: Paolo Bonzini Cc: kvm, linux-kernel, Venkatesh Srinivas, Vitaly Kuznetsov, Sean Christopherson Fix a benign data race reported by syzbot+KCSAN[*] by ensuring vcpu->cpu is read exactly once, and by ensuring the vCPU is booted from guest mode if kvm_arch_vcpu_should_kick() returns true. Fix a similar race in kvm_make_vcpus_request_mask() by ensuring the vCPU is interrupted if kvm_request_needs_ipi() returns true. Reading vcpu->cpu before vcpu->mode (via kvm_arch_vcpu_should_kick() or kvm_request_needs_ipi()) means the target vCPU could get migrated (change vcpu->cpu) and enter !OUTSIDE_GUEST_MODE between reading vcpu->cpud and reading vcpu->mode. If that happens, the kick/IPI will be sent to the old pCPU, not the new pCPU that is now running the vCPU or reading SPTEs. Although failing to kick the vCPU is not exactly ideal, practically speaking it cannot cause a functional issue unless there is also a bug in the caller, and any such bug would exist regardless of kvm_vcpu_kick()'s behavior. The purpose of sending an IPI is purely to get a vCPU into the host (or out of reading SPTEs) so that the vCPU can recognize a change in state, e.g. a KVM_REQ_* request. If vCPU's handling of the state change is required for correctness, KVM must ensure either the vCPU sees the change before entering the guest, or that the sender sees the vCPU as running in guest mode. All architectures handle this by (a) sending the request before calling kvm_vcpu_kick() and (b) checking for requests _after_ setting vcpu->mode. x86's READING_SHADOW_PAGE_TABLES has similar requirements; KVM needs to ensure it kicks and waits for vCPUs that started reading SPTEs _before_ MMU changes were finalized, but any vCPU that starts reading after MMU changes were finalized will see the new state and can continue on uninterrupted. For uses of kvm_vcpu_kick() that are not paired with a KVM_REQ_*, e.g. x86's kvm_arch_sync_dirty_log(), the order of the kick must not be relied upon for functional correctness, e.g. in the dirty log case, userspace cannot assume it has a 100% complete log if vCPUs are still running. All that said, eliminate the benign race since the cost of doing so is an "extra" atomic cmpxchg() in the case where the target vCPU is loaded by the current pCPU or is not loaded at all. I.e. the kick will be skipped due to kvm_vcpu_exiting_guest_mode() seeing a compatible vcpu->mode as opposed to the kick being skipped because of the cpu checks. Keep the "cpu != me" checks even though they appear useless/impossible at first glance. x86 processes guest IPI writes in a fast path that runs in IN_GUEST_MODE, i.e. can call kvm_vcpu_kick() from IN_GUEST_MODE. And calling kvm_vm_bugged()->kvm_make_vcpus_request_mask() from IN_GUEST or READING_SHADOW_PAGE_TABLES is perfectly reasonable. Note, a race with the cpu_online() check in kvm_vcpu_kick() likely persists, e.g. the vCPU could exit guest mode and get offlined between the cpu_online() check and the sending of smp_send_reschedule(). But, the online check appears to exist only to avoid a WARN in x86's native_smp_send_reschedule() that fires if the target CPU is not online. The reschedule WARN exists because CPU offlining takes the CPU out of the scheduling pool, i.e. the WARN is intended to detect the case where the kernel attempts to schedule a task on an offline CPU. The actual sending of the IPI is a non-issue as at worst it will simpy be dropped on the floor. In other words, KVM's usurping of the reschedule IPI could theoretically trigger a WARN if the stars align, but there will be no loss of functionality. [*] https://syzkaller.appspot.com/bug?extid=cd4154e502f43f10808a Cc: Venkatesh Srinivas <venkateshs@google.com> Cc: Vitaly Kuznetsov <vkuznets@redhat.com> Fixes: 97222cc83163 ("KVM: Emulate local APIC in kernel") Signed-off-by: Sean Christopherson <seanjc@google.com> --- virt/kvm/kvm_main.c | 36 ++++++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 3e67c93ca403..786b914db98f 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -273,14 +273,26 @@ bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned int req, continue; kvm_make_request(req, vcpu); - cpu = vcpu->cpu; if (!(req & KVM_REQUEST_NO_WAKEUP) && kvm_vcpu_wake_up(vcpu)) continue; - if (tmp != NULL && cpu != -1 && cpu != me && - kvm_request_needs_ipi(vcpu, req)) - __cpumask_set_cpu(cpu, tmp); + /* + * Note, the vCPU could get migrated to a different pCPU at any + * point after kvm_request_needs_ipi(), which could result in + * sending an IPI to the previous pCPU. But, that's ok because + * the purpose of the IPI is to ensure the vCPU returns to + * OUTSIDE_GUEST_MODE, which is satisfied if the vCPU migrates. + * Entering READING_SHADOW_PAGE_TABLES after this point is also + * ok, as the requirement is only that KVM wait for vCPUs that + * were reading SPTEs _before_ any changes were finalized. See + * kvm_vcpu_kick() for more details on handling requests. + */ + if (tmp != NULL && kvm_request_needs_ipi(vcpu, req)) { + cpu = READ_ONCE(vcpu->cpu); + if (cpu != -1 && cpu != me) + __cpumask_set_cpu(cpu, tmp); + } } called = kvm_kick_many_cpus(tmp, !!(req & KVM_REQUEST_WAIT)); @@ -3309,16 +3321,24 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_wake_up); */ void kvm_vcpu_kick(struct kvm_vcpu *vcpu) { - int me; - int cpu = vcpu->cpu; + int me, cpu; if (kvm_vcpu_wake_up(vcpu)) return; + /* + * Note, the vCPU could get migrated to a different pCPU at any point + * after kvm_arch_vcpu_should_kick(), which could result in sending an + * IPI to the previous pCPU. But, that's ok because the purpose of the + * IPI is to force the vCPU to leave IN_GUEST_MODE, and migrating the + * vCPU also requires it to leave IN_GUEST_MODE. + */ me = get_cpu(); - if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu)) - if (kvm_arch_vcpu_should_kick(vcpu)) + if (kvm_arch_vcpu_should_kick(vcpu)) { + cpu = READ_ONCE(vcpu->cpu); + if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu)) smp_send_reschedule(cpu); + } put_cpu(); } EXPORT_SYMBOL_GPL(kvm_vcpu_kick); -- 2.33.0.rc2.250.ged5fa647cd-goog ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] KVM: Clean up benign vcpu->cpu data races when kicking vCPUs 2021-08-21 0:05 ` [PATCH 1/2] KVM: Clean up benign vcpu->cpu data races when " Sean Christopherson @ 2021-08-23 7:49 ` Vitaly Kuznetsov 0 siblings, 0 replies; 8+ messages in thread From: Vitaly Kuznetsov @ 2021-08-23 7:49 UTC (permalink / raw) To: Sean Christopherson, Paolo Bonzini Cc: kvm, linux-kernel, Venkatesh Srinivas, Sean Christopherson Sean Christopherson <seanjc@google.com> writes: > Fix a benign data race reported by syzbot+KCSAN[*] by ensuring vcpu->cpu > is read exactly once, and by ensuring the vCPU is booted from guest mode > if kvm_arch_vcpu_should_kick() returns true. Fix a similar race in > kvm_make_vcpus_request_mask() by ensuring the vCPU is interrupted if > kvm_request_needs_ipi() returns true. > > Reading vcpu->cpu before vcpu->mode (via kvm_arch_vcpu_should_kick() or > kvm_request_needs_ipi()) means the target vCPU could get migrated (change > vcpu->cpu) and enter !OUTSIDE_GUEST_MODE between reading vcpu->cpud and > reading vcpu->mode. If that happens, the kick/IPI will be sent to the > old pCPU, not the new pCPU that is now running the vCPU or reading SPTEs. > > Although failing to kick the vCPU is not exactly ideal, practically > speaking it cannot cause a functional issue unless there is also a bug in > the caller, and any such bug would exist regardless of kvm_vcpu_kick()'s > behavior. > > The purpose of sending an IPI is purely to get a vCPU into the host (or > out of reading SPTEs) so that the vCPU can recognize a change in state, > e.g. a KVM_REQ_* request. If vCPU's handling of the state change is > required for correctness, KVM must ensure either the vCPU sees the change > before entering the guest, or that the sender sees the vCPU as running in > guest mode. All architectures handle this by (a) sending the request > before calling kvm_vcpu_kick() and (b) checking for requests _after_ > setting vcpu->mode. > > x86's READING_SHADOW_PAGE_TABLES has similar requirements; KVM needs to > ensure it kicks and waits for vCPUs that started reading SPTEs _before_ > MMU changes were finalized, but any vCPU that starts reading after MMU > changes were finalized will see the new state and can continue on > uninterrupted. > > For uses of kvm_vcpu_kick() that are not paired with a KVM_REQ_*, e.g. > x86's kvm_arch_sync_dirty_log(), the order of the kick must not be relied > upon for functional correctness, e.g. in the dirty log case, userspace > cannot assume it has a 100% complete log if vCPUs are still running. > > All that said, eliminate the benign race since the cost of doing so is an > "extra" atomic cmpxchg() in the case where the target vCPU is loaded by > the current pCPU or is not loaded at all. I.e. the kick will be skipped > due to kvm_vcpu_exiting_guest_mode() seeing a compatible vcpu->mode as > opposed to the kick being skipped because of the cpu checks. > > Keep the "cpu != me" checks even though they appear useless/impossible at > first glance. x86 processes guest IPI writes in a fast path that runs in > IN_GUEST_MODE, i.e. can call kvm_vcpu_kick() from IN_GUEST_MODE. And > calling kvm_vm_bugged()->kvm_make_vcpus_request_mask() from IN_GUEST or > READING_SHADOW_PAGE_TABLES is perfectly reasonable. > > Note, a race with the cpu_online() check in kvm_vcpu_kick() likely > persists, e.g. the vCPU could exit guest mode and get offlined between > the cpu_online() check and the sending of smp_send_reschedule(). But, > the online check appears to exist only to avoid a WARN in x86's > native_smp_send_reschedule() that fires if the target CPU is not online. > The reschedule WARN exists because CPU offlining takes the CPU out of the > scheduling pool, i.e. the WARN is intended to detect the case where the > kernel attempts to schedule a task on an offline CPU. The actual sending > of the IPI is a non-issue as at worst it will simpy be dropped on the > floor. In other words, KVM's usurping of the reschedule IPI could > theoretically trigger a WARN if the stars align, but there will be no > loss of functionality. > > [*] https://syzkaller.appspot.com/bug?extid=cd4154e502f43f10808a > > Cc: Venkatesh Srinivas <venkateshs@google.com> > Cc: Vitaly Kuznetsov <vkuznets@redhat.com> > Fixes: 97222cc83163 ("KVM: Emulate local APIC in kernel") > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > virt/kvm/kvm_main.c | 36 ++++++++++++++++++++++++++++-------- > 1 file changed, 28 insertions(+), 8 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 3e67c93ca403..786b914db98f 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -273,14 +273,26 @@ bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned int req, > continue; > > kvm_make_request(req, vcpu); > - cpu = vcpu->cpu; > > if (!(req & KVM_REQUEST_NO_WAKEUP) && kvm_vcpu_wake_up(vcpu)) > continue; > > - if (tmp != NULL && cpu != -1 && cpu != me && > - kvm_request_needs_ipi(vcpu, req)) > - __cpumask_set_cpu(cpu, tmp); > + /* > + * Note, the vCPU could get migrated to a different pCPU at any > + * point after kvm_request_needs_ipi(), which could result in > + * sending an IPI to the previous pCPU. But, that's ok because "OK" (unless there's risk someone will think of Oklahoma and take it as an offense) :-) > + * the purpose of the IPI is to ensure the vCPU returns to > + * OUTSIDE_GUEST_MODE, which is satisfied if the vCPU migrates. > + * Entering READING_SHADOW_PAGE_TABLES after this point is also > + * ok, as the requirement is only that KVM wait for vCPUs that "OK" > + * were reading SPTEs _before_ any changes were finalized. See > + * kvm_vcpu_kick() for more details on handling requests. > + */ > + if (tmp != NULL && kvm_request_needs_ipi(vcpu, req)) { > + cpu = READ_ONCE(vcpu->cpu); > + if (cpu != -1 && cpu != me) > + __cpumask_set_cpu(cpu, tmp); > + } > } > > called = kvm_kick_many_cpus(tmp, !!(req & KVM_REQUEST_WAIT)); > @@ -3309,16 +3321,24 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_wake_up); > */ > void kvm_vcpu_kick(struct kvm_vcpu *vcpu) > { > - int me; > - int cpu = vcpu->cpu; > + int me, cpu; > > if (kvm_vcpu_wake_up(vcpu)) > return; > > + /* > + * Note, the vCPU could get migrated to a different pCPU at any point > + * after kvm_arch_vcpu_should_kick(), which could result in sending an > + * IPI to the previous pCPU. But, that's ok because the purpose of the "OK" > + * IPI is to force the vCPU to leave IN_GUEST_MODE, and migrating the > + * vCPU also requires it to leave IN_GUEST_MODE. > + */ > me = get_cpu(); > - if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu)) > - if (kvm_arch_vcpu_should_kick(vcpu)) > + if (kvm_arch_vcpu_should_kick(vcpu)) { > + cpu = READ_ONCE(vcpu->cpu); > + if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu)) It seems it was Marcelo who wrote "(unsigned)cpu < nr_cpu_ids" back in 2009 but isn't it the same as "cpu != -1" or are there any other possible negative values? I don't think vcpu->cpu can go above nr_cpu_ids somehow but maybe it can? > smp_send_reschedule(cpu); > + } > put_cpu(); > } > EXPORT_SYMBOL_GPL(kvm_vcpu_kick); Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> -- Vitaly ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] KVM: Guard cpusmask NULL check with CONFIG_CPUMASK_OFFSTACK 2021-08-21 0:04 [PATCH 0/2] VM: Fix a benign race in kicking vCPUs Sean Christopherson 2021-08-21 0:05 ` [PATCH 1/2] KVM: Clean up benign vcpu->cpu data races when " Sean Christopherson @ 2021-08-21 0:05 ` Sean Christopherson 2021-08-23 7:54 ` Vitaly Kuznetsov 2021-08-25 4:05 ` Lai Jiangshan 1 sibling, 2 replies; 8+ messages in thread From: Sean Christopherson @ 2021-08-21 0:05 UTC (permalink / raw) To: Paolo Bonzini Cc: kvm, linux-kernel, Venkatesh Srinivas, Vitaly Kuznetsov, Sean Christopherson Check for a NULL cpumask_var_t when kicking multiple vCPUs if and only if cpumasks are configured to be allocated off-stack. This is a meaningless optimization, e.g. avoids a TEST+Jcc and TEST+CMOV on x86, but more importantly helps document that the NULL check is necessary even though all callers pass in a local variable. No functional change intended. Signed-off-by: Sean Christopherson <seanjc@google.com> --- virt/kvm/kvm_main.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 786b914db98f..82c5280dd5ce 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -247,7 +247,7 @@ static void ack_flush(void *_completed) static inline bool kvm_kick_many_cpus(const struct cpumask *cpus, bool wait) { - if (unlikely(!cpus)) + if (IS_ENABLED(CONFIG_CPUMASK_OFFSTACK) && unlikely(!cpus)) cpus = cpu_online_mask; if (cpumask_empty(cpus)) @@ -277,6 +277,14 @@ bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned int req, if (!(req & KVM_REQUEST_NO_WAKEUP) && kvm_vcpu_wake_up(vcpu)) continue; + /* + * tmp can be NULL if cpumasks are allocated off stack, as + * allocation of the mask is deliberately not fatal and is + * handled by falling back to kicking all online CPUs. + */ + if (IS_ENABLED(CONFIG_CPUMASK_OFFSTACK) && !tmp) + continue; + /* * Note, the vCPU could get migrated to a different pCPU at any * point after kvm_request_needs_ipi(), which could result in @@ -288,7 +296,7 @@ bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned int req, * were reading SPTEs _before_ any changes were finalized. See * kvm_vcpu_kick() for more details on handling requests. */ - if (tmp != NULL && kvm_request_needs_ipi(vcpu, req)) { + if (kvm_request_needs_ipi(vcpu, req)) { cpu = READ_ONCE(vcpu->cpu); if (cpu != -1 && cpu != me) __cpumask_set_cpu(cpu, tmp); -- 2.33.0.rc2.250.ged5fa647cd-goog ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] KVM: Guard cpusmask NULL check with CONFIG_CPUMASK_OFFSTACK 2021-08-21 0:05 ` [PATCH 2/2] KVM: Guard cpusmask NULL check with CONFIG_CPUMASK_OFFSTACK Sean Christopherson @ 2021-08-23 7:54 ` Vitaly Kuznetsov 2021-08-25 4:05 ` Lai Jiangshan 1 sibling, 0 replies; 8+ messages in thread From: Vitaly Kuznetsov @ 2021-08-23 7:54 UTC (permalink / raw) To: Sean Christopherson, Paolo Bonzini Cc: kvm, linux-kernel, Venkatesh Srinivas, Sean Christopherson Sean Christopherson <seanjc@google.com> writes: > Check for a NULL cpumask_var_t when kicking multiple vCPUs if and only if > cpumasks are configured to be allocated off-stack. This is a meaningless > optimization, e.g. avoids a TEST+Jcc and TEST+CMOV on x86, but more > importantly helps document that the NULL check is necessary even though > all callers pass in a local variable. > > No functional change intended. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > virt/kvm/kvm_main.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 786b914db98f..82c5280dd5ce 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -247,7 +247,7 @@ static void ack_flush(void *_completed) > > static inline bool kvm_kick_many_cpus(const struct cpumask *cpus, bool wait) > { > - if (unlikely(!cpus)) > + if (IS_ENABLED(CONFIG_CPUMASK_OFFSTACK) && unlikely(!cpus)) > cpus = cpu_online_mask; > > if (cpumask_empty(cpus)) > @@ -277,6 +277,14 @@ bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned int req, > if (!(req & KVM_REQUEST_NO_WAKEUP) && kvm_vcpu_wake_up(vcpu)) > continue; > > + /* > + * tmp can be NULL if cpumasks are allocated off stack, as > + * allocation of the mask is deliberately not fatal and is > + * handled by falling back to kicking all online CPUs. > + */ > + if (IS_ENABLED(CONFIG_CPUMASK_OFFSTACK) && !tmp) > + continue; > + > /* > * Note, the vCPU could get migrated to a different pCPU at any > * point after kvm_request_needs_ipi(), which could result in > @@ -288,7 +296,7 @@ bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned int req, > * were reading SPTEs _before_ any changes were finalized. See > * kvm_vcpu_kick() for more details on handling requests. > */ > - if (tmp != NULL && kvm_request_needs_ipi(vcpu, req)) { > + if (kvm_request_needs_ipi(vcpu, req)) { > cpu = READ_ONCE(vcpu->cpu); > if (cpu != -1 && cpu != me) > __cpumask_set_cpu(cpu, tmp); In case MM people don't like us poking into CONFIG_CPUMASK_OFFSTACK details we can probably get away with a comment. Otherwise Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> -- Vitaly ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] KVM: Guard cpusmask NULL check with CONFIG_CPUMASK_OFFSTACK 2021-08-21 0:05 ` [PATCH 2/2] KVM: Guard cpusmask NULL check with CONFIG_CPUMASK_OFFSTACK Sean Christopherson 2021-08-23 7:54 ` Vitaly Kuznetsov @ 2021-08-25 4:05 ` Lai Jiangshan 2021-08-25 21:57 ` Sean Christopherson 1 sibling, 1 reply; 8+ messages in thread From: Lai Jiangshan @ 2021-08-25 4:05 UTC (permalink / raw) To: Sean Christopherson Cc: Paolo Bonzini, kvm, LKML, Venkatesh Srinivas, Vitaly Kuznetsov On Sat, Aug 21, 2021 at 8:09 AM Sean Christopherson <seanjc@google.com> wrote: > > Check for a NULL cpumask_var_t when kicking multiple vCPUs if and only if > cpumasks are configured to be allocated off-stack. This is a meaningless > optimization, e.g. avoids a TEST+Jcc and TEST+CMOV on x86, but more > importantly helps document that the NULL check is necessary even though > all callers pass in a local variable. > > No functional change intended. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > virt/kvm/kvm_main.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 786b914db98f..82c5280dd5ce 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -247,7 +247,7 @@ static void ack_flush(void *_completed) > > static inline bool kvm_kick_many_cpus(const struct cpumask *cpus, bool wait) > { > - if (unlikely(!cpus)) > + if (IS_ENABLED(CONFIG_CPUMASK_OFFSTACK) && unlikely(!cpus)) > cpus = cpu_online_mask; > > if (cpumask_empty(cpus)) > @@ -277,6 +277,14 @@ bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned int req, > if (!(req & KVM_REQUEST_NO_WAKEUP) && kvm_vcpu_wake_up(vcpu)) > continue; > > + /* > + * tmp can be NULL if cpumasks are allocated off stack, as > + * allocation of the mask is deliberately not fatal and is > + * handled by falling back to kicking all online CPUs. > + */ > + if (IS_ENABLED(CONFIG_CPUMASK_OFFSTACK) && !tmp) > + continue; > + Hello, Sean I don't think it is a good idea to reinvent the cpumask_available(). You can rework the patch as the following code if cpumask_available() fits for you. Thanks Lai diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 3e67c93ca403..ca043ec7ed74 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -245,9 +245,11 @@ static void ack_flush(void *_completed) { } -static inline bool kvm_kick_many_cpus(const struct cpumask *cpus, bool wait) +static inline bool kvm_kick_many_cpus(cpumask_var_t tmp, bool wait) { - if (unlikely(!cpus)) + const struct cpumask *cpus = tmp; + + if (unlikely(!cpumask_available(tmp))) cpus = cpu_online_mask; if (cpumask_empty(cpus)) @@ -278,7 +280,7 @@ bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned int req, if (!(req & KVM_REQUEST_NO_WAKEUP) && kvm_vcpu_wake_up(vcpu)) continue; - if (tmp != NULL && cpu != -1 && cpu != me && + if (cpumask_available(tmp) && cpu != -1 && cpu != me && kvm_request_needs_ipi(vcpu, req)) __cpumask_set_cpu(cpu, tmp); } > /* > * Note, the vCPU could get migrated to a different pCPU at any > * point after kvm_request_needs_ipi(), which could result in > @@ -288,7 +296,7 @@ bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned int req, > * were reading SPTEs _before_ any changes were finalized. See > * kvm_vcpu_kick() for more details on handling requests. > */ > - if (tmp != NULL && kvm_request_needs_ipi(vcpu, req)) { > + if (kvm_request_needs_ipi(vcpu, req)) { > cpu = READ_ONCE(vcpu->cpu); > if (cpu != -1 && cpu != me) > __cpumask_set_cpu(cpu, tmp); > -- > 2.33.0.rc2.250.ged5fa647cd-goog > ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] KVM: Guard cpusmask NULL check with CONFIG_CPUMASK_OFFSTACK 2021-08-25 4:05 ` Lai Jiangshan @ 2021-08-25 21:57 ` Sean Christopherson 2021-08-26 10:24 ` Vitaly Kuznetsov 0 siblings, 1 reply; 8+ messages in thread From: Sean Christopherson @ 2021-08-25 21:57 UTC (permalink / raw) To: Lai Jiangshan Cc: Paolo Bonzini, kvm, LKML, Venkatesh Srinivas, Vitaly Kuznetsov On Wed, Aug 25, 2021, Lai Jiangshan wrote: > On Sat, Aug 21, 2021 at 8:09 AM Sean Christopherson <seanjc@google.com> wrote: > > @@ -277,6 +277,14 @@ bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned int req, > > if (!(req & KVM_REQUEST_NO_WAKEUP) && kvm_vcpu_wake_up(vcpu)) > > continue; > > > > + /* > > + * tmp can be NULL if cpumasks are allocated off stack, as > > + * allocation of the mask is deliberately not fatal and is > > + * handled by falling back to kicking all online CPUs. > > + */ > > + if (IS_ENABLED(CONFIG_CPUMASK_OFFSTACK) && !tmp) > > + continue; > > + > > Hello, Sean > > I don't think it is a good idea to reinvent the cpumask_available(). Using cpumask_available() is waaaay better, thanks! Vitaly / Paolo, take this one instead? From deff3e168c0612a2947d1ef29e488282631a788c Mon Sep 17 00:00:00 2001 From: Sean Christopherson <seanjc@google.com> Date: Fri, 20 Aug 2021 13:36:21 -0700 Subject: [PATCH] KVM: Use cpumask_available() to check for NULL cpumask when kicking vCPUs Check for a NULL cpumask_var_t when kicking multiple vCPUs via cpumask_available(), which performs a !NULL check if and only if cpumasks are configured to be allocated off-stack. This is a meaningless optimization, e.g. avoids a TEST+Jcc and TEST+CMOV on x86, but more importantly helps document that the NULL check is necessary even though all callers pass in a local variable. No functional change intended. Cc: Lai Jiangshan <jiangshanlai@gmail.com> Signed-off-by: Sean Christopherson <seanjc@google.com> --- virt/kvm/kvm_main.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 786b914db98f..2082aceffbf6 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -245,9 +245,13 @@ static void ack_flush(void *_completed) { } -static inline bool kvm_kick_many_cpus(const struct cpumask *cpus, bool wait) +static inline bool kvm_kick_many_cpus(cpumask_var_t tmp, bool wait) { - if (unlikely(!cpus)) + const struct cpumask *cpus; + + if (likely(cpumask_available(tmp))) + cpus = tmp; + else cpus = cpu_online_mask; if (cpumask_empty(cpus)) @@ -277,6 +281,14 @@ bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned int req, if (!(req & KVM_REQUEST_NO_WAKEUP) && kvm_vcpu_wake_up(vcpu)) continue; + /* + * tmp can be "unavailable" if cpumasks are allocated off stack + * as allocation of the mask is deliberately not fatal and is + * handled by falling back to kicking all online CPUs. + */ + if (!cpumask_available(tmp)) + continue; + /* * Note, the vCPU could get migrated to a different pCPU at any * point after kvm_request_needs_ipi(), which could result in @@ -288,7 +300,7 @@ bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned int req, * were reading SPTEs _before_ any changes were finalized. See * kvm_vcpu_kick() for more details on handling requests. */ - if (tmp != NULL && kvm_request_needs_ipi(vcpu, req)) { + if (kvm_request_needs_ipi(vcpu, req)) { cpu = READ_ONCE(vcpu->cpu); if (cpu != -1 && cpu != me) __cpumask_set_cpu(cpu, tmp); -- ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] KVM: Guard cpusmask NULL check with CONFIG_CPUMASK_OFFSTACK 2021-08-25 21:57 ` Sean Christopherson @ 2021-08-26 10:24 ` Vitaly Kuznetsov 0 siblings, 0 replies; 8+ messages in thread From: Vitaly Kuznetsov @ 2021-08-26 10:24 UTC (permalink / raw) To: Sean Christopherson Cc: Paolo Bonzini, kvm, LKML, Venkatesh Srinivas, Lai Jiangshan Sean Christopherson <seanjc@google.com> writes: > On Wed, Aug 25, 2021, Lai Jiangshan wrote: >> On Sat, Aug 21, 2021 at 8:09 AM Sean Christopherson <seanjc@google.com> wrote: >> > @@ -277,6 +277,14 @@ bool kvm_make_vcpus_request_mask(struct kvm *kvm, unsigned int req, >> > if (!(req & KVM_REQUEST_NO_WAKEUP) && kvm_vcpu_wake_up(vcpu)) >> > continue; >> > >> > + /* >> > + * tmp can be NULL if cpumasks are allocated off stack, as >> > + * allocation of the mask is deliberately not fatal and is >> > + * handled by falling back to kicking all online CPUs. >> > + */ >> > + if (IS_ENABLED(CONFIG_CPUMASK_OFFSTACK) && !tmp) >> > + continue; >> > + >> >> Hello, Sean >> >> I don't think it is a good idea to reinvent the cpumask_available(). > > Using cpumask_available() is waaaay better, thanks! > > Vitaly / Paolo, take this one instead? > Sure, putting this to my v3, thanks! -- Vitaly ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-08-26 10:24 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-21 0:04 [PATCH 0/2] VM: Fix a benign race in kicking vCPUs Sean Christopherson 2021-08-21 0:05 ` [PATCH 1/2] KVM: Clean up benign vcpu->cpu data races when " Sean Christopherson 2021-08-23 7:49 ` Vitaly Kuznetsov 2021-08-21 0:05 ` [PATCH 2/2] KVM: Guard cpusmask NULL check with CONFIG_CPUMASK_OFFSTACK Sean Christopherson 2021-08-23 7:54 ` Vitaly Kuznetsov 2021-08-25 4:05 ` Lai Jiangshan 2021-08-25 21:57 ` Sean Christopherson 2021-08-26 10:24 ` Vitaly Kuznetsov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).