Re: [PATCH] [RFC] x86/domctl: Fix getpageframeinfo* handling.

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 18/05/15 12:43, Jan Beulich wrote:
>>>> On 18.05.15 at 12:59, <andrew.cooper3@citrix.com> wrote:
>> In tree, there is one single caller of XEN_DOMCTL_getpageframeinfo3
>> (xc_get_pfn_type_batch()), and no callers of the older variants.
>>
>> getpageframeinfo3 and getpageframeinfo2 are compatible if the parameter
>> contents are considered to be unsigned long; a compat guest calling
>> getpageframeinfo3 falls through into the getpageframeinfo2 handler.
>>
>> However, getpageframeinfo3 and getpageframeinfo2 have different algorithms 
>> for
>> calculating the eventual frame type, which means that a toolstack will get
>> different answers depending on whether it is compat or not, which is a 
>> problem
>> for all possible uses.
> Is there any other difference besides the former being capable of
> returning XEN_DOMCTL_PFINFO_BROKEN (which I would suppose to
> have been a later addition that didn't get properly sync-ed to the
> older handlers)?

That was the only difference I could spot, but any difference is a problem.

>
>> @@ -378,6 +147,81 @@ long arch_do_domctl(
>>          break;
>>      }
>>  
>> +    case XEN_DOMCTL_getpageframeinfo3:
>> +    {
>> +        unsigned int num = domctl->u.getpageframeinfo3.num;
>> +
>> +        /* Games to allow this code block to handle a compat guest. */
>> +        void * __user guest_handle = domctl->u.getpageframeinfo3.array.p;
> The __used belongs between "void" and "*".
>
> Also the blank line above looks somewhat misplaced (I guess you
> added it to kind of emphasize the comment).

I think it ended up like this more by accident, but the emphasis is
important.  I will shuffle the width up.

>
>> +        unsigned int width = has_32bit_shinfo(currd) ? 32 : 64;
> These are bit counts, yet where you use the value you want byte
> granularity.

So they are.  I am surprised that this didn't blow up.

> 32bit unsigned long into it
>> +        if ( unlikely(num > 1024) ||
>> +             unlikely(num != domctl->u.getpageframeinfo3.num) )
>> +        {
>> +            ret = -E2BIG;
>> +            break;
>> +        }
>> +
>> +        for ( i = 0; i < num; ++i )
>> +        {
>> +            unsigned long gfn = 0, type = 0;
> gfn's initializer looks pointless (and if anything it should be INVALID_MFN
> or some such).

It must absolutely be 0 for when we read a 32bit values into it,
although I realise I do need to extend ~0U to ~0UL for compat guests.

>
>> +            struct page_info *page;
>> +            p2m_type_t t;
>> +
>> +            if ( raw_copy_from_guest(&gfn, guest_handle + (i * width), width) )
>> +            {
>> +                ret = -EFAULT;
>> +                break;
>> +            }
>> +
>> +            page = get_page_from_gfn(d, gfn, &t, P2M_ALLOC);
>> +
>> +            if ( unlikely(!page) ||
>> +                 unlikely(is_xen_heap_page(page)) )
>> +            {
>> +                if ( p2m_is_broken(t) )
>> +                    type = XEN_DOMCTL_PFINFO_BROKEN;
>> +                else
>> +                    type = XEN_DOMCTL_PFINFO_XTAB;
> Realizing that this was this way in the old code too, would you
> nevertheless mind adding unlikely() and/or flip the if and else
> branches?

Certainly.

Does this mean that you are happy in principle with the raw_* use?

~Andrew