Re: [RFC Patch v4 5/9] check if mfn is supported by IOCTL_PRIVCMD_MMAPBATCH before calling ioctl()

[Wen Congyang <wency@cn.fujitsu.com>]

On 09/22/2014 10:26 PM, Ian Campbell wrote:
> On Mon, 2014-09-22 at 13:59 +0800, Wen Congyang wrote:
>> If mfn is invalid, ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH, ..) also returns 0,
>> and set error information in error bits(bits28-31). So if the user input
>> a large valid mfn, we cannot reliably distinguish between a large MFN and
>> an error. So we should check the input mfn before calling ioctl().
>> The user can input more than one mfn, and part of them are ~0UL. In this
>> case, the user expects we can map the memory for all valid mfn. So we
>> cannot just return NULL if some mfn is not supported.
> 
> I don't follow this last bit. linux_privcmd_map_foreign_bulk already
> returns NULL and maps nothing in some error cases (e.g. mmap failure),
> what is the problem also doing that here?

Yes, if mmap fails, linux_privcmd_map_foreign_bulk() returns NULL.
But some mfn is invalid, ioctl() returns 0, and then linux_privcmd_map_foreign_bulk()
doesn't return NULL.

For example:
page0, page1, ... page m, page m+1, ... page n
mfn 0, mfn 1, ... mfn m,  mfn m+1, ... mfn n

If only mfn m is invalid, the user can access page 0, page 1, page m+1.
The user can know which page can't be accessed by the array err[].

If some mfn is valid, but it is large, and IOCTL_PRIVCMD_MMAPBATCH doesn't
support it. The user doesn't know the page can't be accessed, and will
cause page fault(the user program may segment fault) when the user accesses the page.

> 
> The way you have it here we need to worry about what the behaviour of
> Xen/privcmd is on pfn==~0UL, and whether we can rely on it. Far better
> to just declare such attempts to be fundamentally broken on the part of
> the caller and return.

In the function apply_batch():
        /* setup region_mfn[] for batch map, if necessary.
         * For HVM guests, this interface takes PFNs, not MFNs */
        if ( pagetype == XEN_DOMCTL_PFINFO_XTAB
             || pagetype == XEN_DOMCTL_PFINFO_XALLOC )
            region_mfn[i] = ~0UL; /* map will fail but we don't care */
        else
            region_mfn[i] = ctx->hvm ? pfn : ctx->p2m[pfn];
It is why I choose ~0UL. I don't know how to check if the mfn is valid,
and we should allow the caller passes ~0UL, otherwise, it will break
migration.

Thanks
Wen Congyang

> 
> Ian.
> 
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> ---
>>  tools/libxc/xc_linux_osdep.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c
>> index a19e4b6..d11bcee 100644
>> --- a/tools/libxc/xc_linux_osdep.c
>> +++ b/tools/libxc/xc_linux_osdep.c
>> @@ -321,6 +321,18 @@ static void *linux_privcmd_map_foreign_bulk(xc_interface *xch, xc_osdep_handle h
>>          }
>>  
>>          memcpy(pfn, arr, num * sizeof(*arr));
>> +        for ( i = 0; i < num; i++ )
>> +        {
>> +            /*
>> +             * IOCTL_PRIVCMD_MMAPBATCH doesn't support the mfn which
>> +             * error bits are set
>> +             */
>> +            if ( pfn[i] & PRIVCMD_MMAPBATCH_MFN_ERROR )
>> +            {
>> +                pfn[i] = ~0UL;
>> +                err[i] = -EINVAL;
>> +            }
>> +        }
>>  
>>          ioctlx.num = num;
>>          ioctlx.dom = dom;
>> @@ -333,6 +345,9 @@ static void *linux_privcmd_map_foreign_bulk(xc_interface *xch, xc_osdep_handle h
>>  
>>          for ( i = 0; i < num; ++i )
>>          {
>> +            if ( pfn[i] == ~0UL )
>> +                continue;
>> +
>>              switch ( pfn[i] ^ arr[i] )
>>              {
>>              case 0:
> 
> 
> .
>