[v1] xen/sched: fix sched_move_domain() for domain without vcpus

[PATCH] xen/sched: fix sched_move_domain() for domain without vcpus

Posted by Juergen Gross 4 years, 5 months ago

In case a domain is created with a cpupool other than Pool-0 specified
it will be moved to that cpupool before any vcpus are allocated.

This will lead to a NULL pointer dereference in sched_move_domain().

Fix that by tolerating vcpus not being allocated yet.

Fixes: 70fadc41635b9b6 ("xen/cpupool: support moving domain between cpupools with different granularity")
Reported-by: Bertrand Marquis <bertrand.marquis@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/sched/core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 8d178baf3d..79c9100680 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -671,6 +671,10 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
 
     for ( unit_idx = 0; unit_idx < n_units; unit_idx++ )
     {
+        /* Special case for move at domain creation time. */
+        if ( !d->vcpu[unit_idx * gran] )
+            break;
+
         unit = sched_alloc_unit_mem();
         if ( unit )
         {
-- 
2.26.2

Re: [PATCH] xen/sched: fix sched_move_domain() for domain without vcpus

Posted by Andrew Cooper 4 years, 5 months ago

On 06/09/2021 12:00, Juergen Gross wrote:
> In case a domain is created with a cpupool other than Pool-0 specified
> it will be moved to that cpupool before any vcpus are allocated.
>
> This will lead to a NULL pointer dereference in sched_move_domain().
>
> Fix that by tolerating vcpus not being allocated yet.
>
> Fixes: 70fadc41635b9b6 ("xen/cpupool: support moving domain between cpupools with different granularity")
> Reported-by: Bertrand Marquis <bertrand.marquis@arm.com>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  xen/common/sched/core.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> index 8d178baf3d..79c9100680 100644
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -671,6 +671,10 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
>  
>      for ( unit_idx = 0; unit_idx < n_units; unit_idx++ )
>      {
> +        /* Special case for move at domain creation time. */
> +        if ( !d->vcpu[unit_idx * gran] )
> +            break;
> +
>          unit = sched_alloc_unit_mem();
>          if ( unit )
>          {

I think the logic would be clearer if you wrap the entire for loop in if
( d->max_vcpus ).  This loop is only allocating units in the new
scheduler for existing vcpus, so there's no point entering the loop at
all during domain creation.

Also, this removes a non-speculatively-guarded d->vcpu[] deference.

~Andrew

Re: [PATCH] xen/sched: fix sched_move_domain() for domain without vcpus

Posted by Andrew Cooper 4 years, 5 months ago

On 06/09/2021 12:14, Andrew Cooper wrote:
> On 06/09/2021 12:00, Juergen Gross wrote:
>> In case a domain is created with a cpupool other than Pool-0 specified
>> it will be moved to that cpupool before any vcpus are allocated.
>>
>> This will lead to a NULL pointer dereference in sched_move_domain().
>>
>> Fix that by tolerating vcpus not being allocated yet.
>>
>> Fixes: 70fadc41635b9b6 ("xen/cpupool: support moving domain between cpupools with different granularity")
>> Reported-by: Bertrand Marquis <bertrand.marquis@arm.com>
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  xen/common/sched/core.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
>> index 8d178baf3d..79c9100680 100644
>> --- a/xen/common/sched/core.c
>> +++ b/xen/common/sched/core.c
>> @@ -671,6 +671,10 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
>>  
>>      for ( unit_idx = 0; unit_idx < n_units; unit_idx++ )
>>      {
>> +        /* Special case for move at domain creation time. */
>> +        if ( !d->vcpu[unit_idx * gran] )
>> +            break;
>> +
>>          unit = sched_alloc_unit_mem();
>>          if ( unit )
>>          {
> I think the logic would be clearer if you wrap the entire for loop in if
> ( d->max_vcpus ).

And of course, this is wrong.  Turns out the domain_has_vcpus()
predicate still hasn't been committed, but d->vcpu[0] is the correct
internal.

~Andrew

>   This loop is only allocating units in the new
> scheduler for existing vcpus, so there's no point entering the loop at
> all during domain creation.
>
> Also, this removes a non-speculatively-guarded d->vcpu[] deference.
>
> ~Andrew
>
>

Re: [PATCH] xen/sched: fix sched_move_domain() for domain without vcpus

Posted by Jan Beulich 4 years, 5 months ago

On 06.09.2021 13:18, Andrew Cooper wrote:
> On 06/09/2021 12:14, Andrew Cooper wrote:
>> On 06/09/2021 12:00, Juergen Gross wrote:
>>> In case a domain is created with a cpupool other than Pool-0 specified
>>> it will be moved to that cpupool before any vcpus are allocated.
>>>
>>> This will lead to a NULL pointer dereference in sched_move_domain().
>>>
>>> Fix that by tolerating vcpus not being allocated yet.
>>>
>>> Fixes: 70fadc41635b9b6 ("xen/cpupool: support moving domain between cpupools with different granularity")
>>> Reported-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>>  xen/common/sched/core.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
>>> index 8d178baf3d..79c9100680 100644
>>> --- a/xen/common/sched/core.c
>>> +++ b/xen/common/sched/core.c
>>> @@ -671,6 +671,10 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
>>>  
>>>      for ( unit_idx = 0; unit_idx < n_units; unit_idx++ )
>>>      {
>>> +        /* Special case for move at domain creation time. */
>>> +        if ( !d->vcpu[unit_idx * gran] )
>>> +            break;
>>> +
>>>          unit = sched_alloc_unit_mem();
>>>          if ( unit )
>>>          {
>> I think the logic would be clearer if you wrap the entire for loop in if
>> ( d->max_vcpus ).
> 
> And of course, this is wrong.  Turns out the domain_has_vcpus()
> predicate still hasn't been committed, but d->vcpu[0] is the correct
> internal.

Which in turn might want to be done by setting n_units to zero when
d->vcpus[0] is NULL?

Jan

Re: [PATCH] xen/sched: fix sched_move_domain() for domain without vcpus

Posted by Juergen Gross 4 years, 5 months ago

On 06.09.21 13:23, Jan Beulich wrote:
> On 06.09.2021 13:18, Andrew Cooper wrote:
>> On 06/09/2021 12:14, Andrew Cooper wrote:
>>> On 06/09/2021 12:00, Juergen Gross wrote:
>>>> In case a domain is created with a cpupool other than Pool-0 specified
>>>> it will be moved to that cpupool before any vcpus are allocated.
>>>>
>>>> This will lead to a NULL pointer dereference in sched_move_domain().
>>>>
>>>> Fix that by tolerating vcpus not being allocated yet.
>>>>
>>>> Fixes: 70fadc41635b9b6 ("xen/cpupool: support moving domain between cpupools with different granularity")
>>>> Reported-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>>   xen/common/sched/core.c | 4 ++++
>>>>   1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
>>>> index 8d178baf3d..79c9100680 100644
>>>> --- a/xen/common/sched/core.c
>>>> +++ b/xen/common/sched/core.c
>>>> @@ -671,6 +671,10 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
>>>>   
>>>>       for ( unit_idx = 0; unit_idx < n_units; unit_idx++ )
>>>>       {
>>>> +        /* Special case for move at domain creation time. */
>>>> +        if ( !d->vcpu[unit_idx * gran] )
>>>> +            break;
>>>> +
>>>>           unit = sched_alloc_unit_mem();
>>>>           if ( unit )
>>>>           {
>>> I think the logic would be clearer if you wrap the entire for loop in if
>>> ( d->max_vcpus ).
>>
>> And of course, this is wrong.  Turns out the domain_has_vcpus()
>> predicate still hasn't been committed, but d->vcpu[0] is the correct
>> internal.
> 
> Which in turn might want to be done by setting n_units to zero when
> d->vcpus[0] is NULL?

Yes, this would be possible.

OTOH my variant is more robust in case not all vcpus are allocated,
but I guess this will explode somewhere else anyway.

In case I don't get any other comment today I'll change the patch to set
n_units to 0 if d->vcpus[0] is NULL.


Juergen

Re: [PATCH] xen/sched: fix sched_move_domain() for domain without vcpus

Posted by Juergen Gross 4 years, 5 months ago

On 06.09.21 13:14, Andrew Cooper wrote:
> On 06/09/2021 12:00, Juergen Gross wrote:
>> In case a domain is created with a cpupool other than Pool-0 specified
>> it will be moved to that cpupool before any vcpus are allocated.
>>
>> This will lead to a NULL pointer dereference in sched_move_domain().
>>
>> Fix that by tolerating vcpus not being allocated yet.
>>
>> Fixes: 70fadc41635b9b6 ("xen/cpupool: support moving domain between cpupools with different granularity")
>> Reported-by: Bertrand Marquis <bertrand.marquis@arm.com>
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   xen/common/sched/core.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
>> index 8d178baf3d..79c9100680 100644
>> --- a/xen/common/sched/core.c
>> +++ b/xen/common/sched/core.c
>> @@ -671,6 +671,10 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
>>   
>>       for ( unit_idx = 0; unit_idx < n_units; unit_idx++ )
>>       {
>> +        /* Special case for move at domain creation time. */
>> +        if ( !d->vcpu[unit_idx * gran] )
>> +            break;
>> +
>>           unit = sched_alloc_unit_mem();
>>           if ( unit )
>>           {
> 
> I think the logic would be clearer if you wrap the entire for loop in if
> ( d->max_vcpus ).

No, d->max_vcpus is not 0 here, otherwise n_units would be 0.

> This loop is only allocating units in the new
> scheduler for existing vcpus, so there's no point entering the loop at
> all during domain creation.
> 
> Also, this removes a non-speculatively-guarded d->vcpu[] deference.

I don't think this dereference is a real problem. In case you are
worried about it we should replace the one further below, too.


Juergen