* [PATCH] KVM: x86/xen: Fix bug in kvm_xen_vcpu_set_attr()
@ 2022-07-28 19:47 Coleman Dietsch
2022-07-28 20:41 ` Sean Christopherson via Linux-kernel-mentees
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Coleman Dietsch @ 2022-07-28 19:47 UTC (permalink / raw)
To: kvm
Cc: x86, Sean Christopherson, Dave Hansen, linux-kernel,
syzbot+e54f930ed78eb0f85281, Ingo Molnar, Borislav Petkov,
H . Peter Anvin, Paolo Bonzini, Thomas Gleixner, Pavel Skripkin,
linux-kernel-mentees
This crash appears to be happening when vcpu->arch.xen.timer is already set and kvm_xen_init_timer(vcpu) is called.
During testing with the syzbot reproducer code it seemed apparent that the else if statement in the KVM_XEN_VCPU_ATTR_TYPE_TIMER switch case was not being reached, which is where the kvm_xen_stop_timer(vcpu) call is located.
Link: https://syzkaller.appspot.com/bug?id=8234a9dfd3aafbf092cc5a7cd9842e3ebc45fc42
Reported-and-tested-by: syzbot+e54f930ed78eb0f85281@syzkaller.appspotmail.com
Signed-off-by: Coleman Dietsch <dietschc@csp.edu>
---
arch/x86/kvm/xen.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 610beba35907..4b4b985813c5 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -707,6 +707,12 @@ int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
break;
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;
@@ -720,9 +726,6 @@ int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
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;
}
r = 0;
--
2.34.1
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: x86/xen: Fix bug in kvm_xen_vcpu_set_attr()
2022-07-28 19:47 [PATCH] KVM: x86/xen: Fix bug in kvm_xen_vcpu_set_attr() Coleman Dietsch
@ 2022-07-28 20:41 ` Sean Christopherson via Linux-kernel-mentees
2022-07-28 22:49 ` Coleman Dietsch
2022-07-29 7:41 ` Greg KH
2022-08-08 13:51 ` David Woodhouse
2 siblings, 1 reply; 6+ messages in thread
From: Sean Christopherson via Linux-kernel-mentees @ 2022-07-28 20:41 UTC (permalink / raw)
To: Coleman Dietsch
Cc: x86, kvm, Pavel Skripkin, Dave Hansen, linux-kernel,
syzbot+e54f930ed78eb0f85281, Ingo Molnar, Borislav Petkov,
H . Peter Anvin, Paolo Bonzini, Thomas Gleixner,
linux-kernel-mentees
Be more specific in the shortlog. "Fix a bug in XYZ" doesn't provide any info
about the bug itself, and can even become frustratingly stale if XYZ is renamed.
I believe we should end up with two patches (see below), e.g.
KVM: x86/xen: Initialize Xen timer only once (when it's NOT running)
and
KVM: x86/xen: Stop Xen timer before changing the IRQ vector
Note, I'm assuming timer_virq is a vector of some form, I haven't actually looked
that far into the code.
On Thu, Jul 28, 2022, Coleman Dietsch wrote:
> This crash appears to be happening when vcpu->arch.xen.timer is already set
Instead of saying "This crash", provide the actual splat (sanitized to make it
more readable). That way readers, reviewers, and archaeologists don't need to
open up a hyperlink to get details on what broken.
> and kvm_xen_init_timer(vcpu) is called.
Wrap changelogs at ~75 chars.
> During testing with the syzbot reproducer code it seemed apparent that the
> else if statement in the KVM_XEN_VCPU_ATTR_TYPE_TIMER switch case was not
> being reached, which is where the kvm_xen_stop_timer(vcpu) call is located.
Neither the shortlog nor the changelog actually says anything about what is actually
being changed.
> Link: https://syzkaller.appspot.com/bug?id=8234a9dfd3aafbf092cc5a7cd9842e3ebc45fc42
> Reported-and-tested-by: syzbot+e54f930ed78eb0f85281@syzkaller.appspotmail.com
> Signed-off-by: Coleman Dietsch <dietschc@csp.edu>
> ---
> arch/x86/kvm/xen.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index 610beba35907..4b4b985813c5 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -707,6 +707,12 @@ int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
> break;
>
> 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;
I'm not entirely sure this is correct. Probably doesn't matter, but there's a
subtle ABI change here in that invoking the ioctl with a "bad" priority will
cancel any existing timer.
And there appear to be two separate bugs: initializing the hrtimer while it's
running, and not canceling a running timer before changing timer_virq.
Calling kvm_xen_init_timer() on "every" KVM_XEN_VCPU_ATTR_TYPE_TIMER is odd and
unnecessary, it only needs to be called once during vCPU setup. If Xen doesn't
have such a hook, then a !ULL check can be done on vcpu->arch.xen.timer.function
to initialize the timer on-demand.
With that out of the way, the code can be streamlined a bit, e.g. something like
this?
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;
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;
> @@ -720,9 +726,6 @@ int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
> 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;
> }
>
> r = 0;
> --
> 2.34.1
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: x86/xen: Fix bug in kvm_xen_vcpu_set_attr()
2022-07-28 20:41 ` Sean Christopherson via Linux-kernel-mentees
@ 2022-07-28 22:49 ` Coleman Dietsch
0 siblings, 0 replies; 6+ messages in thread
From: Coleman Dietsch @ 2022-07-28 22:49 UTC (permalink / raw)
To: Sean Christopherson
Cc: x86, kvm, Pavel Skripkin, Dave Hansen, linux-kernel,
syzbot+e54f930ed78eb0f85281, Ingo Molnar, Borislav Petkov,
H . Peter Anvin, Paolo Bonzini, Thomas Gleixner,
linux-kernel-mentees
On Thu, Jul 28, 2022 at 08:41:14PM +0000, Sean Christopherson wrote:
> Be more specific in the shortlog. "Fix a bug in XYZ" doesn't provide any info
> about the bug itself, and can even become frustratingly stale if XYZ is renamed.
> I believe we should end up with two patches (see below), e.g.
>
> KVM: x86/xen: Initialize Xen timer only once (when it's NOT running)
>
> and
>
> KVM: x86/xen: Stop Xen timer before changing the IRQ vector
>
Got it, I will work on splitting the v2 into a patch set as you suggested
(with better names of course).
> Note, I'm assuming timer_virq is a vector of some form, I haven't actually looked
> that far into the code.
>
> On Thu, Jul 28, 2022, Coleman Dietsch wrote:
> > This crash appears to be happening when vcpu->arch.xen.timer is already set
>
> Instead of saying "This crash", provide the actual splat (sanitized to make it
> more readable). That way readers, reviewers, and archaeologists don't need to
> open up a hyperlink to get details on what broken.
>
> > and kvm_xen_init_timer(vcpu) is called.
>
> Wrap changelogs at ~75 chars.
>
> > During testing with the syzbot reproducer code it seemed apparent that the
> > else if statement in the KVM_XEN_VCPU_ATTR_TYPE_TIMER switch case was not
> > being reached, which is where the kvm_xen_stop_timer(vcpu) call is located.
>
> Neither the shortlog nor the changelog actually says anything about what is actually
> being changed.
>
I will make sure to address all these issues in the v2 patch set.
> > Link: https://syzkaller.appspot.com/bug?id=8234a9dfd3aafbf092cc5a7cd9842e3ebc45fc42
> > Reported-and-tested-by: syzbot+e54f930ed78eb0f85281@syzkaller.appspotmail.com
> > Signed-off-by: Coleman Dietsch <dietschc@csp.edu>
> > ---
> > arch/x86/kvm/xen.c | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> > index 610beba35907..4b4b985813c5 100644
> > --- a/arch/x86/kvm/xen.c
> > +++ b/arch/x86/kvm/xen.c
> > @@ -707,6 +707,12 @@ int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
> > break;
> >
> > 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;
>
> I'm not entirely sure this is correct. Probably doesn't matter, but there's a
> subtle ABI change here in that invoking the ioctl with a "bad" priority will
> cancel any existing timer.
>
I will try to get some clarification before I send in the next patch.
> And there appear to be two separate bugs: initializing the hrtimer while it's
> running, and not canceling a running timer before changing timer_virq.
>
This does seem to be the case so I will be splitting v2 into a patch
set.
> Calling kvm_xen_init_timer() on "every" KVM_XEN_VCPU_ATTR_TYPE_TIMER is odd and
> unnecessary, it only needs to be called once during vCPU setup. If Xen doesn't
> have such a hook, then a !ULL check can be done on vcpu->arch.xen.timer.function
> to initialize the timer on-demand.
>
Yes I also thought that was a bit odd that kvm_xen_init_timer() is called on "every" KVM_XEN_VCPU_ATTR_TYPE_TIMER
> With that out of the way, the code can be streamlined a bit, e.g. something like
> this?
>
> 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;
>
> 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;
>
I agree this code could use some cleanup, I'll see what I can do.
> > @@ -720,9 +726,6 @@ int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
> > 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;
> > }
> >
> > r = 0;
> > --
> > 2.34.1
> >
Thank you for the feedback Sean, it has been most helpful!
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: x86/xen: Fix bug in kvm_xen_vcpu_set_attr()
2022-07-28 19:47 [PATCH] KVM: x86/xen: Fix bug in kvm_xen_vcpu_set_attr() Coleman Dietsch
2022-07-28 20:41 ` Sean Christopherson via Linux-kernel-mentees
@ 2022-07-29 7:41 ` Greg KH
2022-07-29 23:29 ` Coleman Dietsch
2022-08-08 13:51 ` David Woodhouse
2 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2022-07-29 7:41 UTC (permalink / raw)
To: Coleman Dietsch
Cc: syzbot+e54f930ed78eb0f85281, Dave Hansen, kvm,
Sean Christopherson, x86, linux-kernel, Ingo Molnar,
Borislav Petkov, H . Peter Anvin, Paolo Bonzini, Thomas Gleixner,
Pavel Skripkin, linux-kernel-mentees
On Thu, Jul 28, 2022 at 02:47:37PM -0500, Coleman Dietsch wrote:
> This crash appears to be happening when vcpu->arch.xen.timer is already set and kvm_xen_init_timer(vcpu) is called.
What does "this crash" refer to ?
>
> During testing with the syzbot reproducer code it seemed apparent that the else if statement in the KVM_XEN_VCPU_ATTR_TYPE_TIMER switch case was not being reached, which is where the kvm_xen_stop_timer(vcpu) call is located.
Please properly wrap your kernel changelog at 72 columns.
Didn't checkpatch.pl complain about this?
thanks,
greg k-h
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: x86/xen: Fix bug in kvm_xen_vcpu_set_attr()
2022-07-29 7:41 ` Greg KH
@ 2022-07-29 23:29 ` Coleman Dietsch
0 siblings, 0 replies; 6+ messages in thread
From: Coleman Dietsch @ 2022-07-29 23:29 UTC (permalink / raw)
To: Greg KH
Cc: syzbot+e54f930ed78eb0f85281, Dave Hansen, kvm,
Sean Christopherson, x86, linux-kernel, Ingo Molnar,
Borislav Petkov, H . Peter Anvin, Paolo Bonzini, Thomas Gleixner,
Pavel Skripkin, linux-kernel-mentees
On Fri, Jul 29, 2022 at 09:41:02AM +0200, Greg KH wrote:
> On Thu, Jul 28, 2022 at 02:47:37PM -0500, Coleman Dietsch wrote:
> > This crash appears to be happening when vcpu->arch.xen.timer is already set and kvm_xen_init_timer(vcpu) is called.
>
> What does "this crash" refer to ?
>
> >
> > During testing with the syzbot reproducer code it seemed apparent that the else if statement in the KVM_XEN_VCPU_ATTR_TYPE_TIMER switch case was not being reached, which is where the kvm_xen_stop_timer(vcpu) call is located.
>
> Please properly wrap your kernel changelog at 72 columns.
>
> Didn't checkpatch.pl complain about this?
>
I believe I made the mistake of editing the patch file directly so it was
on me for forgetting to run checkpatch.pl manually.
> thanks,
>
> greg k-h
Thanks for the feedback Greg, I believe I have (at least these) issues
resolved in the next version of the patch.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: x86/xen: Fix bug in kvm_xen_vcpu_set_attr()
2022-07-28 19:47 [PATCH] KVM: x86/xen: Fix bug in kvm_xen_vcpu_set_attr() Coleman Dietsch
2022-07-28 20:41 ` Sean Christopherson via Linux-kernel-mentees
2022-07-29 7:41 ` Greg KH
@ 2022-08-08 13:51 ` David Woodhouse
2 siblings, 0 replies; 6+ messages in thread
From: David Woodhouse @ 2022-08-08 13:51 UTC (permalink / raw)
To: Coleman Dietsch, kvm, metikaya
Cc: x86, Sean Christopherson, Dave Hansen, linux-kernel,
syzbot+e54f930ed78eb0f85281, Ingo Molnar, Borislav Petkov,
H . Peter Anvin, Paolo Bonzini, Thomas Gleixner, Pavel Skripkin,
linux-kernel-mentees
[-- Attachment #1.1: Type: text/plain, Size: 1882 bytes --]
On Thu, 2022-07-28 at 14:47 -0500, Coleman Dietsch wrote:
> This crash appears to be happening when vcpu->arch.xen.timer is
> already set and kvm_xen_init_timer(vcpu) is called.
>
> During testing with the syzbot reproducer code it seemed apparent
> that the else if statement in the KVM_XEN_VCPU_ATTR_TYPE_TIMER switch
> case was not being reached, which is where the
> kvm_xen_stop_timer(vcpu) call is located.
>
> Link: https://syzkaller.appspot.com/bug?id=8234a9dfd3aafbf092cc5a7cd9842e3ebc45fc42
>
> Reported-and-tested-by: syzbot+e54f930ed78eb0f85281@syzkaller.appspotmail.com
>
> Signed-off-by: Coleman Dietsch <dietschc@csp.edu>
Modulo the cosmetic issues discussed,
Acked-by: David Woodhouse <dwmw@amazon.co.uk>
Thanks.
> ---
> arch/x86/kvm/xen.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index 610beba35907..4b4b985813c5 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -707,6 +707,12 @@ int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
> break;
>
> 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;
> @@ -720,9 +726,6 @@ int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
> 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;
> }
>
> r = 0;
>
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]
[-- Attachment #2: Type: text/plain, Size: 201 bytes --]
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-08-08 13:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-28 19:47 [PATCH] KVM: x86/xen: Fix bug in kvm_xen_vcpu_set_attr() Coleman Dietsch
2022-07-28 20:41 ` Sean Christopherson via Linux-kernel-mentees
2022-07-28 22:49 ` Coleman Dietsch
2022-07-29 7:41 ` Greg KH
2022-07-29 23:29 ` Coleman Dietsch
2022-08-08 13:51 ` David Woodhouse
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).