Xen devel - Nov 2012 - [PATCH 1/2] Fix broken IOCTL_PRIVCMD_MMAPBATCH (old version).

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>
---
 drivers/xen/privcmd.c |   18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 8adb9cc..b378343 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -347,6 +347,7 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int
version)
 
 	if (ret)
 		goto out;
+
 	if (list_empty(&pagelist)) {
 		ret = -EINVAL;
 		goto out;
@@ -383,12 +384,17 @@ 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

On 16/11/12 10:47, Mats Petersson wrote:
> 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!)
Better subject line:

"xen/privcmd: 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().  It would be nice if
the commit message mentioned this.
> Signed off by: Mats Petersson <mats.petersson@citrix.com>
If the subject/commit message is improved:

Acked-by: David Vrabel <david.vrabel@citrix.com>
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -347,6 +347,7 @@ static long privcmd_ioctl_mmap_batch(void __user
*udata, int version)
>  
>  	if (ret)
>  		goto out;
> +
Stray change, please remove.
>  	if (list_empty(&pagelist)) {
>  		ret = -EINVAL;
>  		goto out;
David

Updated according to David Vrabel''s comments. 

--
Mats

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>

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

On Fri, Nov 16, 2012 at 03:02:02PM +0000, Mats Petersson
wrote:
> Updated according to David Vrabel''s comments. 
Huh? Where is the patch?