From: Sean Christopherson <seanjc@google.com> To: Coleman Dietsch <dietschc@csp.edu> Cc: kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>, Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org, "H . Peter Anvin" <hpa@zytor.com>, linux-kernel@vger.kernel.org, skhan@linuxfoundation.org, Pavel Skripkin <paskripkin@gmail.com>, linux-kernel-mentees@lists.linuxfoundation.org, syzbot+e54f930ed78eb0f85281@syzkaller.appspotmail.com Subject: Re: [PATCH v2 2/2] KVM: x86/xen: Stop Xen timer before changing the IRQ vector Date: Fri, 5 Aug 2022 18:50:15 +0000 [thread overview] Message-ID: <Yu1mZyJeYf/0/LP+@google.com> (raw) In-Reply-To: <20220729184640.244969-3-dietschc@csp.edu> On Fri, Jul 29, 2022, Coleman Dietsch wrote: > This moves the stop xen timer call outside of the previously unreachable Please avoid "This", "This patch", etc... and describe what the change is, not what the patch is. > if else statement as well as making sure that the timer is stopped first > before changing IRQ vector. Code was streamlined a bit also. Generally speaking, don't describe the literal code changes, e.g. write the changelog as if you were describing the bug and the fix to someone in a verbal conversation. > This was contributing to the ODEBUG bug in kvm_xen_vcpu_set_attr crash that > was discovered by syzbot. That's not proper justification as it doesn't explain why this patch is needed even after fixing the immedate cause of the ODEBUG splat. Stop Xen's timer (if it's running) prior to changing the vector and potentially (re)starting the timer. Changing the vector while the timer is still running can result in KVM injecting a garbage event, e.g. vm_xen_inject_timer_irqs() could see a non-zero xen.timer_pending from a previous timer but inject the new xen.timer_virq. > ODEBUG: init active (active state 0) > object type: hrtimer hint: xen_timer_callbac0 > RIP: 0010:debug_print_object+0x16e/0x250 lib/debugobjects.c:502 > Call Trace: > __debug_object_init > debug_hrtimer_init > debug_init > hrtimer_init > kvm_xen_init_timer > kvm_xen_vcpu_set_attr > kvm_arch_vcpu_ioctl > kvm_vcpu_ioctl > vfs_ioctl > Fixes: 536395260582 ("KVM: x86/xen: handle PV timers oneshot mode") Cc: stable@vger.kernel.org > Link: https://syzkaller.appspot.com/bug?id=8234a9dfd3aafbf092cc5a7cd9842e3ebc45fc42 > Reported-by: syzbot+e54f930ed78eb0f85281@syzkaller.appspotmail.com > Signed-off-by: Coleman Dietsch <dietschc@csp.edu> > --- > arch/x86/kvm/xen.c | 37 ++++++++++++++++++------------------- > 1 file changed, 18 insertions(+), 19 deletions(-) > > diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c > index 2dd0f72a62f2..f612fac0e379 100644 > --- a/arch/x86/kvm/xen.c > +++ b/arch/x86/kvm/xen.c > @@ -707,27 +707,26 @@ int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data) > break; > > case KVM_XEN_VCPU_ATTR_TYPE_TIMER: > - if (data->u.timer.port) { > - if (data->u.timer.priority != KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL) { > - r = -EINVAL; > - break; > - } > - vcpu->arch.xen.timer_virq = data->u.timer.port; > - > - /* Check for existing timer */ > - if (!vcpu->arch.xen.timer.function) > - kvm_xen_init_timer(vcpu); > - > - /* Restart the timer if it's set */ > - if (data->u.timer.expires_ns) > - kvm_xen_start_timer(vcpu, data->u.timer.expires_ns, > - data->u.timer.expires_ns - > - get_kvmclock_ns(vcpu->kvm)); > - } else if (kvm_xen_timer_enabled(vcpu)) { > - kvm_xen_stop_timer(vcpu); > - vcpu->arch.xen.timer_virq = 0; > + if (data->u.timer.port && > + data->u.timer.priority != KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL) { > + r = -EINVAL; > + break; > } > > + /* Check for existing timer */ > + if (!vcpu->arch.xen.timer.function) > + kvm_xen_init_timer(vcpu); > + > + /* Stop the timer (if it's running) before changing the vector */ > + kvm_xen_stop_timer(vcpu); > + vcpu->arch.xen.timer_virq = data->u.timer.port; > + > + /* Restart the timer if it's set */ The "if it's set" part is stale, maybe this? /* Start the timer if the new value has a valid vector+expiry. */ > + if (data->u.timer.port && data->u.timer.expires_ns) > + kvm_xen_start_timer(vcpu, data->u.timer.expires_ns, > + data->u.timer.expires_ns - > + get_kvmclock_ns(vcpu->kvm)); > + > r = 0; > break; > > -- > 2.34.1 >
WARNING: multiple messages have this Message-ID (diff)
From: Sean Christopherson via Linux-kernel-mentees <linux-kernel-mentees@lists.linuxfoundation.org> To: Coleman Dietsch <dietschc@csp.edu> Cc: x86@kernel.org, kvm@vger.kernel.org, Pavel Skripkin <paskripkin@gmail.com>, Dave Hansen <dave.hansen@linux.intel.com>, linux-kernel@vger.kernel.org, syzbot+e54f930ed78eb0f85281@syzkaller.appspotmail.com, Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, "H . Peter Anvin" <hpa@zytor.com>, Paolo Bonzini <pbonzini@redhat.com>, Thomas Gleixner <tglx@linutronix.de>, linux-kernel-mentees@lists.linuxfoundation.org Subject: Re: [PATCH v2 2/2] KVM: x86/xen: Stop Xen timer before changing the IRQ vector Date: Fri, 5 Aug 2022 18:50:15 +0000 [thread overview] Message-ID: <Yu1mZyJeYf/0/LP+@google.com> (raw) In-Reply-To: <20220729184640.244969-3-dietschc@csp.edu> On Fri, Jul 29, 2022, Coleman Dietsch wrote: > This moves the stop xen timer call outside of the previously unreachable Please avoid "This", "This patch", etc... and describe what the change is, not what the patch is. > if else statement as well as making sure that the timer is stopped first > before changing IRQ vector. Code was streamlined a bit also. Generally speaking, don't describe the literal code changes, e.g. write the changelog as if you were describing the bug and the fix to someone in a verbal conversation. > This was contributing to the ODEBUG bug in kvm_xen_vcpu_set_attr crash that > was discovered by syzbot. That's not proper justification as it doesn't explain why this patch is needed even after fixing the immedate cause of the ODEBUG splat. Stop Xen's timer (if it's running) prior to changing the vector and potentially (re)starting the timer. Changing the vector while the timer is still running can result in KVM injecting a garbage event, e.g. vm_xen_inject_timer_irqs() could see a non-zero xen.timer_pending from a previous timer but inject the new xen.timer_virq. > ODEBUG: init active (active state 0) > object type: hrtimer hint: xen_timer_callbac0 > RIP: 0010:debug_print_object+0x16e/0x250 lib/debugobjects.c:502 > Call Trace: > __debug_object_init > debug_hrtimer_init > debug_init > hrtimer_init > kvm_xen_init_timer > kvm_xen_vcpu_set_attr > kvm_arch_vcpu_ioctl > kvm_vcpu_ioctl > vfs_ioctl > Fixes: 536395260582 ("KVM: x86/xen: handle PV timers oneshot mode") Cc: stable@vger.kernel.org > Link: https://syzkaller.appspot.com/bug?id=8234a9dfd3aafbf092cc5a7cd9842e3ebc45fc42 > Reported-by: syzbot+e54f930ed78eb0f85281@syzkaller.appspotmail.com > Signed-off-by: Coleman Dietsch <dietschc@csp.edu> > --- > arch/x86/kvm/xen.c | 37 ++++++++++++++++++------------------- > 1 file changed, 18 insertions(+), 19 deletions(-) > > diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c > index 2dd0f72a62f2..f612fac0e379 100644 > --- a/arch/x86/kvm/xen.c > +++ b/arch/x86/kvm/xen.c > @@ -707,27 +707,26 @@ int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data) > break; > > case KVM_XEN_VCPU_ATTR_TYPE_TIMER: > - if (data->u.timer.port) { > - if (data->u.timer.priority != KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL) { > - r = -EINVAL; > - break; > - } > - vcpu->arch.xen.timer_virq = data->u.timer.port; > - > - /* Check for existing timer */ > - if (!vcpu->arch.xen.timer.function) > - kvm_xen_init_timer(vcpu); > - > - /* Restart the timer if it's set */ > - if (data->u.timer.expires_ns) > - kvm_xen_start_timer(vcpu, data->u.timer.expires_ns, > - data->u.timer.expires_ns - > - get_kvmclock_ns(vcpu->kvm)); > - } else if (kvm_xen_timer_enabled(vcpu)) { > - kvm_xen_stop_timer(vcpu); > - vcpu->arch.xen.timer_virq = 0; > + if (data->u.timer.port && > + data->u.timer.priority != KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL) { > + r = -EINVAL; > + break; > } > > + /* Check for existing timer */ > + if (!vcpu->arch.xen.timer.function) > + kvm_xen_init_timer(vcpu); > + > + /* Stop the timer (if it's running) before changing the vector */ > + kvm_xen_stop_timer(vcpu); > + vcpu->arch.xen.timer_virq = data->u.timer.port; > + > + /* Restart the timer if it's set */ The "if it's set" part is stale, maybe this? /* Start the timer if the new value has a valid vector+expiry. */ > + if (data->u.timer.port && data->u.timer.expires_ns) > + kvm_xen_start_timer(vcpu, data->u.timer.expires_ns, > + data->u.timer.expires_ns - > + get_kvmclock_ns(vcpu->kvm)); > + > r = 0; > break; > > -- > 2.34.1 > _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
next prev parent reply other threads:[~2022-08-05 18:50 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-07-29 18:46 [PATCH v2 0/2] KVM: x86/xen: Prevent xen timer init when running Coleman Dietsch 2022-07-29 18:46 ` Coleman Dietsch 2022-07-29 18:46 ` [PATCH v2 1/2] KVM: x86/xen: Initialize Xen timer only once Coleman Dietsch 2022-07-29 18:46 ` Coleman Dietsch 2022-08-05 18:37 ` Sean Christopherson 2022-08-05 18:37 ` Sean Christopherson via Linux-kernel-mentees 2022-08-08 16:50 ` Coleman Dietsch 2022-08-08 16:50 ` Coleman Dietsch 2022-07-29 18:46 ` [PATCH v2 2/2] KVM: x86/xen: Stop Xen timer before changing the IRQ vector Coleman Dietsch 2022-07-29 18:46 ` Coleman Dietsch 2022-08-05 18:50 ` Sean Christopherson [this message] 2022-08-05 18:50 ` Sean Christopherson via Linux-kernel-mentees 2022-08-08 17:00 ` Coleman Dietsch 2022-08-08 17:00 ` Coleman Dietsch
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=Yu1mZyJeYf/0/LP+@google.com \ --to=seanjc@google.com \ --cc=bp@alien8.de \ --cc=dave.hansen@linux.intel.com \ --cc=dietschc@csp.edu \ --cc=hpa@zytor.com \ --cc=kvm@vger.kernel.org \ --cc=linux-kernel-mentees@lists.linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mingo@redhat.com \ --cc=paskripkin@gmail.com \ --cc=pbonzini@redhat.com \ --cc=skhan@linuxfoundation.org \ --cc=syzbot+e54f930ed78eb0f85281@syzkaller.appspotmail.com \ --cc=tglx@linutronix.de \ --cc=x86@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.