Re: [PATCH 1/2] Fix sched_priv corruption in ARINC653 alloc_vdata.

[Andrew Cooper <andrew.cooper3@citrix.com>]

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;
>  }
>  
>  /**