| Message ID | 1511278211-12257-5-git-send-email-liran.alon@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2017-11-21 17:30+0200, Liran Alon: > An alternative could have been done to return -EBUSY in this case. > For now, we decided to just silently override exception and warn on > such an attempt. > > Signed-off-by: Liran Alon <liran.alon@oracle.com> > Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com> > Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > --- > arch/x86/kvm/x86.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 1490da89de4b..c8cec7c39c1c 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -3153,12 +3153,25 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu, > return -EINVAL; > > process_nmi(vcpu); > + > + /* > + * Warn if userspace is overriding existing > + * injected exception > + */ > + WARN_ON_ONCE(vcpu->arch.exception.injected && > + events->exception.injected); I think that overwriting the injected exception/interrupt is a perfectly valid operation -- userspace could have rolled back the state to a time of the previous injection. Syzkaller would complain sooner or later and I don't see it as a useful printk, so dropping this patch would be preferred, thanks.
On 22/11/17 22:34, Radim Krčmář wrote: > 2017-11-21 17:30+0200, Liran Alon: >> An alternative could have been done to return -EBUSY in this case. >> For now, we decided to just silently override exception and warn on >> such an attempt. >> >> Signed-off-by: Liran Alon <liran.alon@oracle.com> >> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com> >> Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> >> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> >> --- >> arch/x86/kvm/x86.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 1490da89de4b..c8cec7c39c1c 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -3153,12 +3153,25 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu, >> return -EINVAL; >> >> process_nmi(vcpu); >> + >> + /* >> + * Warn if userspace is overriding existing >> + * injected exception >> + */ >> + WARN_ON_ONCE(vcpu->arch.exception.injected && >> + events->exception.injected); > > I think that overwriting the injected exception/interrupt is a perfectly > valid operation -- userspace could have rolled back the state to a time > of the previous injection. > > Syzkaller would complain sooner or later and I don't see it as a useful > printk, so dropping this patch would be preferred, > > thanks. > Hmm haven't thought about this use-case. I agree. This patch should be dropped from series. Thanks for spotting this. -Liran
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 1490da89de4b..c8cec7c39c1c 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3153,12 +3153,25 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu, return -EINVAL; process_nmi(vcpu); + + /* + * Warn if userspace is overriding existing + * injected exception + */ + WARN_ON_ONCE(vcpu->arch.exception.injected && + events->exception.injected); vcpu->arch.exception.injected = events->exception.injected; vcpu->arch.exception.pending = false; vcpu->arch.exception.nr = events->exception.nr; vcpu->arch.exception.has_error_code = events->exception.has_error_code; vcpu->arch.exception.error_code = events->exception.error_code; + /* + * Warn if userspace is overriding existing + * injected interrupt + */ + WARN_ON_ONCE(vcpu->arch.interrupt.injected && + events->interrupt.injected); vcpu->arch.interrupt.injected = events->interrupt.injected; vcpu->arch.interrupt.nr = events->interrupt.nr; vcpu->arch.interrupt.soft = events->interrupt.soft;