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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.09.14 at 00:13, <mengxu@xxxxxxxxxxxxx> wrote:
> +static int
> +rt_dom_cntl(
> +    const struct scheduler *ops,
> +    struct domain *d,
> +    struct xen_domctl_scheduler_op *op)
> +{
> +    struct rt_dom * const sdom = rt_dom(d);
> +    struct rt_vcpu *svc;
> +    struct list_head *iter;
> +    int rc = 0;
> +
> +    switch ( op->cmd )
> +    {
> +    case XEN_DOMCTL_SCHEDOP_getinfo:
> +        svc = list_entry(sdom->vcpu.next, struct rt_vcpu, sdom_elem);
> +        op->u.rtds.period = svc->period / MICROSECS(1); /* transfer to us */
> +        op->u.rtds.budget = svc->budget / MICROSECS(1);
> +        break;
> +    case XEN_DOMCTL_SCHEDOP_putinfo:
> +        list_for_each( iter, &sdom->vcpu )
> +        {
> +            struct rt_vcpu * svc = list_entry(iter, struct rt_vcpu, 
> sdom_elem);
> +            svc->period = MICROSECS(op->u.rtds.period); /* transfer to 
> nanosec */
> +            svc->budget = MICROSECS(op->u.rtds.budget);

So Coverity's bug inverting the sense of an ASSERT() produced this

*** 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. 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).

Furthermore the function appears to lack serialization with other
scheduler operations.

Jan



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