Xen devel - Oct 2013 - [PATCH 0/2] Fix crashes with arinc653 cpupools

From: Nathan Studer <nate.studer@dornerworks.com>

The ARINC653 scheduler''s vdata_alloc and vdata_free functions
contain a number of errors which cause the hypervisor
to crash when using the ARINC653 scheduler with cpupools.

Nathan Studer (2):
  Fix sched_priv corruption in ARINC653 alloc_vdata.
  Fix NULL pointer dereference in ARINC653 free_vdata.

 xen/common/sched_arinc653.c |   19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

-- 
1.7.9.5

From: Nathan Studer <nate.studer@dornerworks.com>

The ARINC653 scheduler was directly assigning and manipulating
the sched_priv field of a vcpu in its alloc_vdata function.

When creating a cpu pool, this resulted in the corruption
of the sched_priv field of the vcpu, which was then passed
to the initial scheduler''s free_vdata function with
disastrous results.

Signed-off-by: Nathan Studer <nate.studer@dornerworks.com>
---
 xen/common/sched_arinc653.c |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/xen/common/sched_arinc653.c b/xen/common/sched_arinc653.c
index 2502192..a1d9443 100644
--- a/xen/common/sched_arinc653.c
+++ b/xen/common/sched_arinc653.c
@@ -380,11 +380,14 @@ a653sched_deinit(const struct scheduler *ops)
 static void *
 a653sched_alloc_vdata(const struct scheduler *ops, struct vcpu *vc, void *dd)
 {
+    arinc653_vcpu_t *svc;
+
     /* 
      * Allocate memory for the ARINC 653-specific scheduler data information
      * associated with the given VCPU (vc). 
-     */ 
-    if ( (vc->sched_priv = xmalloc(arinc653_vcpu_t)) == NULL )
+     */
+    svc = xmalloc(arinc653_vcpu_t);
+    if ( svc == NULL )
         return NULL;
 
     /*
@@ -393,13 +396,13 @@ a653sched_alloc_vdata(const struct scheduler *ops, struct
vcpu *vc, void *dd)
      * will call the vcpu_wake scheduler callback function and our scheduler 
      * will mark the VCPU awake.
      */
-    AVCPU(vc)->vc = vc;
-    AVCPU(vc)->awake = 0;
+    svc->vc = vc;
+    svc->awake = 0;
     if ( !is_idle_vcpu(vc) )
-        list_add(&AVCPU(vc)->list, &SCHED_PRIV(ops)->vcpu_list);
+        list_add(&svc->list, &SCHED_PRIV(ops)->vcpu_list);
     update_schedule_vcpus(ops);
 
-    return AVCPU(vc);
+    return svc;
 }
 
 /**
-- 
1.7.9.5

From: Nathan Studer <nate.studer@dornerworks.com>

The ARINC653 scheduler alloc_vdata function does not add the
idle cpu to its internal vcpu_list, but when the free_vdata
function is called, the scheduler attempted to remove the vcpu
from its internal vcpu_list, regardless of whether or not
the vcpu was the idle vcpu.  Since the idle vcpu''s list field
was never initialized, a NULL pointer was passed to list_del.

When using cpupools, this resulted in a crash when moving a cpu
from an arinc653 scheduler pool.

Signed-off-by: Nathan Studer <nate.studer@dornerworks.com>
---
 xen/common/sched_arinc653.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/common/sched_arinc653.c b/xen/common/sched_arinc653.c
index a1d9443..8a5bd9c 100644
--- a/xen/common/sched_arinc653.c
+++ b/xen/common/sched_arinc653.c
@@ -418,7 +418,9 @@ a653sched_free_vdata(const struct scheduler *ops, void
*priv)
     if (av == NULL)
         return;
 
-    list_del(&av->list);
+    if ( !is_idle_vcpu(av->vc) )
+        list_del(&av->list);
+
     xfree(av);
     update_schedule_vcpus(ops);
 }
-- 
1.7.9.5

On 31/10/13 20:47, Nathan Studer wrote:
> From: Nathan Studer <nate.studer@dornerworks.com>
>
> The ARINC653 scheduler was directly assigning and manipulating
> the sched_priv field of a vcpu in its alloc_vdata function.
>
> When creating a cpu pool, this resulted in the corruption
> of the sched_priv field of the vcpu, which was then passed
> to the initial scheduler''s free_vdata function with
> disastrous results.
>
> Signed-off-by: Nathan Studer <nate.studer@dornerworks.com>
This looks sane.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

You have one line of misc whitespace change.  Given the extent of
trailing whitespace in the file, it might be worth having a separate
patch in the series which fixes all the whitespace at once.  Along with
that, it would be kind to put a "Local variables" block in as well
(see
the bottom of sched.h as an example).

> ---
>  xen/common/sched_arinc653.c |   15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/xen/common/sched_arinc653.c b/xen/common/sched_arinc653.c
> index 2502192..a1d9443 100644
> --- a/xen/common/sched_arinc653.c
> +++ b/xen/common/sched_arinc653.c
> @@ -380,11 +380,14 @@ a653sched_deinit(const struct scheduler *ops)
>  static void *
>  a653sched_alloc_vdata(const struct scheduler *ops, struct vcpu *vc, void
*dd)
>  {
> +    arinc653_vcpu_t *svc;
> +
>      /* 
>       * Allocate memory for the ARINC 653-specific scheduler data
information
>       * associated with the given VCPU (vc). 
> -     */ 
> -    if ( (vc->sched_priv = xmalloc(arinc653_vcpu_t)) == NULL )
> +     */
> +    svc = xmalloc(arinc653_vcpu_t);
> +    if ( svc == NULL )
>          return NULL;
>  
>      /*
> @@ -393,13 +396,13 @@ a653sched_alloc_vdata(const struct scheduler *ops,
struct vcpu *vc, void *dd)
>       * will call the vcpu_wake scheduler callback function and our
scheduler
>       * will mark the VCPU awake.
>       */
> -    AVCPU(vc)->vc = vc;
> -    AVCPU(vc)->awake = 0;
> +    svc->vc = vc;
> +    svc->awake = 0;
>      if ( !is_idle_vcpu(vc) )
> -        list_add(&AVCPU(vc)->list,
&SCHED_PRIV(ops)->vcpu_list);
> +        list_add(&svc->list, &SCHED_PRIV(ops)->vcpu_list);
>      update_schedule_vcpus(ops);
>  
> -    return AVCPU(vc);
> +    return svc;
>  }
>  
>  /**

On 31/10/13 20:47, Nathan Studer wrote:
> From: Nathan Studer <nate.studer@dornerworks.com>
>
> The ARINC653 scheduler alloc_vdata function does not add the
> idle cpu to its internal vcpu_list, but when the free_vdata
> function is called, the scheduler attempted to remove the vcpu
> from its internal vcpu_list, regardless of whether or not
> the vcpu was the idle vcpu.  Since the idle vcpu''s list field
> was never initialized, a NULL pointer was passed to list_del.
>
> When using cpupools, this resulted in a crash when moving a cpu
> from an arinc653 scheduler pool.
>
> Signed-off-by: Nathan Studer <nate.studer@dornerworks.com>
This again looks sane, but can''t it logically be merged with the
previous patch?  Both of the patches are "dont break on
{alloc,free}_vdata when using cpupools"

I guess this is a matter of taste.

~Andrew
> ---
>  xen/common/sched_arinc653.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/xen/common/sched_arinc653.c b/xen/common/sched_arinc653.c
> index a1d9443..8a5bd9c 100644
> --- a/xen/common/sched_arinc653.c
> +++ b/xen/common/sched_arinc653.c
> @@ -418,7 +418,9 @@ a653sched_free_vdata(const struct scheduler *ops, void
*priv)
>      if (av == NULL)
>          return;
>  
> -    list_del(&av->list);
> +    if ( !is_idle_vcpu(av->vc) )
> +        list_del(&av->list);
> +
>      xfree(av);
>      update_schedule_vcpus(ops);
>  }

On 01/11/13 13:56, Andrew Cooper wrote:
> On 31/10/13 20:47, Nathan Studer wrote:
>> From: Nathan Studer <nate.studer@dornerworks.com>
>>
>> The ARINC653 scheduler alloc_vdata function does not add the
>> idle cpu to its internal vcpu_list, but when the free_vdata
>> function is called, the scheduler attempted to remove the vcpu
>> from its internal vcpu_list, regardless of whether or not
>> the vcpu was the idle vcpu.  Since the idle vcpu''s list field
>> was never initialized, a NULL pointer was passed to list_del.
>>
>> When using cpupools, this resulted in a crash when moving a cpu
>> from an arinc653 scheduler pool.
>>
>> Signed-off-by: Nathan Studer <nate.studer@dornerworks.com>
> This again looks sane, but can''t it logically be merged with the
> previous patch?  Both of the patches are "dont break on
> {alloc,free}_vdata when using cpupools"
>
> I guess this is a matter of taste.
I don''t think it''s worth asking for a whole re-spin just to
merge two
relatively short commits that *could* be merged but also work separately.

Both these patches can have my Ack, BTW (although I don''t think they 
need it): I just wanted to be a good citizen and do a bit of review. :-)

  -George

On 01/11/13 14:13, George Dunlap wrote:
> On 01/11/13 13:56, Andrew Cooper wrote:
>> On 31/10/13 20:47, Nathan Studer wrote:
>>> From: Nathan Studer <nate.studer@dornerworks.com>
>>>
>>> The ARINC653 scheduler alloc_vdata function does not add the
>>> idle cpu to its internal vcpu_list, but when the free_vdata
>>> function is called, the scheduler attempted to remove the vcpu
>>> from its internal vcpu_list, regardless of whether or not
>>> the vcpu was the idle vcpu.  Since the idle vcpu''s list
field
>>> was never initialized, a NULL pointer was passed to list_del.
>>>
>>> When using cpupools, this resulted in a crash when moving a cpu
>>> from an arinc653 scheduler pool.
>>>
>>> Signed-off-by: Nathan Studer <nate.studer@dornerworks.com>
>> This again looks sane, but can''t it logically be merged with
the
>> previous patch?  Both of the patches are "dont break on
>> {alloc,free}_vdata when using cpupools"
>>
>> I guess this is a matter of taste.
>
> I don''t think it''s worth asking for a whole re-spin just
to merge two
> relatively short commits that *could* be merged but also work separately.
>
> Both these patches can have my Ack, BTW (although I don''t think
they
> need it): I just wanted to be a good citizen and do a bit of review. :-)
>
>  -George
>
Fair point - the fixes are far more critical than the problems I
nitpicked at.

FWIW, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

On 11/1/2013 10:13 AM, George Dunlap wrote:
> Both these patches can have my Ack, BTW (although I don''t think
they
> need it): I just wanted to be a good citizen and do a bit of review. :-)
Thanks Andrew and George.

Just to make sure, and to avoid needlessly filling George''s inbox,
Robbie (The
other ARINC653 scheduler maintainer) should usually be the one acking patches
like this, correct?
> 
>   -George
>

On 11/1/2013 9:53 AM, Andrew Cooper wrote:
> On 31/10/13 20:47, Nathan Studer wrote:
>> From: Nathan Studer <nate.studer@dornerworks.com>
>>
>> The ARINC653 scheduler was directly assigning and manipulating
>> the sched_priv field of a vcpu in its alloc_vdata function.
>>
>> When creating a cpu pool, this resulted in the corruption
>> of the sched_priv field of the vcpu, which was then passed
>> to the initial scheduler''s free_vdata function with
>> disastrous results.
>>
>> Signed-off-by: Nathan Studer <nate.studer@dornerworks.com>
> 
> This looks sane.
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> You have one line of misc whitespace change.  Given the extent of
> trailing whitespace in the file, it might be worth having a separate
> patch in the series which fixes all the whitespace at once.  Along with
> that, it would be kind to put a "Local variables" block in as
well (see
> the bottom of sched.h as an example).
These patches just fix the immediate problem of the hypervisor crashing when
using the arinc653 scheduler in cpu pools.  I expect some more patches in the
near future to get pools fully working with the arinc653 scheduler, and since I
agree with your assessment, I can address this then.

Thanks for suggesting it.

On 03/11/13 23:01, Nate Studer wrote:
> On 11/1/2013 10:13 AM, George Dunlap wrote:
>> Both these patches can have my Ack, BTW (although I don''t
think they
>> need it): I just wanted to be a good citizen and do a bit of review.
:-)
> Thanks Andrew and George.
>
> Just to make sure, and to avoid needlessly filling George''s inbox,
Robbie (The
> other ARINC653 scheduler maintainer) should usually be the one acking
patches
> like this, correct?
Oh, don''t worry about filling up my inbox.  I do want to know
what''s
going on in the scheduling area, and I probably *should* review this 
kind of patch anyway.  I was just saying, I don''t think the committers 
necessarily need to wait for an Ack from me to commit it. :-)

I''m not sure exactly what the policy would be here -- I think the
"need
an ack from someone else" rule is primarily for committers to commit 
their own patches.  If you''re just submitting it, as a maintainer, it 
seems like having the committer look over it (and being satisfied that 
the community has had a chance to comment) should be enough, and needing 
an extra Ack from the other committer is a bit over-kill, particularly 
in this case.

But I could be wrong about that. :-)

  -George

On Mon, 2013-11-04 at 10:46 +0000, George Dunlap wrote:
> On 03/11/13 23:01, Nate Studer wrote:
> > On 11/1/2013 10:13 AM, George Dunlap wrote:
> >> Both these patches can have my Ack, BTW (although I don''t
think they
> >> need it): I just wanted to be a good citizen and do a bit of
review. :-)
> > Thanks Andrew and George.
> >
> > Just to make sure, and to avoid needlessly filling George''s
inbox, Robbie (The
> > other ARINC653 scheduler maintainer) should usually be the one acking
patches
> > like this, correct?
> 
> Oh, don''t worry about filling up my inbox.  I do want to know
what''s
> going on in the scheduling area, and I probably *should* review this 
> kind of patch anyway.  I was just saying, I don''t think the
committers
> necessarily need to wait for an Ack from me to commit it. :-)
> 
> I''m not sure exactly what the policy would be here -- I think the
"need
> an ack from someone else" rule is primarily for committers to commit 
> their own patches.  If you''re just submitting it, as a maintainer,
it
> seems like having the committer look over it (and being satisfied that 
> the community has had a chance to comment) should be enough, and needing 
> an extra Ack from the other committer is a bit over-kill, particularly 
> in this case.
If I were a committer in this area (which I''m not) this is the sort of
approach I would take...
> 
> But I could be wrong about that. :-)
> 
>   -George
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

>>> On 04.11.13 at 11:46, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> On 03/11/13 23:01, Nate Studer wrote:
>> Just to make sure, and to avoid needlessly filling George''s
inbox, Robbie (The
>> other ARINC653 scheduler maintainer) should usually be the one acking
patches
>> like this, correct?
> 
> I''m not sure exactly what the policy would be here -- I think the
"need
> an ack from someone else" rule is primarily for committers to commit 
> their own patches.  If you''re just submitting it, as a maintainer,
it
> seems like having the committer look over it (and being satisfied that 
> the community has had a chance to comment) should be enough, and needing 
> an extra Ack from the other committer is a bit over-kill, particularly 
> in this case.
> 
> But I could be wrong about that. :-)
My understanding is the same.

Jan

On 31/10/13 20:47, Nathan Studer wrote:
> From: Nathan Studer <nate.studer@dornerworks.com>
>
> The ARINC653 scheduler was directly assigning and manipulating
> the sched_priv field of a vcpu in its alloc_vdata function.
>
> When creating a cpu pool, this resulted in the corruption
> of the sched_priv field of the vcpu, which was then passed
> to the initial scheduler''s free_vdata function with
> disastrous results.
>
> Signed-off-by: Nathan Studer <nate.studer@dornerworks.com>
Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
>   xen/common/sched_arinc653.c |   15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/xen/common/sched_arinc653.c b/xen/common/sched_arinc653.c
> index 2502192..a1d9443 100644
> --- a/xen/common/sched_arinc653.c
> +++ b/xen/common/sched_arinc653.c
> @@ -380,11 +380,14 @@ a653sched_deinit(const struct scheduler *ops)
>   static void *
>   a653sched_alloc_vdata(const struct scheduler *ops, struct vcpu *vc, void
*dd)
>   {
> +    arinc653_vcpu_t *svc;
> +
>       /*
>        * Allocate memory for the ARINC 653-specific scheduler data
information
>        * associated with the given VCPU (vc).
> -     */
> -    if ( (vc->sched_priv = xmalloc(arinc653_vcpu_t)) == NULL )
> +     */
> +    svc = xmalloc(arinc653_vcpu_t);
> +    if ( svc == NULL )
>           return NULL;
>   
>       /*
> @@ -393,13 +396,13 @@ a653sched_alloc_vdata(const struct scheduler *ops,
struct vcpu *vc, void *dd)
>        * will call the vcpu_wake scheduler callback function and our
scheduler
>        * will mark the VCPU awake.
>        */
> -    AVCPU(vc)->vc = vc;
> -    AVCPU(vc)->awake = 0;
> +    svc->vc = vc;
> +    svc->awake = 0;
>       if ( !is_idle_vcpu(vc) )
> -        list_add(&AVCPU(vc)->list,
&SCHED_PRIV(ops)->vcpu_list);
> +        list_add(&svc->list, &SCHED_PRIV(ops)->vcpu_list);
>       update_schedule_vcpus(ops);
>   
> -    return AVCPU(vc);
> +    return svc;
>   }
>   
>   /**