[PATCH v2 0/2] KVM: x86/xen: Prevent xen timer init when running

[PATCH v2 0/2] KVM: x86/xen: Prevent xen timer init when running

[Coleman Dietsch]

This patch series was created to address the following bug found by syzbot
WARNING: ODEBUG bug in kvm_xen_vcpu_set_attr.

When running the syzbot reproducer code, the following crash dump occurs:

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

The ODEBUG bug crash appears to be happening when vcpu->arch.xen.timer is
already set and kvm_xen_init_timer() is called, which appears to be the
result of two separate issues.

The first issue is that kvm_xen_init_timer() is run "every"
KVM_XEN_VCPU_ATTR_TYPE_TIMER. This is addressed in patch 1.

The second issue is that the stop xen timer code should be run before
changing the IRQ vector. This is addressed in patch 2 with some cleanup.

version 2 changes (mostly feedback from Sean Christopherson)
-split patch into 2 patches
-fix changelogs to be more descriptive
-fix formatting issues
-add check for existing xen timer before trying to initialize another one
-removed conditional for kvm_xen_stop_timer() so that it always runs
-ensure that xen timer is stopped before changing IRQ vector
-streamlined switch case KVM_XEN_VCPU_ATTR_TYPE_TIMER a bit

Coleman Dietsch (2):
  KVM: x86/xen: Initialize Xen timer only once
  KVM: x86/xen: Stop Xen timer before changing the IRQ vector

 arch/x86/kvm/xen.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

-- 
2.34.1

[PATCH v2 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

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 | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 610beba35907..2dd0f72a62f2 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -713,7 +713,10 @@ 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);
+
+			/* 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)
-- 
2.34.1

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

[Sean Christopherson]

On Fri, Jul 29, 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>
> ---

One uber nit below, otherwise:

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

>  arch/x86/kvm/xen.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index 610beba35907..2dd0f72a62f2 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -713,7 +713,10 @@ 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);
> +
> +			/* Check for existing timer */

I vote to omit the comment, "existing timer" is ambiguous, e.g. it could mean that
there's an existing _running_ timer as opposed to an existing initialized timer.
IMO the code is obvious enough on its own.

> +			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
> 

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

[Coleman Dietsch]

On Fri, Aug 05, 2022 at 06:37:13PM +0000, Sean Christopherson wrote:
> On Fri, Jul 29, 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>
> > ---
> 
> One uber nit below, otherwise:
> 
> Reviewed-by: Sean Christopherson <seanjc@google.com>
> 

Got it, thanks Sean.

> >  arch/x86/kvm/xen.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> > index 610beba35907..2dd0f72a62f2 100644
> > --- a/arch/x86/kvm/xen.c
> > +++ b/arch/x86/kvm/xen.c
> > @@ -713,7 +713,10 @@ 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);
> > +
> > +			/* Check for existing timer */
> 
> I vote to omit the comment, "existing timer" is ambiguous, e.g. it could mean that
> there's an existing _running_ timer as opposed to an existing initialized timer.
> IMO the code is obvious enough on its own.
> 

This makes perfect sense. I will ommit the comment in the next patch.

> > +			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 v2 2/2] KVM: x86/xen: Stop Xen timer before changing the IRQ vector

[Coleman Dietsch]

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

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;
 
-- 
2.34.1

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

[Sean Christopherson]

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
> 

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

[Coleman Dietsch]

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