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

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

On 09/24/2014 02:25 PM, Meng Xu wrote:
> 2014-09-24 9:14 GMT-04:00 Dario Faggioli <dario.faggioli@xxxxxxxxxx>:
> > On mer, 2014-09-24 at 13:09 +0100, Jan Beulich wrote:
> > > > > > On 21.09.14 at 00:13, <mengxu@xxxxxxxxxxxxx> 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

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