Re: [PATCH 03/16] xen: sched: make implementing .alloc_pdata optional

[Juergen Gross <jgross@suse.com>]

On 01/04/16 19:01, Dario Faggioli wrote:
> On Mon, 2016-03-21 at 09:07 -0600, Jan Beulich wrote:
>>>>> On 21.03.16 at 15:48, <JGross@suse.com> wrote:
>>> On 18/03/16 20:04, Dario Faggioli wrote:
>>>> @@ -1491,9 +1491,9 @@ static int cpu_schedule_up(unsigned int
>>>> cpu)
>>>>      if ( idle_vcpu[cpu] == NULL )
>>>>          return -ENOMEM;
>>>>  
>>>> -    if ( (ops.alloc_pdata != NULL) &&
>>>> -         ((sd->sched_priv = ops.alloc_pdata(&ops, cpu)) == NULL)
>>>> )
>>>> -        return -ENOMEM;
>>>> +    sd->sched_priv = SCHED_OP(&ops, alloc_pdata, cpu);
>>>> +    if ( IS_ERR(sd->sched_priv) )
>>>> +        return PTR_ERR(sd->sched_priv);
>>> Calling xfree() with an IS_ERR() value might be a bad idea.
>>> Either you need to set sd->sched_priv to NULL in error case or you
>>> modify xfree() to return immediately not only in the NULL case, but
>>> in the IS_ERR() case as well.
>> The latter option is a no-go imo.
>>
> Ok, I'll do:
> 
>     sd->sched_priv = SCHED_OP(&ops, alloc_pdata, cpu);
>     if ( IS_ERR(sd->sched_priv) )
>     {
>         int err = PTR_ERR(sd->sched_priv);
> 
>         sd->sched_priv = NULL;
>         return err;
>     }
> 
> Is this ok?

Yes, that's what I would prefer.

> And, just to be sure I got your point (Juergen), you're referring to
> the situation where cpu_schedule_up() fails, we go to CPU_UP_CANCELLED,
> which calls cpu_schedule_donw(), which calls free_pdata on
> sd->sched_priv (inside which we may reach an xfree()), aren't you?

Basically, yes.

> In fact, alloc_pdata is also called in schedule_cpu_switch(), but in
> that case, I don't see anyone calling xfree() if alloc_pdata fails...
> Am I missing it?

I just want to avoid the situation where sched_priv would contain a
non-NULL value not pointing to an allocated area. That's always
dangerous, even if with current code nothing bad might happen.

Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel