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

[Andres Lagar-Cavilla <andreslc@xxxxxxxxxxxxxx>]

On Aug 31, 2012, at 9:08 AM, David Vrabel wrote:

> On 30/08/12 19:32, Andres Lagar-Cavilla wrote:
>> Second repost of my version, heavily based on David's. 
> 
> Doing another allocation that may be larger than a page (and its
> associated additional error paths) seems to me to be a hammer to crack
> the (admittedly bit wonky) casting nut.
> 
> That said, it's up to Konrad which version he prefers.

Yeah absolutely.

> 
> There are also some minor improvements you could make if you respin this
> patch.
> 
>> Complementary to this patch, on the xen tree I intend to add
>> PRIVCMD_MMAPBATCH_*_ERROR into the libxc header files, and remove
>> XEN_DOMCTL_PFINFO_PAGEDTAB from domctl.h
> 
> Yes, a good idea.  There's no correspondence between the ioctl's error
> reporting values and the DOMCTL_PFINFO flags.
> 
>> commit 3f40e8d79b7e032527ee207a97499ddbc81ca12b
>> Author: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
>> Date:   Thu Aug 30 12:23:33 2012 -0400
>> 
>>    xen/privcmd: add PRIVCMD_MMAPBATCH_V2 ioctl
>> 
>>    PRIVCMD_MMAPBATCH_V2 extends PRIVCMD_MMAPBATCH with an additional
>>    field for reporting the error code for every frame that could not be
>>    mapped.  libxc prefers PRIVCMD_MMAPBATCH_V2 over PRIVCMD_MMAPBATCH.
>> 
>>    Also expand PRIVCMD_MMAPBATCH to return appropriate error-encoding top 
>> nibble
>>    in the mfn array.
>> 
>>    Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>
>>    Signed-off-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
> [...]
>> -static long privcmd_ioctl_mmap_batch(void __user *udata)
>> +static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
>> {
>>      int ret;
>> -    struct privcmd_mmapbatch m;
>> +    struct privcmd_mmapbatch_v2 m;
>>      struct mm_struct *mm = current->mm;
>>      struct vm_area_struct *vma;
>>      unsigned long nr_pages;
>>      LIST_HEAD(pagelist);
>> +    int *err_array;
> 
> int *err_array = NULL;
> 
> and you could avoid the additional jump label as kfree(NULL) is safe.

Didn't know, great.

> 
>>      struct mmap_batch_state state;
>> 
>>      if (!xen_initial_domain())
>>              return -EPERM;
>> 
>> -    if (copy_from_user(&m, udata, sizeof(m)))
>> -            return -EFAULT;
>> +    switch (version) {
>> +    case 1:
>> +            if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch)))
>> +                    return -EFAULT;
>> +            /* Returns per-frame error in m.arr. */
>> +            m.err = NULL;
>> +            if (!access_ok(VERIFY_WRITE, m.arr, m.num * sizeof(*m.arr)))
>> +                    return -EFAULT;
>> +            break;
>> +    case 2:
>> +            if (copy_from_user(&m, udata, sizeof(struct 
>> privcmd_mmapbatch_v2)))
>> +                    return -EFAULT;
>> +            /* Returns per-frame error code in m.err. */
>> +            if (!access_ok(VERIFY_WRITE, m.err, m.num * (sizeof(*m.err))))
>> +                    return -EFAULT;
>> +            break;
>> +    default:
>> +            return -EINVAL;
>> +    }
>> 
>>      nr_pages = m.num;
>>      if ((m.num <= 0) || (nr_pages > (LONG_MAX >> PAGE_SHIFT)))
>>              return -EINVAL;
>> 
>> -    ret = gather_array(&pagelist, m.num, sizeof(xen_pfn_t),
>> -                       m.arr);
>> +    ret = gather_array(&pagelist, m.num, sizeof(xen_pfn_t), m.arr);
>> 
>>      if (ret || list_empty(&pagelist))
>> -            goto out;
>> +            goto out_no_err_array;
>> +
>> +    err_array = kmalloc_array(m.num, sizeof(int), GFP_KERNEL);
> 
> kcalloc() (see below).
> 
>> +    if (err_array == NULL)
>> +    {
> 
> Style: if (err_array == NULL) {
> 
>> +    if (state.global_error) {
>> +            int efault;
>> +
>> +            if (state.global_error == -ENOENT)
>> +                    ret = -ENOENT;
>> +
>> +            /* Write back errors in second pass. */
>> +            state.user_mfn = (xen_pfn_t *)m.arr;
>> +            state.user_err = m.err;
>> +            state.err      = err_array;
>> +            efault = traverse_pages(m.num, sizeof(xen_pfn_t),
>> +                                     &pagelist, mmap_return_errors, &state);
>> +            if (efault)
>> +                    ret = efault;
>> +    } else if (m.err)
>> +            ret = __clear_user(m.err, m.num * sizeof(*m.err));
> 
> Since you have an array of errors already there's no need to iterate
> through all the MFNs again for V2.  A simple copy_to_user() is
> sufficient provided err_array was zeroed with kcalloc().
I can kcalloc for extra paranoia, but the code will set all error slots, and is 
guaranteed to not jump over anything. I +1 the shortcut you outline below. 
Re-spin coming.
Andres
> 
> if (m.err)
>    ret = __copy_to_user(m.err, err_array, m.num * sizeof(*m.err));
> else {
>    /* Write back errors in second pass. */
>    state.user_mfn = (xen_pfn_t *)m.arr;
>    state.user_err = m.err;
>    state.err      = err_array;
>    ret = traverse_pages(m.num, sizeof(xen_pfn_t),
>            &pagelist, mmap_return_errors, &state);
> }
> 
> if (ret == 0 && state.global_error == -ENOENT)
>    ret = -ENOENT;
> 
> David


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