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/23/2014 06:08 PM, Ian Campbell wrote:
> On Tue, 2014-09-23 at 09:40 +0800, Wen Congyang wrote:
>> 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.
> 
> The caller of the mapping function must already handle a NULL return by
> erroring out.
> 
>> 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[].
> 
> Not for these problematic MFNs they can't, because err cannot be
> trusted.
> 
>> 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.
> 
> If the mfn is valid but is large enough to have the top nibble (the
> "error nibble") set then there is no way to do proper error reporting,
> because the mfn and the error code will get intermixed, so you can't
> even tell success from failure.
> 
> 
>> 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.
> 
> You don't need to know if it is valid, just that the error-nibble isn't
> set. I'd be OK with special casing ~0UL as a way for the caller to
> indicate that they don't want a mapping at a given index (~0UL is
> already called INVALID_MFN elsewhere) and that they are prepared to deal
> with that case explicitly.
> 
> That's a different case from the caller supplying an MFN which they
> think is good but which is actually bad because the error-nibble has set
> bits in it. The problem with that sort of bad MFN is that the caller
> doesn't know they are bad and will expect the mapping to succeed, either
> now or on a subsequent iteration, but these MFNs can *never* be mapped
> (using this interface). This is IMHO a fairly catastrophic failure and
> it should be treated as such, by returning NULL and requiring the caller
> to deal with it as it would any other NULL return (by failing
> immediately).

OK. I will update this patch soon

Thanks
Wen Congyang

> 
> Ian.
> 
> .
>