Andres Lagar-Cavilla
2012-Nov-16 15:37 UTC
Re: [PATCH] Correctly return success from IOCTL_PRIVCMD_MMAPBATCH
> This is a regression introduced by ceb90fa0 (xen/privcmd: add > PRIVCMD_MMAPBATCH_V2 ioctl). It broke xentrace as it used > xc_map_foreign() instead of xc_map_foreign_bulk(). > > Most code-paths prefer the MMAPBATCH_V2, so this wasn''t very obvious > that it broke. The return value is set early on to -EINVAL, and if all > goes well, the "set top bits of the MFN''s" never gets called, so the > return value is still EINVAL when the function gets to the end, causing > the caller to think it went wrong (which it didn''t!) > > Signed off by: Mats Petersson <mats.petersson@citrix.com> > Acked-by: David Vrabel <david.vrabel@citrix.com>Uggh. What a complicated API. Good catch, thanks. Now, isn''t this a simpler fix? (and by only changing ret to non-zero in error paths, less prone to allow for inadvertent errors in the future) If this is preferred I can prepare a proper patch. Andres diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c index ef63895..4a6bcb2 100644 --- a/drivers/xen/privcmd.c +++ b/drivers/xen/privcmd.c @@ -361,13 +361,13 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version) down_write(&mm->mmap_sem); vma = find_vma(mm, m.addr); - ret = -EINVAL; if (!vma || vma->vm_ops != &privcmd_vm_ops || (m.addr != vma->vm_start) || ((m.addr + (nr_pages << PAGE_SHIFT)) != vma->vm_end) || !privcmd_enforce_singleshot_mapping(vma)) { up_write(&mm->mmap_sem); + ret = -EINVAL; goto out; }> > --- > drivers/xen/privcmd.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c > index 8adb9cc..24aec2f 100644 > --- a/drivers/xen/privcmd.c > +++ b/drivers/xen/privcmd.c > @@ -383,12 +383,16 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version) > > up_write(&mm->mmap_sem); > > - if (state.global_error && (version == 1)) { > - /* Write back errors in second pass. */ > - state.user_mfn = (xen_pfn_t *)m.arr; > - state.err = err_array; > - ret = traverse_pages(m.num, sizeof(xen_pfn_t), > - &pagelist, mmap_return_errors_v1, &state); > + if (version == 1) { > + if (state.global_error) { > + /* Write back errors in second pass. */ > + state.user_mfn = (xen_pfn_t *)m.arr; > + state.err = err_array; > + ret = traverse_pages(m.num, sizeof(xen_pfn_t), > + &pagelist, mmap_return_errors_v1, &state); > + } else > + ret = 0; > + > } else if (version == 2) { > ret = __copy_to_user(m.err, err_array, m.num * sizeof(int)); > if (ret) > -- > 1.7.9.5
David Vrabel
2012-Nov-16 15:43 UTC
Re: [PATCH] Correctly return success from IOCTL_PRIVCMD_MMAPBATCH
On 16/11/12 15:37, Andres Lagar-Cavilla wrote:>> This is a regression introduced by ceb90fa0 (xen/privcmd: add >> PRIVCMD_MMAPBATCH_V2 ioctl). It broke xentrace as it used >> xc_map_foreign() instead of xc_map_foreign_bulk(). >> >> Most code-paths prefer the MMAPBATCH_V2, so this wasn''t very obvious >> that it broke. The return value is set early on to -EINVAL, and if all >> goes well, the "set top bits of the MFN''s" never gets called, so the >> return value is still EINVAL when the function gets to the end, causing >> the caller to think it went wrong (which it didn''t!) >> >> Signed off by: Mats Petersson <mats.petersson@citrix.com> >> Acked-by: David Vrabel <david.vrabel@citrix.com> > > Uggh. What a complicated API. Good catch, thanks. > > Now, isn''t this a simpler fix? (and by only changing ret to non-zero > in error paths, less prone to allow for inadvertent errors in the future)I had considered this, but I think Mats patch is clearer overall as it makes the v1 and the v2 paths more similar. David> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c > index ef63895..4a6bcb2 100644 > --- a/drivers/xen/privcmd.c > +++ b/drivers/xen/privcmd.c > @@ -361,13 +361,13 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version) > down_write(&mm->mmap_sem); > > vma = find_vma(mm, m.addr); > - ret = -EINVAL; > if (!vma || > vma->vm_ops != &privcmd_vm_ops || > (m.addr != vma->vm_start) || > ((m.addr + (nr_pages << PAGE_SHIFT)) != vma->vm_end) || > !privcmd_enforce_singleshot_mapping(vma)) { > up_write(&mm->mmap_sem); > + ret = -EINVAL; > goto out; > } > > >> >> --- >> drivers/xen/privcmd.c | 16 ++++++++++------ >> 1 file changed, 10 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c >> index 8adb9cc..24aec2f 100644 >> --- a/drivers/xen/privcmd.c >> +++ b/drivers/xen/privcmd.c >> @@ -383,12 +383,16 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version) >> >> up_write(&mm->mmap_sem); >> >> - if (state.global_error && (version == 1)) { >> - /* Write back errors in second pass. */ >> - state.user_mfn = (xen_pfn_t *)m.arr; >> - state.err = err_array; >> - ret = traverse_pages(m.num, sizeof(xen_pfn_t), >> - &pagelist, mmap_return_errors_v1, &state); >> + if (version == 1) { >> + if (state.global_error) { >> + /* Write back errors in second pass. */ >> + state.user_mfn = (xen_pfn_t *)m.arr; >> + state.err = err_array; >> + ret = traverse_pages(m.num, sizeof(xen_pfn_t), >> + &pagelist, mmap_return_errors_v1, &state); >> + } else >> + ret = 0; >> + >> } else if (version == 2) { >> ret = __copy_to_user(m.err, err_array, m.num * sizeof(int)); >> if (ret) >> -- >> 1.7.9.5 >
Andres Lagar-Cavilla
2012-Nov-16 16:00 UTC
Re: [PATCH] Correctly return success from IOCTL_PRIVCMD_MMAPBATCH
On Nov 16, 2012, at 10:43 AM, David Vrabel <david.vrabel@citrix.com> wrote:> On 16/11/12 15:37, Andres Lagar-Cavilla wrote: >>> This is a regression introduced by ceb90fa0 (xen/privcmd: add >>> PRIVCMD_MMAPBATCH_V2 ioctl). It broke xentrace as it used >>> xc_map_foreign() instead of xc_map_foreign_bulk(). >>> >>> Most code-paths prefer the MMAPBATCH_V2, so this wasn''t very obvious >>> that it broke. The return value is set early on to -EINVAL, and if all >>> goes well, the "set top bits of the MFN''s" never gets called, so the >>> return value is still EINVAL when the function gets to the end, causing >>> the caller to think it went wrong (which it didn''t!) >>> >>> Signed off by: Mats Petersson <mats.petersson@citrix.com> >>> Acked-by: David Vrabel <david.vrabel@citrix.com> >> >> Uggh. What a complicated API. Good catch, thanks. >> >> Now, isn''t this a simpler fix? (and by only changing ret to non-zero >> in error paths, less prone to allow for inadvertent errors in the future) > > I had considered this, but I think Mats patch is clearer overall as it > makes the v1 and the v2 paths more similar.You mean the code structure becoming similar to a switch (version) { … }, instead of collapsing multiple conditions in the first if? Ok, I guess. Both patches are fine, I acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org> Mats''s patch, and they should be merged imho to make the logic clearer. Andres> > David > >> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c >> index ef63895..4a6bcb2 100644 >> --- a/drivers/xen/privcmd.c >> +++ b/drivers/xen/privcmd.c >> @@ -361,13 +361,13 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version) >> down_write(&mm->mmap_sem); >> >> vma = find_vma(mm, m.addr); >> - ret = -EINVAL; >> if (!vma || >> vma->vm_ops != &privcmd_vm_ops || >> (m.addr != vma->vm_start) || >> ((m.addr + (nr_pages << PAGE_SHIFT)) != vma->vm_end) || >> !privcmd_enforce_singleshot_mapping(vma)) { >> up_write(&mm->mmap_sem); >> + ret = -EINVAL; >> goto out; >> } >> >> >>> >>> --- >>> drivers/xen/privcmd.c | 16 ++++++++++------ >>> 1 file changed, 10 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c >>> index 8adb9cc..24aec2f 100644 >>> --- a/drivers/xen/privcmd.c >>> +++ b/drivers/xen/privcmd.c >>> @@ -383,12 +383,16 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version) >>> >>> up_write(&mm->mmap_sem); >>> >>> - if (state.global_error && (version == 1)) { >>> - /* Write back errors in second pass. */ >>> - state.user_mfn = (xen_pfn_t *)m.arr; >>> - state.err = err_array; >>> - ret = traverse_pages(m.num, sizeof(xen_pfn_t), >>> - &pagelist, mmap_return_errors_v1, &state); >>> + if (version == 1) { >>> + if (state.global_error) { >>> + /* Write back errors in second pass. */ >>> + state.user_mfn = (xen_pfn_t *)m.arr; >>> + state.err = err_array; >>> + ret = traverse_pages(m.num, sizeof(xen_pfn_t), >>> + &pagelist, mmap_return_errors_v1, &state); >>> + } else >>> + ret = 0; >>> + >>> } else if (version == 2) { >>> ret = __copy_to_user(m.err, err_array, m.num * sizeof(int)); >>> if (ret) >>> -- >>> 1.7.9.5 >> >