[PATCH] Correctly return success from IOCTL_PRIVCMD_MMAPBATCH

[PATCH] Correctly return success from IOCTL_PRIVCMD_MMAPBATCH

[Mats Petersson]

Updated according to David Vrabel's comments. 

--
Mats

[PATCH] Correctly return success from IOCTL_PRIVCMD_MMAPBATCH

[Mats Petersson]

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

Re: [PATCH] Correctly return success from IOCTL_PRIVCMD_MMAPBATCH

[Konrad Rzeszutek Wilk]

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?

Re: [PATCH] Correctly return success from IOCTL_PRIVCMD_MMAPBATCH

[Andres Lagar-Cavilla]

> 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

Re: [PATCH] Correctly return success from IOCTL_PRIVCMD_MMAPBATCH

[David Vrabel]

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
> 

Re: [PATCH] Correctly return success from IOCTL_PRIVCMD_MMAPBATCH

[Andres Lagar-Cavilla]

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

[PATCH V3] Correctly return success from IOCTL_PRIVCMD_MMAPBATCH

[Mats Petersson]

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

Re: [PATCH V3] Correctly return success from IOCTL_PRIVCMD_MMAPBATCH

[David Vrabel]

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)