Re: [PATCH 2/2] xen/privcmd: add PRIVCMD_MMAPBATCH_V2 ioctl

[David Vrabel <david.vrabel@citrix.com>]

On 30/08/12 17:41, Andres Lagar-Cavilla wrote:
> David,
> The patch looks functionally ok, but I still have two lingering concerns:
> - the hideous casting of mfn into err

I considered couple of other approaches (unions, extending
gather_array() to add gaps for the int return). They were all worse.

I also tried your proposal here but it doesn't work. See below.

> - why not signal paged out frames for V1

Because V1 is broken on 64bit and there doesn't seem to be any point in
changing it given that libxc won't call it if V2 is present,

> Rather than keep writing English, I wrote some C :)
> 
> And took the liberty to include your signed-off. David & Konrad, let
> me know what you think, and once we settle on either version we can move
> into unit testing this.
[...]
>  static int mmap_batch_fn(void *data, void *state)
>  {
>         xen_pfn_t *mfnp = data;
>         struct mmap_batch_state *st = state;
> +       int ret;
> +
> +       ret = xen_remap_domain_mfn_range(st->vma, st->va & PAGE_MASK, *mfnp, 1,
> +                                        st->vma->vm_page_prot, st->domain);
> +       if (ret < 0) {
> +               /*
> +                * V2 provides a user-space (pre-checked for access) user_err
> +                * pointer, in which we store the individual map error codes.
> +                *
> +                * V1 encodes the error codes in the 32bit top nibble of the
> +                * mfn (with its known limitations vis-a-vis 64 bit callers).
> +                *
> +                * In either case, global state.err is zero unless one or more
> +                * individual maps fail with -ENOENT, in which case it is -ENOENT.
> +                *
> +                */
> +               if (st->user_err)
> +                       BUG_ON(__put_user(ret, st->user_err++));

You can't access user space pages here while holding
current->mm->mmap_sem.  I tried this and it would sometimes deadlock in
the page fault handler.

access_ok() only checks if the pointer is in the user space virtual
address space - not that a valid mapping exists and is writable.  So
BUG_ON(__put_user()) should not be done.

David