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

[Andres Lagar-Cavilla <andreslc@xxxxxxxxxxxxxx>]

On Aug 30, 2012, at 1:04 PM, David Vrabel wrote:

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

Very true. Thanks for the pointer. Clearly the reason for the 
gather_array/traverse_pages structure.
Re-posting my version
Andres
> 
> David


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