| Message ID | 20171122142627.73170-2-borntraeger@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x fixes (post 2.11) | expand |
On 22.11.2017 15:26, Christian Borntraeger wrote: > valgrind pointed out that we call KVM_S390_GET_IRQ_STATE with an > undefined value for flags. Right now this is unused, but we > better play safe. > The same is true for SET_IRQ_STATE. While QEMU is now fixed in > that regard, we should make sure to not use the flag and pad > fields in the kernel. A corresponding kernel documentation patch > will be submitted later. > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > target/s390x/kvm.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > index 343fcec..76065ec 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -2069,7 +2069,10 @@ int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state) > > void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) > { > - struct kvm_s390_irq_state irq_state; > + struct kvm_s390_irq_state irq_state = { > + .buf = (uint64_t) cpu->irqstate, > + .len = VCPU_IRQ_BUF_SIZE, > + }; > CPUState *cs = CPU(cpu); > int32_t bytes; > > @@ -2077,9 +2080,6 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) > return; > } > > - irq_state.buf = (uint64_t) cpu->irqstate; > - irq_state.len = VCPU_IRQ_BUF_SIZE; > - > bytes = kvm_vcpu_ioctl(cs, KVM_S390_GET_IRQ_STATE, &irq_state); > if (bytes < 0) { > cpu->irqstate_saved_size = 0; > @@ -2093,7 +2093,10 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) > int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu) > { > CPUState *cs = CPU(cpu); > - struct kvm_s390_irq_state irq_state; > + struct kvm_s390_irq_state irq_state = { > + .buf = (uint64_t) cpu->irqstate, > + .len = cpu->irqstate_saved_size, > + }; > int r; > > if (cpu->irqstate_saved_size == 0) { > @@ -2104,9 +2107,6 @@ int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu) > return -ENOSYS; > } > > - irq_state.buf = (uint64_t) cpu->irqstate; > - irq_state.len = cpu->irqstate_saved_size; > - > r = kvm_vcpu_ioctl(cs, KVM_S390_SET_IRQ_STATE, &irq_state); > if (r) { > error_report("Setting interrupt state failed %d", r); > Reviewed-by: Thomas Huth <thuth@redhat.com> IMHO that would still be fine for 2.11, too. Thomas
On Wed, 22 Nov 2017 15:26:26 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > valgrind pointed out that we call KVM_S390_GET_IRQ_STATE with an > undefined value for flags. Right now this is unused, but we > better play safe. > The same is true for SET_IRQ_STATE. While QEMU is now fixed in > that regard, we should make sure to not use the flag and pad > fields in the kernel. A corresponding kernel documentation patch > will be submitted later. I'd reword this a bit; it is confusing to read changelogs describing a then-future change which already happened. (I assume that we will do the kernel sanitizing for 4.15?) "valgrind pointed out that we call KVM_S390_GET_IRQ_STATE with an undefined value for flags. Kernels prior to 4.15 did not use that field, and later kernels ignore it for compatibility reasons, but we better play safe. The same is true for SET_IRQ_STATE. We should make sure to not use the flag field, either." [I do not see a 'pad' field in the structure; that is a bug in the kernel documentation.] > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > target/s390x/kvm.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > index 343fcec..76065ec 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -2069,7 +2069,10 @@ int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state) > > void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) > { > - struct kvm_s390_irq_state irq_state; > + struct kvm_s390_irq_state irq_state = { > + .buf = (uint64_t) cpu->irqstate, > + .len = VCPU_IRQ_BUF_SIZE, > + }; > CPUState *cs = CPU(cpu); > int32_t bytes; > > @@ -2077,9 +2080,6 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) > return; > } > > - irq_state.buf = (uint64_t) cpu->irqstate; > - irq_state.len = VCPU_IRQ_BUF_SIZE; > - > bytes = kvm_vcpu_ioctl(cs, KVM_S390_GET_IRQ_STATE, &irq_state); > if (bytes < 0) { > cpu->irqstate_saved_size = 0; > @@ -2093,7 +2093,10 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) > int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu) > { > CPUState *cs = CPU(cpu); > - struct kvm_s390_irq_state irq_state; > + struct kvm_s390_irq_state irq_state = { > + .buf = (uint64_t) cpu->irqstate, > + .len = cpu->irqstate_saved_size, > + }; > int r; > > if (cpu->irqstate_saved_size == 0) { > @@ -2104,9 +2107,6 @@ int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu) > return -ENOSYS; > } > > - irq_state.buf = (uint64_t) cpu->irqstate; > - irq_state.len = cpu->irqstate_saved_size; > - > r = kvm_vcpu_ioctl(cs, KVM_S390_SET_IRQ_STATE, &irq_state); > if (r) { > error_report("Setting interrupt state failed %d", r); Patch looks good.
On Wed, 22 Nov 2017 15:37:55 +0100 Thomas Huth <thuth@redhat.com> wrote: > On 22.11.2017 15:26, Christian Borntraeger wrote: > > valgrind pointed out that we call KVM_S390_GET_IRQ_STATE with an > > undefined value for flags. Right now this is unused, but we > > better play safe. > > The same is true for SET_IRQ_STATE. While QEMU is now fixed in > > that regard, we should make sure to not use the flag and pad > > fields in the kernel. A corresponding kernel documentation patch > > will be submitted later. > > > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > > --- > > target/s390x/kvm.c | 16 ++++++++-------- > > 1 file changed, 8 insertions(+), 8 deletions(-) (...) > Reviewed-by: Thomas Huth <thuth@redhat.com> > > IMHO that would still be fine for 2.11, too. I can queue this if another fix comes along, but I'd not push it on its own.
On 11/22/2017 03:43 PM, Cornelia Huck wrote: > On Wed, 22 Nov 2017 15:26:26 +0100 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> valgrind pointed out that we call KVM_S390_GET_IRQ_STATE with an >> undefined value for flags. Right now this is unused, but we >> better play safe. >> The same is true for SET_IRQ_STATE. While QEMU is now fixed in >> that regard, we should make sure to not use the flag and pad >> fields in the kernel. A corresponding kernel documentation patch >> will be submitted later. > > I'd reword this a bit; it is confusing to read changelogs describing a > then-future change which already happened. (I assume that we will do > the kernel sanitizing for 4.15?) > > "valgrind pointed out that we call KVM_S390_GET_IRQ_STATE with an > undefined value for flags. Kernels prior to 4.15 did not use that > field, and later kernels ignore it for compatibility reasons, but we > better play safe. > > The same is true for SET_IRQ_STATE. We should make sure to not use the > flag field, either." > > [I do not see a 'pad' field in the structure; that is a bug in the > kernel documentation.] Would be ok for me. Can you take the patch and change the wording yourself? > >> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> target/s390x/kvm.c | 16 ++++++++-------- >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >> index 343fcec..76065ec 100644 >> --- a/target/s390x/kvm.c >> +++ b/target/s390x/kvm.c >> @@ -2069,7 +2069,10 @@ int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state) >> >> void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) >> { >> - struct kvm_s390_irq_state irq_state; >> + struct kvm_s390_irq_state irq_state = { >> + .buf = (uint64_t) cpu->irqstate, >> + .len = VCPU_IRQ_BUF_SIZE, >> + }; >> CPUState *cs = CPU(cpu); >> int32_t bytes; >> >> @@ -2077,9 +2080,6 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) >> return; >> } >> >> - irq_state.buf = (uint64_t) cpu->irqstate; >> - irq_state.len = VCPU_IRQ_BUF_SIZE; >> - >> bytes = kvm_vcpu_ioctl(cs, KVM_S390_GET_IRQ_STATE, &irq_state); >> if (bytes < 0) { >> cpu->irqstate_saved_size = 0; >> @@ -2093,7 +2093,10 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) >> int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu) >> { >> CPUState *cs = CPU(cpu); >> - struct kvm_s390_irq_state irq_state; >> + struct kvm_s390_irq_state irq_state = { >> + .buf = (uint64_t) cpu->irqstate, >> + .len = cpu->irqstate_saved_size, >> + }; >> int r; >> >> if (cpu->irqstate_saved_size == 0) { >> @@ -2104,9 +2107,6 @@ int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu) >> return -ENOSYS; >> } >> >> - irq_state.buf = (uint64_t) cpu->irqstate; >> - irq_state.len = cpu->irqstate_saved_size; >> - >> r = kvm_vcpu_ioctl(cs, KVM_S390_SET_IRQ_STATE, &irq_state); >> if (r) { >> error_report("Setting interrupt state failed %d", r); > > Patch looks good. >
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c index 343fcec..76065ec 100644 --- a/target/s390x/kvm.c +++ b/target/s390x/kvm.c @@ -2069,7 +2069,10 @@ int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state) void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) { - struct kvm_s390_irq_state irq_state; + struct kvm_s390_irq_state irq_state = { + .buf = (uint64_t) cpu->irqstate, + .len = VCPU_IRQ_BUF_SIZE, + }; CPUState *cs = CPU(cpu); int32_t bytes; @@ -2077,9 +2080,6 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) return; } - irq_state.buf = (uint64_t) cpu->irqstate; - irq_state.len = VCPU_IRQ_BUF_SIZE; - bytes = kvm_vcpu_ioctl(cs, KVM_S390_GET_IRQ_STATE, &irq_state); if (bytes < 0) { cpu->irqstate_saved_size = 0; @@ -2093,7 +2093,10 @@ void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu) { CPUState *cs = CPU(cpu); - struct kvm_s390_irq_state irq_state; + struct kvm_s390_irq_state irq_state = { + .buf = (uint64_t) cpu->irqstate, + .len = cpu->irqstate_saved_size, + }; int r; if (cpu->irqstate_saved_size == 0) { @@ -2104,9 +2107,6 @@ int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu) return -ENOSYS; } - irq_state.buf = (uint64_t) cpu->irqstate; - irq_state.len = cpu->irqstate_saved_size; - r = kvm_vcpu_ioctl(cs, KVM_S390_SET_IRQ_STATE, &irq_state); if (r) { error_report("Setting interrupt state failed %d", r);
valgrind pointed out that we call KVM_S390_GET_IRQ_STATE with an undefined value for flags. Right now this is unused, but we better play safe. The same is true for SET_IRQ_STATE. While QEMU is now fixed in that regard, we should make sure to not use the flag and pad fields in the kernel. A corresponding kernel documentation patch will be submitted later. Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> --- target/s390x/kvm.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)