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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2013-12-20 at 07:52 +0000, Jan Beulich wrote:
> >>> 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.

If it were the loop termination check or something then I would agree,
but this is effectively some other arbitrary check within the loop (even
if it does happen to exit, it's not the primary loop termination
condition).

> >> @@ -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 

Ah! Yes, I misread that rather badly.

> (typeof() doesn't produce a value, but a type).

I know, that's why I was so confused when it looked like it was being
treated as a value!

> >> +
> >> +        /* 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?

"Check for malicious or buggy input" would do the job. (with or without
"or buggy")

Ian.


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