* Re: [PATCH] Correctly return success from IOCTL_PRIVCMD_MMAPBATCH [not found] <mailman.16724.1353079390.1399.xen-devel@lists.xen.org> @ 2012-11-16 15:37 ` Andres Lagar-Cavilla 2012-11-16 15:43 ` David Vrabel 2012-11-16 18:36 ` [PATCH V3] " Mats Petersson 1 sibling, 1 reply; 5+ messages in thread From: Andres Lagar-Cavilla @ 2012-11-16 15:37 UTC (permalink / raw) To: xen-devel; +Cc: Mats Petersson, David Vrabel, Konrad Wilk > 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Correctly return success from IOCTL_PRIVCMD_MMAPBATCH 2012-11-16 15:37 ` [PATCH] Correctly return success from IOCTL_PRIVCMD_MMAPBATCH Andres Lagar-Cavilla @ 2012-11-16 15:43 ` David Vrabel 2012-11-16 16:00 ` Andres Lagar-Cavilla 0 siblings, 1 reply; 5+ messages in thread From: David Vrabel @ 2012-11-16 15:43 UTC (permalink / raw) To: Andres Lagar-Cavilla; +Cc: Mats Petersson, Konrad Wilk, xen-devel@lists.xen.org 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Correctly return success from IOCTL_PRIVCMD_MMAPBATCH 2012-11-16 15:43 ` David Vrabel @ 2012-11-16 16:00 ` Andres Lagar-Cavilla 0 siblings, 0 replies; 5+ messages in thread From: Andres Lagar-Cavilla @ 2012-11-16 16:00 UTC (permalink / raw) To: David Vrabel; +Cc: Mats Petersson, Konrad Wilk, xen-devel@lists.xen.org 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 >> > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH V3] Correctly return success from IOCTL_PRIVCMD_MMAPBATCH [not found] <mailman.16724.1353079390.1399.xen-devel@lists.xen.org> 2012-11-16 15:37 ` [PATCH] Correctly return success from IOCTL_PRIVCMD_MMAPBATCH Andres Lagar-Cavilla @ 2012-11-16 18:36 ` Mats Petersson 2012-11-19 11:52 ` David Vrabel 1 sibling, 1 reply; 5+ messages in thread From: Mats Petersson @ 2012-11-16 18:36 UTC (permalink / raw) To: xen-devel; +Cc: Mats Petersson, konrad.wilk 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!) Now also including Andres "move the ret = -EINVAL into the error handling path, as this avoids other similar errors in future. Signed off by: Mats Petersson <mats.petersson@citrix.com> Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org> Acked-by: David Vrabel <david.vrabel@citrix.com> --- drivers/xen/privcmd.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c index 8adb9cc..71f5c45 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; } @@ -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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH V3] Correctly return success from IOCTL_PRIVCMD_MMAPBATCH 2012-11-16 18:36 ` [PATCH V3] " Mats Petersson @ 2012-11-19 11:52 ` David Vrabel 0 siblings, 0 replies; 5+ messages in thread From: David Vrabel @ 2012-11-19 11:52 UTC (permalink / raw) To: Mats Petersson; +Cc: konrad.r.wilk, konrad.wilk, xen-devel On 16/11/12 18:36, Mats Petersson 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(). Konrad, This is a regression introduced in 3.7-rc1. Can you pick this one up for 3.7? Thanks. David > > 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!) > > Now also including Andres "move the ret = -EINVAL into the error handling > path, as this avoids other similar errors in future. > > Signed off by: Mats Petersson <mats.petersson@citrix.com> > Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org> > Acked-by: David Vrabel <david.vrabel@citrix.com> > --- > drivers/xen/privcmd.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c > index 8adb9cc..71f5c45 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; > } > > @@ -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) ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-11-19 11:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <mailman.16724.1353079390.1399.xen-devel@lists.xen.org>
2012-11-16 15:37 ` [PATCH] Correctly return success from IOCTL_PRIVCMD_MMAPBATCH Andres Lagar-Cavilla
2012-11-16 15:43 ` David Vrabel
2012-11-16 16:00 ` Andres Lagar-Cavilla
2012-11-16 18:36 ` [PATCH V3] " Mats Petersson
2012-11-19 11:52 ` David Vrabel
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.