Re: [PATCH for 4.5 v4 1/4] xen: add real time scheduler rtds

[George Dunlap <george.dunlap@eu.citrix.com>]

On 09/24/2014 02:25 PM, Meng Xu wrote:
> 2014-09-24 9:14 GMT-04:00 Dario Faggioli <dario.faggioli@citrix.com>:
>> On mer, 2014-09-24 at 13:09 +0100, Jan Beulich wrote:
>>>>>> On 21.09.14 at 00:13, <mengxu@cis.upenn.edu> wrote:
>>> *** CID 1240234:  Division or modulo by zero  (DIVIDE_BY_ZERO)
>>> /xen/common/sched_rt.c: 327 in rt_update_deadline()
>>> 321             do
>>> 322                 svc->cur_deadline += svc->period;
>>> 323             while ( svc->cur_deadline <= now );
>>> 324         }
>>> 325         else
>>> 326         {
>>>>>>      CID 1240234:  Division or modulo by zero  (DIVIDE_BY_ZERO)
>>>>>>      In expression "(now - svc->cur_deadline) / svc->period", division by expression "svc->period" which may be zero has undefined behavior.
>>> 327             long count = ((now - svc->cur_deadline) / svc->period) + 1;
>>> 328             svc->cur_deadline += count * svc->period;
>>> 329         }
>>> 330
>>> 331         svc->cur_budget = svc->budget;
>>>
>>> with
>>>
>>>      ASSERT(svc->period != 0);
>>>
>>> a few lines up. However, the ASSERT() itself is currently invalid
>>> because above code doesn't make sure zero wouldn't get stored
>>> into that field. I.e. the ASSERT() currently (indirectly) verifies
>>> valid caller input rather than valid hypervisor state.
>>>
>> Right, good point. Sorry I missed this while reviewing the series.
>>
>>> This needs
>>> to be fixed. And I would have sent a patch right away if it wasn't
>>> unclear to me whether op->u.rtds.budget should also be checked
>>> against zero (under the assumption that all other values are valid
>>> for these two fields).
>>>
>> I'd go for doing the sanity checking in rt_dom_cntl(), SCHEDOP_putinfo
>> branch, of course... Meng, do you agree? If yes, can you send a patch to
>> that effect?
> Sure! We need do sanity check for this, although the toolstack does
> not allow zero value for period and budget of a vcpu. When I do the
> sanity check, I will return EINVAL if period or budget is zero.

Yes, sorry I missed this -- the hypervisor always needs to do all of its 
own checking (which is why I initially said that it was redundant to do 
checking in libxl).

  -George