[PATCH v3 1/2] KVM: x86/xen: Initialize Xen timer only once

[PATCH v3 1/2] KVM: x86/xen: Initialize Xen timer only once

[Coleman Dietsch]

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=8234a9dfd3aafbf092cc5a7cd9842e3ebc45fc42
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)
-- 
2.34.1

[PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ

[Coleman Dietsch]

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=8234a9dfd3aafbf092cc5a7cd9842e3ebc45fc42
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;
 
-- 
2.34.1

Re: [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ

[Sean Christopherson]

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=8234a9dfd3aafbf092cc5a7cd9842e3ebc45fc42
> Reported-by: syzbot+e54f930ed78eb0f85281@syzkaller.appspotmail.com
> Signed-off-by: Coleman Dietsch <dietschc@csp.edu>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 569 bytes --]

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?

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ

[Paolo Bonzini]

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

Re: [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 850 bytes --]

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@infradead.org/

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ

[Sean Christopherson]

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@infradead.org/

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).

Re: [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2954 bytes --]

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@infradead.org/
> > 
> 
> 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. 


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ

[Paolo Bonzini]

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

Re: [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1061 bytes --]

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.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ

[Sean Christopherson]

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.

Re: [PATCH v3 2/2] KVM: x86/xen: Stop Xen timer before changing IRQ

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 585 bytes --]

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.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH v3 1/2] KVM: x86/xen: Initialize Xen timer only once

[Sean Christopherson]

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=8234a9dfd3aafbf092cc5a7cd9842e3ebc45fc42
> Reported-by: syzbot+e54f930ed78eb0f85281@syzkaller.appspotmail.com
> Signed-off-by: Coleman Dietsch <dietschc@csp.edu>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH v3 1/2] KVM: x86/xen: Initialize Xen timer only once

[Paolo Bonzini]

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=8234a9dfd3aafbf092cc5a7cd9842e3ebc45fc42
>> 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