Add a check for existing xen timers before initializing a new one.
Currently kvm_xen_init_timer() is called on every KVM_XEN_VCPU_ATTR_TYPE_TIMER, which is causing the following ODEBUG crash when vcpu->arch.xen.timer is already set.
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=8234a9dfd3aafbf092cc5a7cd9842e3ebc45fc4... Reported-by: syzbot+e54f930ed78eb0f85281@syzkaller.appspotmail.com Signed-off-by: Coleman Dietsch dietschc@csp.edu --- arch/x86/kvm/xen.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c index a0c05ccbf4b1..6e554041e862 100644 --- a/arch/x86/kvm/xen.c +++ b/arch/x86/kvm/xen.c @@ -713,7 +713,9 @@ int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data) break; } vcpu->arch.xen.timer_virq = data->u.timer.port; - kvm_xen_init_timer(vcpu); + + if (!vcpu->arch.xen.timer.function) + kvm_xen_init_timer(vcpu);
/* Restart the timer if it's set */ if (data->u.timer.expires_ns)
Stop Xen timer (if it's running) prior to changing the IRQ vector and potentially (re)starting the timer. Changing the IRQ 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.
Fixes: 536395260582 ("KVM: x86/xen: handle PV timers oneshot mode") Cc: stable@vger.kernel.org Link: https://syzkaller.appspot.com/bug?id=8234a9dfd3aafbf092cc5a7cd9842e3ebc45fc4... Reported-by: syzbot+e54f930ed78eb0f85281@syzkaller.appspotmail.com Signed-off-by: Coleman Dietsch dietschc@csp.edu --- arch/x86/kvm/xen.c | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-)
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c index 6e554041e862..280cb5dc7341 100644 --- a/arch/x86/kvm/xen.c +++ b/arch/x86/kvm/xen.c @@ -707,26 +707,25 @@ 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; - - 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; }
+ 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; + + /* 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;
On Mon, Aug 08, 2022, Coleman Dietsch wrote:
Stop Xen timer (if it's running) prior to changing the IRQ vector and potentially (re)starting the timer. Changing the IRQ 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.
Fixes: 536395260582 ("KVM: x86/xen: handle PV timers oneshot mode") Cc: stable@vger.kernel.org Link: https://syzkaller.appspot.com/bug?id=8234a9dfd3aafbf092cc5a7cd9842e3ebc45fc4... Reported-by: syzbot+e54f930ed78eb0f85281@syzkaller.appspotmail.com Signed-off-by: Coleman Dietsch dietschc@csp.edu
Reviewed-by: Sean Christopherson seanjc@google.com
On Mon, 2022-08-08 at 14:06 -0500, Coleman Dietsch wrote:
Stop Xen timer (if it's running) prior to changing the IRQ vector and potentially (re)starting the timer. Changing the IRQ 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.
Hm, wasn't that already addressed in the first patch I saw, which just called kvm_xen_stop_timer() unconditionally before (possibly) setting it up again?
On 8/9/22 11:22, David Woodhouse wrote:
On Mon, 2022-08-08 at 14:06 -0500, Coleman Dietsch wrote:
Stop Xen timer (if it's running) prior to changing the IRQ vector and potentially (re)starting the timer. Changing the IRQ 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.
Hm, wasn't that already addressed in the first patch I saw, which just called kvm_xen_stop_timer() unconditionally before (possibly) setting it up again?
Which patch is that?
Paolo
On Tue, 2022-08-09 at 14:59 +0200, Paolo Bonzini wrote:
On 8/9/22 11:22, David Woodhouse wrote:
On Mon, 2022-08-08 at 14:06 -0500, Coleman Dietsch wrote:
Stop Xen timer (if it's running) prior to changing the IRQ vector and potentially (re)starting the timer. Changing the IRQ 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.
Hm, wasn't that already addressed in the first patch I saw, which just called kvm_xen_stop_timer() unconditionally before (possibly) setting it up again?
Which patch is that?
The one I acked in https://lore.kernel.org/all/9bad724858b6a06c25ead865b2b3d9dfc216d01c.camel@i...
On Tue, Aug 09, 2022, David Woodhouse wrote:
On Tue, 2022-08-09 at 14:59 +0200, Paolo Bonzini wrote:
On 8/9/22 11:22, David Woodhouse wrote:
On Mon, 2022-08-08 at 14:06 -0500, Coleman Dietsch wrote:
Stop Xen timer (if it's running) prior to changing the IRQ vector and potentially (re)starting the timer. Changing the IRQ 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.
Hm, wasn't that already addressed in the first patch I saw, which just called kvm_xen_stop_timer() unconditionally before (possibly) setting it up again?
Which patch is that?
The one I acked in https://lore.kernel.org/all/9bad724858b6a06c25ead865b2b3d9dfc216d01c.camel@i...
It's effectively the same patch. I had asked Coleman to split it into two separate patches: (1) fix the re-initialization of an active timer bug and (2) stop the active timer before changing the vector (this patch).
On Tue, 2022-08-09 at 14:07 +0000, Sean Christopherson wrote:
On Tue, Aug 09, 2022, David Woodhouse wrote:
On Tue, 2022-08-09 at 14:59 +0200, Paolo Bonzini wrote:
On 8/9/22 11:22, David Woodhouse wrote:
On Mon, 2022-08-08 at 14:06 -0500, Coleman Dietsch wrote:
Stop Xen timer (if it's running) prior to changing the IRQ vector and potentially (re)starting the timer. Changing the IRQ 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.
Hm, wasn't that already addressed in the first patch I saw, which just called kvm_xen_stop_timer() unconditionally before (possibly) setting it up again?
Which patch is that?
The one I acked in https://lore.kernel.org/all/9bad724858b6a06c25ead865b2b3d9dfc216d01c.camel@i...
It's effectively the same patch. I had asked Coleman to split it into two separate patches: (1) fix the re-initialization of an active timer bug and (2) stop the active timer before changing the vector (this patch).
But both bugs just require that the timer is stopped first. I preferred the original which was less intrusive, which did just that:
case KVM_XEN_VCPU_ATTR_TYPE_TIMER: /* Stop current timer if it is enabled */ if (kvm_xen_timer_enabled(vcpu)) { kvm_xen_stop_timer(vcpu); vcpu->arch.xen.timer_virq = 0; } 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; 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)); } r = 0; break;
I find the new version a bit harder to follow, with its init-then-stop- then-start logic:
case KVM_XEN_VCPU_ATTR_TYPE_TIMER: if (data->u.timer.port && data->u.timer.priority != KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL) { r = -EINVAL; break; }
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;
/* 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;
But I won't fight you for it.
On 8/9/22 16:16, David Woodhouse wrote:
I find the new version a bit harder to follow, with its init-then-stop- then-start logic:
case KVM_XEN_VCPU_ATTR_TYPE_TIMER: if (data->u.timer.port && data->u.timer.priority != KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL) { r = -EINVAL; break; }
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;
I think this is fine, if anything the kvm_xen_stop_timer() call could be placed in an "else" but I'm leaning towards applying this version of the patch.
Paolo
On Tue, 2022-08-09 at 16:31 +0200, Paolo Bonzini wrote:
On 8/9/22 16:16, David Woodhouse wrote:
I find the new version a bit harder to follow, with its init-then-stop- then-start logic:
case KVM_XEN_VCPU_ATTR_TYPE_TIMER: if (data->u.timer.port && data->u.timer.priority != KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL) { r = -EINVAL; break; }
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;I think this is fine, if anything the kvm_xen_stop_timer() call could be placed in an "else" but I'm leaning towards applying this version of the patch.
Fair enough. It should definitely work, and is functionally identical in all interesting cases. In that case, for both of the patches from this v3 series:
Acked-by: David Woodhouse dwmw@amazon.co.uk
Thanks.
On Tue, Aug 09, 2022, Paolo Bonzini wrote:
On 8/9/22 16:16, David Woodhouse wrote:
I find the new version a bit harder to follow, with its init-then-stop- then-start logic:
case KVM_XEN_VCPU_ATTR_TYPE_TIMER: if (data->u.timer.port && data->u.timer.priority != KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL) { r = -EINVAL; break; }
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;I think this is fine, if anything the kvm_xen_stop_timer() call could be placed in an "else" but I'm leaning towards applying this version of the patch.
I wanted to separate the "init" from the "stop+start", e.g. if there were a more appropriate place for invoking kvm_xen_init_timer() I would have suggested moving the call outside of KVM_XEN_VCPU_ATTR_TYPE_TIMER entirely.
On Tue, 2022-08-09 at 14:40 +0000, Sean Christopherson wrote:
I wanted to separate the "init" from the "stop+start", e.g. if there were a more appropriate place for invoking kvm_xen_init_timer() I would have suggested moving the call outside of KVM_XEN_VCPU_ATTR_TYPE_TIMER entirely.
Yeah, there's nowhere sensible that would apply to only Xen guests. I looked at kvm_xen_init_vcpu() but that's unconditional.
I do note that we're now calling kvm_xen_init_timer() even when an *unset* timer is being restored after live update/live migration. But I think that's OK.
On Mon, Aug 08, 2022, Coleman Dietsch wrote:
Add a check for existing xen timers before initializing a new one.
Currently kvm_xen_init_timer() is called on every KVM_XEN_VCPU_ATTR_TYPE_TIMER, which is causing the following ODEBUG crash when vcpu->arch.xen.timer is already set.
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=8234a9dfd3aafbf092cc5a7cd9842e3ebc45fc4... Reported-by: syzbot+e54f930ed78eb0f85281@syzkaller.appspotmail.com Signed-off-by: Coleman Dietsch dietschc@csp.edu
Reviewed-by: Sean Christopherson seanjc@google.com
On 8/9/22 02:32, Sean Christopherson wrote:
On Mon, Aug 08, 2022, Coleman Dietsch wrote:
Add a check for existing xen timers before initializing a new one.
Currently kvm_xen_init_timer() is called on every KVM_XEN_VCPU_ATTR_TYPE_TIMER, which is causing the following ODEBUG crash when vcpu->arch.xen.timer is already set.
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=8234a9dfd3aafbf092cc5a7cd9842e3ebc45fc4... Reported-by: syzbot+e54f930ed78eb0f85281@syzkaller.appspotmail.com Signed-off-by: Coleman Dietsch dietschc@csp.edu
Reviewed-by: Sean Christopherson seanjc@google.com
Queued both (pending resolution of David's question), thanks.
Paolo
linux-stable-mirror@lists.linaro.org
-
Coleman Dietsch -
David Woodhouse -
Paolo Bonzini -
Sean Christopherson