cpu: don't allow negative core id

[Qemu-devel] [PATCH] cpu: don't allow negative core id

Posted by Laurent Vivier 6 years, 8 months ago

With pseries machine type a negative core-id is not managed properly:
-1 gives an inaccurate error message ("core -1 already populated"),
-2 crashes QEMU (core dump)

As it seems a negative value is invalid for any architecture,
instead of checking this in spapr_core_pre_plug() I think it's better
to check this in the generic part, core_prop_set_core_id()

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/cpu/core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/cpu/core.c b/hw/cpu/core.c
index 2bf960d..bd578ab 100644
--- a/hw/cpu/core.c
+++ b/hw/cpu/core.c
@@ -33,6 +33,11 @@ static void core_prop_set_core_id(Object *obj, Visitor *v, const char *name,
         return;
     }
 
+    if (value < 0) {
+        error_setg(errp, "Invalid core id %"PRId64, value);
+        return;
+    }
+
     core->core_id = value;
 }
 
-- 
2.9.4

Re: [Qemu-devel] [PATCH] cpu: don't allow negative core id

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

Hi Laurent,

On Wed, Aug 2, 2017 at 7:32 AM, Laurent Vivier <lvivier@redhat.com> wrote:
> With pseries machine type a negative core-id is not managed properly:
> -1 gives an inaccurate error message ("core -1 already populated"),
> -2 crashes QEMU (core dump)
>
> As it seems a negative value is invalid for any architecture,
> instead of checking this in spapr_core_pre_plug() I think it's better
> to check this in the generic part, core_prop_set_core_id()

Why is this property signed? If there is not reason to use it negative,
is it possible to use object_property_add(.."uint"..)?

Also what about core_prop_set_nr_threads()? It might coredump the
same way.

Regards,

Phil.

>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  hw/cpu/core.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> index 2bf960d..bd578ab 100644
> --- a/hw/cpu/core.c
> +++ b/hw/cpu/core.c
> @@ -33,6 +33,11 @@ static void core_prop_set_core_id(Object *obj, Visitor *v, const char *name,
>          return;
>      }
>
> +    if (value < 0) {
> +        error_setg(errp, "Invalid core id %"PRId64, value);
> +        return;
> +    }
> +
>      core->core_id = value;
>  }
>
> --
> 2.9.4
>
>

Re: [Qemu-devel] [PATCH] cpu: don't allow negative core id

Posted by Laurent Vivier 6 years, 8 months ago

On 02/08/2017 15:42, Philippe Mathieu-Daudé wrote:
> Hi Laurent,
> 
> On Wed, Aug 2, 2017 at 7:32 AM, Laurent Vivier <lvivier@redhat.com> wrote:
>> With pseries machine type a negative core-id is not managed properly:
>> -1 gives an inaccurate error message ("core -1 already populated"),
>> -2 crashes QEMU (core dump)
>>
>> As it seems a negative value is invalid for any architecture,
>> instead of checking this in spapr_core_pre_plug() I think it's better
>> to check this in the generic part, core_prop_set_core_id()
> 
> Why is this property signed? If there is not reason to use it negative,
> is it possible to use object_property_add(.."uint"..)?

You should be right:

{ 'struct': 'NumaNodeOptions',
  'data': {
   '*nodeid': 'uint16',
   '*cpus':   ['uint16'],
   '*mem':    'size',
   '*memdev': 'str' }}

but

{ 'struct': 'CpuInstanceProperties',
  'data': { '*node-id': 'int',
            '*socket-id': 'int',
            '*core-id': 'int',
            '*thread-id': 'int'
  }
}

But I'm not sure it's a good idea to change the API now.

> Also what about core_prop_set_nr_threads()? It might coredump the
> same way.

In pseries case, there is another test in the spapr part that prevents that.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH] cpu: don't allow negative core id

Posted by Eduardo Habkost 6 years, 8 months ago

