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
| ||
|
Date: Fri, 3 Jul 2015 13:12:11 +0200 From: Paolo Bonzini <pbonzini@...hat.com> To: Peter Zijlstra <peterz@...radead.org>, Pontus Fuchs <pontus.fuchs@...il.com>, Linus Torvalds <torvalds@...ux-foundation.org> Cc: mingo@...hat.com, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, gleb@...nel.org Subject: Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage On 25/06/2015 14:55, Peter Zijlstra wrote: > Subject: sched,kvm: Fix KVM preempt_notifier usage > > The preempt-notifier API needs to sleep with the addition of the > static_key, we do however need to hold off preemption while modifying > the preempt notifier list, otherwise a preemption could observe an > inconsistent list state. > > There is no reason to have preemption disabled in the caller, > registering a preempt notifier is a purely task local affair. > > Therefore move the preempt_disable into the functions and change the > callers to a preemptible context. > > Cc: Gleb Natapov <gleb@...nel.org> Uhm, you forgot at least me and the KVM mailing list. And you didn't even Cc Gleb on the original patch. So nice of you, and it makes me wonder if you even grepped for preempt_notifier_register when you wrote the original patch. Probably not, since you couldn't even be bothered to test the *only* user of preempt notifiers. In fact you shouldn't have just tested the patch on a case _without_ preemption notifiers, you should have also benchmarked the impact that static keys have _with_ preemption notifiers. In a not-really-artificial case (one single-processor guest running on the host), the static key patch adds a static_key_slow_inc on a relatively hot path for KVM, which is not acceptable. So, NACK. > Fixes: 1cde2930e154 ("sched/preempt: Add static_key() to preempt_notifiers") Linus, can you please revert the above patch instead? Paolo > Reported-and-Tested-by: Pontus Fuchs <pontus.fuchs@...il.com> > Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org> > --- > kernel/sched/core.c | 11 +++++++++++ > virt/kvm/kvm_main.c | 5 +++-- > 2 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index b803e1b8ab0c..6169c167ac98 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -2327,7 +2327,12 @@ static struct static_key preempt_notifier_key = STATIC_KEY_INIT_FALSE; > void preempt_notifier_register(struct preempt_notifier *notifier) > { > static_key_slow_inc(&preempt_notifier_key); > + /* > + * Avoid preemption while changing the preempt notifier list. > + */ > + preempt_disable(); > hlist_add_head(¬ifier->link, ¤t->preempt_notifiers); > + preempt_enable(); > } > EXPORT_SYMBOL_GPL(preempt_notifier_register); > > @@ -2339,7 +2344,13 @@ EXPORT_SYMBOL_GPL(preempt_notifier_register); > */ > void preempt_notifier_unregister(struct preempt_notifier *notifier) > { > + /* > + * Avoid preemption while changing the preempt notifier list. > + */ > + preempt_disable(); > hlist_del(¬ifier->link); > + preempt_enable(); > + > static_key_slow_dec(&preempt_notifier_key); > } > EXPORT_SYMBOL_GPL(preempt_notifier_unregister); > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 90977418aeb6..d7aafa0458a0 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -128,8 +128,9 @@ int vcpu_load(struct kvm_vcpu *vcpu) > > if (mutex_lock_killable(&vcpu->mutex)) > return -EINTR; > - cpu = get_cpu(); > preempt_notifier_register(&vcpu->preempt_notifier); > + > + cpu = get_cpu(); > kvm_arch_vcpu_load(vcpu, cpu); > put_cpu(); > return 0; > @@ -139,8 +140,8 @@ void vcpu_put(struct kvm_vcpu *vcpu) > { > preempt_disable(); > kvm_arch_vcpu_put(vcpu); > - preempt_notifier_unregister(&vcpu->preempt_notifier); > preempt_enable(); > + preempt_notifier_unregister(&vcpu->preempt_notifier); > mutex_unlock(&vcpu->mutex); > } > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists