| Message ID | 20220729184640.244969-3-dietschc@csp.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86/xen: Prevent xen timer init when running | expand |
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 >
On Fri, Aug 05, 2022 at 06:50:15PM +0000, Sean Christopherson wrote: > 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. > Understood. > > 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. > Thanks for helping clarify this Sean. > > 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. */ > Agreed, I'll clean that comment up a bit. > > + 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 > >
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 */ + 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;
This moves the stop xen timer call outside of the previously unreachable if else statement as well as making sure that the timer is stopped first before changing IRQ vector. Code was streamlined a bit also. This was contributing to the ODEBUG bug in kvm_xen_vcpu_set_attr crash that was discovered by syzbot. 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 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(-)