On Wed, Aug 02, 2017 at 03:50:36PM +0200, Laurent Vivier wrote:
> On 02/08/2017 15:42, Philippe Mathieu-Daudé wrote:
> > Hi Laurent,
> > 
> > On Wed, Aug 2, 2017 at 7:32 AM, Laurent Vivier <lvivier@redhat.com> wrote:
> >> With pseries machine type a negative core-id is not managed properly:
> >> -1 gives an inaccurate error message ("core -1 already populated"),
> >> -2 crashes QEMU (core dump)
> >>
> >> As it seems a negative value is invalid for any architecture,
> >> instead of checking this in spapr_core_pre_plug() I think it's better
> >> to check this in the generic part, core_prop_set_core_id()
> > 
> > Why is this property signed? If there is not reason to use it negative,
> > is it possible to use object_property_add(.."uint"..)?
> 
> You should be right:
> 
> { 'struct': 'NumaNodeOptions',
>   'data': {
>    '*nodeid': 'uint16',
>    '*cpus':   ['uint16'],
>    '*mem':    'size',
>    '*memdev': 'str' }}
> 
> but
> 
> { 'struct': 'CpuInstanceProperties',
>   'data': { '*node-id': 'int',
>             '*socket-id': 'int',
>             '*core-id': 'int',
>             '*thread-id': 'int'
>   }
> }
> 
> But I'm not sure it's a good idea to change the API now.

Property parsing is not affected by the QAPI schema at all, so
touching the schema wouldn't fix the bug.

The same applies to the 'type' argument to object_property_add():
it is ignored everywhere.

