Re: [Xen-devel] [PATCH 2/4] fix XENMEM_add_to_physmap preemption handling

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.12.13 at 18:59, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Wed, 2013-12-18 at 14:35 +0000, Jan Beulich wrote:
>> J
>>          /* Check for continuation if it's not the last interation. */
>> -        if ( xatp->size > 0 && hypercall_preempt_check() )
>> +        if ( xatp->size > ++done && hypercall_preempt_check() )
> 
> Hiding the loop increment inside the preemption checking is a bit subtle
> (it took me a while to find it). Can't it go either before or after this
> loop? With a suitable +/- 1 to the rc below if necessary.

It could, but I find it quite natural to be together with the check.

>> @@ -724,6 +731,12 @@ long do_memory_op(unsigned long cmd, XEN
>>      {
>>          struct xen_add_to_physmap xatp;
>>  
>> +        BUILD_BUG_ON((typeof(xatp.size))-1 > (UINT_MAX >> 
>> MEMOP_EXTENT_SHIFT));
> 
> What does "typeof(xatp.size)-1" evaluate to? typeof can't return an int,
> cant it?

Properly parenthesized this is ((uint16_t)-1), i.e. 0xffff (typeof()
doesn't produce a value, but a type).

>> +
>> +        /* Check for faked input. */
> 
> Faked as in "malicious" or faked as in "something we made up for
> continuation purposes" ?

As in "malicious" (or buggy), i.e. the caller passing an out of range
"cmd" to HYPERVISOR_memory_op. Do you have any better wording
in mind?

Jan


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