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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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).

Ian.


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