cpu: Avoid latent bug calling cpu_reset() on uninitialized vCPU

[PATCH 2/2] cpu: Assert a vCPU is created before resetting it

Posted by Philippe Mathieu-Daudé 5 years, 8 months ago

cpu_reset() might modify architecture-specific fields allocated
by qemu_init_vcpu(). To avoid bugs similar to the one fixed in
commit 00d0f7cb66 when introducing new architectures, assert a
vCPU is created before resetting it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/core/cpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/core/cpu.c b/hw/core/cpu.c
index fe65ca62ac..09e49f8d6a 100644
--- a/hw/core/cpu.c
+++ b/hw/core/cpu.c
@@ -251,6 +251,7 @@ void cpu_reset(CPUState *cpu)
 {
     CPUClass *klass = CPU_GET_CLASS(cpu);
 
+    assert(cpu->created);
     if (klass->reset != NULL) {
         (*klass->reset)(cpu);
     }
-- 
2.21.1

Re: [PATCH 2/2] cpu: Assert a vCPU is created before resetting it

Posted by Peter Maydell 5 years, 8 months ago

On Mon, 9 Mar 2020 at 12:11, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> cpu_reset() might modify architecture-specific fields allocated
> by qemu_init_vcpu(). To avoid bugs similar to the one fixed in
> commit 00d0f7cb66 when introducing new architectures, assert a
> vCPU is created before resetting it.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/core/cpu.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/core/cpu.c b/hw/core/cpu.c
> index fe65ca62ac..09e49f8d6a 100644
> --- a/hw/core/cpu.c
> +++ b/hw/core/cpu.c
> @@ -251,6 +251,7 @@ void cpu_reset(CPUState *cpu)
>  {
>      CPUClass *klass = CPU_GET_CLASS(cpu);
>
> +    assert(cpu->created);
>      if (klass->reset != NULL) {
>          (*klass->reset)(cpu);
>      }

This will conflict with the change to use DeviceClass::reset.

Ideally we should do an equivalent assert in the DeviceClass
(and flush out all the bugs where we forgot to realize the
device before using it).

thanks
-- PMM

Re: [PATCH 2/2] cpu: Assert a vCPU is created before resetting it

Posted by Philippe Mathieu-Daudé 5 years, 8 months ago

On 3/9/20 2:10 PM, Peter Maydell wrote:
> On Mon, 9 Mar 2020 at 12:11, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> cpu_reset() might modify architecture-specific fields allocated
>> by qemu_init_vcpu(). To avoid bugs similar to the one fixed in
>> commit 00d0f7cb66 when introducing new architectures, assert a
>> vCPU is created before resetting it.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   hw/core/cpu.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/core/cpu.c b/hw/core/cpu.c
>> index fe65ca62ac..09e49f8d6a 100644
>> --- a/hw/core/cpu.c
>> +++ b/hw/core/cpu.c
>> @@ -251,6 +251,7 @@ void cpu_reset(CPUState *cpu)
>>   {
>>       CPUClass *klass = CPU_GET_CLASS(cpu);
>>
>> +    assert(cpu->created);
>>       if (klass->reset != NULL) {
>>           (*klass->reset)(cpu);
>>       }
> 
> This will conflict with the change to use DeviceClass::reset.
> 
> Ideally we should do an equivalent assert in the DeviceClass
> (and flush out all the bugs where we forgot to realize the
> device before using it).

OK (I should have sent as RFC probably).

Anyway this fails the ppc64le/s390x linux-user tests on Travis-CI:

qemu-ppc64le: hw/core/cpu.c:254: cpu_reset: Assertion `cpu->created' failed.

qemu-s390x: hw/core/cpu.c:254: cpu_reset: Assertion `cpu->created' failed.

> 
> thanks
> -- PMM
>