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