However, the property setter can simply use a visitor for unsigned values, and
it will reject negative values automatically, e.g.:

  diff --git a/hw/cpu/core.c b/hw/cpu/core.c
  index 2bf960d..b5af2bf 100644
  --- a/hw/cpu/core.c
  +++ b/hw/cpu/core.c
  @@ -25,9 +25,9 @@ static void core_prop_set_core_id(Object *obj, Visitor *v, const char *name,
   {
       CPUCore *core = CPU_CORE(obj);
       Error *local_err = NULL;
  -    int64_t value;
  +    uint32_t value;
   
  -    visit_type_int(v, name, &value, &local_err);
  +    visit_type_uint32(v, name, &value, &local_err);
       if (local_err) {
           error_propagate(errp, local_err);
           return;


  $ ./ppc64-softmmu/qemu-system-ppc64 -device POWER8_v2.0-spapr-cpu-core,core-id=-2
  qemu-system-ppc64: -device POWER8_v2.0-spapr-cpu-core,core-id=-2: Parameter 'core-id' expects uint32_t


I would suggest changing the CPUCore struct fields to uint32_t or
uint64_t, but it would be more intrusive and we're past hard
freeze.  Your patch looks good for 2.10.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

I'm queueing it on machine-next.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] cpu: don't allow negative core id

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

On 08/02/2017 06:29 PM, Eduardo Habkost wrote:
> On Wed, Aug 02, 2017 at 03:50:36PM +0200, Laurent Vivier wrote:
>> On 02/08/2017 15:42, Philippe Mathieu-Daudé wrote:
>>> Hi Laurent,
>>>
>>> On Wed, Aug 2, 2017 at 7:32 AM, Laurent Vivier <lvivier@redhat.com> wrote:
>>>> With pseries machine type a negative core-id is not managed properly:
>>>> -1 gives an inaccurate error message ("core -1 already populated"),
>>>> -2 crashes QEMU (core dump)
>>>>
>>>> As it seems a negative value is invalid for any architecture,
>>>> instead of checking this in spapr_core_pre_plug() I think it's better
>>>> to check this in the generic part, core_prop_set_core_id()
>>>
>>> Why is this property signed? If there is not reason to use it negative,
>>> is it possible to use object_property_add(.."uint"..)?
>>
>> You should be right:
>>
>> { 'struct': 'NumaNodeOptions',
>>    'data': {
>>     '*nodeid': 'uint16',
>>     '*cpus':   ['uint16'],
>>     '*mem':    'size',
>>     '*memdev': 'str' }}
>>
>> but
>>
>> { 'struct': 'CpuInstanceProperties',
>>    'data': { '*node-id': 'int',
>>              '*socket-id': 'int',
>>              '*core-id': 'int',
>>              '*thread-id': 'int'
>>    }
>> }
>>
>> But I'm not sure it's a good idea to change the API now.
> 
> Property parsing is not affected by the QAPI schema at all, so
> touching the schema wouldn't fix the bug.
> 
> The same applies to the 'type' argument to object_property_add():
> it is ignored everywhere.
> 
> However, the property setter can simply use a visitor for unsigned values, and
> it will reject negative values automatically, e.g.:
> 
>    diff --git a/hw/cpu/core.c b/hw/cpu/core.c
>    index 2bf960d..b5af2bf 100644
>    --- a/hw/cpu/core.c
>    +++ b/hw/cpu/core.c
>    @@ -25,9 +25,9 @@ static void core_prop_set_core_id(Object *obj, Visitor *v, const char *name,
>     {
>         CPUCore *core = CPU_CORE(obj);
>         Error *local_err = NULL;
>    -    int64_t value;
>    +    uint32_t value;
>     
>    -    visit_type_int(v, name, &value, &local_err);
>    +    visit_type_uint32(v, name, &value, &local_err);
>         if (local_err) {
>             error_propagate(errp, local_err);
>             return;
> 
> 
>    $ ./ppc64-softmmu/qemu-system-ppc64 -device POWER8_v2.0-spapr-cpu-core,core-id=-2
>    qemu-system-ppc64: -device POWER8_v2.0-spapr-cpu-core,core-id=-2: Parameter 'core-id' expects uint32_t
> 
> 
> I would suggest changing the CPUCore struct fields to uint32_t or
> uint64_t, but it would be more intrusive and we're past hard
> freeze.  Your patch looks good for 2.10.

Laurent with a /* TODO ... */  you can add:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> I'm queueing it on machine-next.
>

Re: [Qemu-devel] [PATCH] cpu: don't allow negative core id

Posted by Igor Mammedov 6 years, 8 months ago

On Wed, 2 Aug 2017 18:29:33 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Aug 02, 2017 at 03:50:36PM +0200, Laurent Vivier wrote:
> > On 02/08/2017 15:42, Philippe Mathieu-Daudé wrote:  
> > > Hi Laurent,
> > > 
> > > On Wed, Aug 2, 2017 at 7:32 AM, Laurent Vivier <lvivier@redhat.com> wrote:  
> > >> With pseries machine type a negative core-id is not managed properly:
> > >> -1 gives an inaccurate error message ("core -1 already populated"),
> > >> -2 crashes QEMU (core dump)
> > >>
> > >> As it seems a negative value is invalid for any architecture,
> > >> instead of checking this in spapr_core_pre_plug() I think it's better
> > >> to check this in the generic part, core_prop_set_core_id()  
> > > 
> > > Why is this property signed? If there is not reason to use it negative,
> > > is it possible to use object_property_add(.."uint"..)?  
> > 
> > You should be right:
> > 
> > { 'struct': 'NumaNodeOptions',
> >   'data': {
> >    '*nodeid': 'uint16',
> >    '*cpus':   ['uint16'],
> >    '*mem':    'size',
> >    '*memdev': 'str' }}
> > 
> > but
> > 
> > { 'struct': 'CpuInstanceProperties',
> >   'data': { '*node-id': 'int',
> >             '*socket-id': 'int',
> >             '*core-id': 'int',
> >             '*thread-id': 'int'
> >   }
> > }
> > 
> > But I'm not sure it's a good idea to change the API now.  
> 
> Property parsing is not affected by the QAPI schema at all, so
> touching the schema wouldn't fix the bug.
> 
> The same applies to the 'type' argument to object_property_add():
> it is ignored everywhere.
> 
> However, the property setter can simply use a visitor for unsigned values, and
> it will reject negative values automatically, e.g.:
> 
>   diff --git a/hw/cpu/core.c b/hw/cpu/core.c
>   index 2bf960d..b5af2bf 100644
>   --- a/hw/cpu/core.c
>   +++ b/hw/cpu/core.c
>   @@ -25,9 +25,9 @@ static void core_prop_set_core_id(Object *obj, Visitor *v, const char *name,
>    {
>        CPUCore *core = CPU_CORE(obj);
>        Error *local_err = NULL;
>   -    int64_t value;
>   +    uint32_t value;
>    
>   -    visit_type_int(v, name, &value, &local_err);
>   +    visit_type_uint32(v, name, &value, &local_err);
>        if (local_err) {
>            error_propagate(errp, local_err);
>            return;
> 
> 
>   $ ./ppc64-softmmu/qemu-system-ppc64 -device POWER8_v2.0-spapr-cpu-core,core-id=-2
>   qemu-system-ppc64: -device POWER8_v2.0-spapr-cpu-core,core-id=-2: Parameter 'core-id' expects uint32_t
> 
> 
> I would suggest changing the CPUCore struct fields to uint32_t or
> uint64_t, but it would be more intrusive and we're past hard
> freeze.  Your patch looks good for 2.10.
there is one reason to use signed int here,
negative values might be used to mark not set property value,
I recall we do something like this in target-i386.


> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> I'm queueing it on machine-next.
>

Re: [Qemu-devel] [Qemu-ppc] [PATCH] cpu: don't allow negative core id

Posted by Greg Kurz 6 years, 8 months ago

On Wed,  2 Aug 2017 12:32:59 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> With pseries machine type a negative core-id is not managed properly:
> -1 gives an inaccurate error message ("core -1 already populated"),
> -2 crashes QEMU (core dump)
> 
> As it seems a negative value is invalid for any architecture,
> instead of checking this in spapr_core_pre_plug() I think it's better
> to check this in the generic part, core_prop_set_core_id()
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---

Not sure why core_id is signed but, as you said to Philippe, it's probably
not the right time to change the API.

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/cpu/core.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> index 2bf960d..bd578ab 100644
> --- a/hw/cpu/core.c
> +++ b/hw/cpu/core.c
> @@ -33,6 +33,11 @@ static void core_prop_set_core_id(Object *obj, Visitor *v, const char *name,
>          return;
>      }
>  
> +    if (value < 0) {
> +        error_setg(errp, "Invalid core id %"PRId64, value);
> +        return;
> +    }
> +
>      core->core_id = value;
>  }
>

Re: [Qemu-devel] [PATCH] cpu: don't allow negative core id

Posted by David Gibson 6 years, 8 months ago

On Wed, Aug 02, 2017 at 12:32:59PM +0200, Laurent Vivier wrote:
> With pseries machine type a negative core-id is not managed properly:
> -1 gives an inaccurate error message ("core -1 already populated"),
> -2 crashes QEMU (core dump)
> 
> As it seems a negative value is invalid for any architecture,
> instead of checking this in spapr_core_pre_plug() I think it's better
> to check this in the generic part, core_prop_set_core_id()
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/cpu/core.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> index 2bf960d..bd578ab 100644
> --- a/hw/cpu/core.c
> +++ b/hw/cpu/core.c
> @@ -33,6 +33,11 @@ static void core_prop_set_core_id(Object *obj, Visitor *v, const char *name,
>          return;
>      }
>  
> +    if (value < 0) {
> +        error_setg(errp, "Invalid core id %"PRId64, value);
> +        return;
> +    }
> +
>      core->core_id = value;
>  }
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson