From mboxrd@z Thu Jan  1 00:00:00 1970
From: George Dunlap <george.dunlap@eu.citrix.com>
Subject: Re: [PATCH for 4.5 v4 1/4] xen: add real time scheduler
	rtds
Date: Wed, 24 Sep 2014 14:33:43 +0100
Message-ID: <5422C837.4040104@eu.citrix.com>
References: <1411251228-2093-1-git-send-email-mengxu@cis.upenn.edu>	<5422D0A70200007800038343@mail.emea.novell.com>	<1411564495.4336.11.camel@Solace.lan>
	<CAENZ-+nVij=KaninAGarikAV8NWr8MDkiGjHpy01o-kVg=Q2AQ@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <CAENZ-+nVij=KaninAGarikAV8NWr8MDkiGjHpy01o-kVg=Q2AQ@mail.gmail.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Meng Xu <xumengpanda@gmail.com>, Dario Faggioli <dario.faggioli@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>, Sisu Xi <xisisu@gmail.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>, Chenyang Lu <lu@cse.wustl.edu>, Ian Jackson <ian.jackson@eu.citrix.com>, "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>, Linh Thi Xuan Phan <ptxlinh@gmail.com>, Meng Xu <mengxu@cis.upenn.edu>, Jan Beulich <JBeulich@suse.com>, Chao Wang <chaowang@wustl.edu>, Chong Li <lichong659@gmail.com>, Dagaen Golomb <dgolomb@seas.upenn.edu>
List-Id: xen-devel@lists.xenproject.org

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