[patch 3/3] xen/privcmd: remove const modifier from declaration

[patch 3/3] xen/privcmd: remove const modifier from declaration

[Dan Carpenter]

When we use this pointer, we cast away the const modifier and modify the
data.  I think it was an accident to declare it as const.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/include/xen/privcmd.h b/include/xen/privcmd.h
index a853168..58ed953 100644
--- a/include/xen/privcmd.h
+++ b/include/xen/privcmd.h
@@ -69,7 +69,7 @@ struct privcmd_mmapbatch_v2 {
 	unsigned int num; /* number of pages to populate */
 	domid_t dom;      /* target domain */
 	__u64 addr;       /* virtual address */
-	const xen_pfn_t __user *arr; /* array of mfns */
+	xen_pfn_t __user *arr; /* array of mfns */
 	int __user *err;  /* array of error codes */
 };
 
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 0ce006a..fceb83e 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -389,7 +389,7 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
 
 	if (state.global_error && (version == 1)) {
 		/* Write back errors in second pass. */
-		state.user_mfn = (xen_pfn_t *)m.arr;
+		state.user_mfn = m.arr;
 		state.err      = err_array;
 		ret = traverse_pages(m.num, sizeof(xen_pfn_t),
 				     &pagelist, mmap_return_errors_v1, &state);

Re: [patch 3/3] xen/privcmd: remove const modifier from declaration

[Andres Lagar-Cavilla]

On Sep 8, 2012, at 5:58 AM, Dan Carpenter wrote:

> When we use this pointer, we cast away the const modifier and modify the
> data.  I think it was an accident to declare it as const.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

Thanks for the careful review & hardening.
Andres

> 
> diff --git a/include/xen/privcmd.h b/include/xen/privcmd.h
> index a853168..58ed953 100644
> --- a/include/xen/privcmd.h
> +++ b/include/xen/privcmd.h
> @@ -69,7 +69,7 @@ struct privcmd_mmapbatch_v2 {
> 	unsigned int num; /* number of pages to populate */
> 	domid_t dom;      /* target domain */
> 	__u64 addr;       /* virtual address */
> -	const xen_pfn_t __user *arr; /* array of mfns */
> +	xen_pfn_t __user *arr; /* array of mfns */
> 	int __user *err;  /* array of error codes */
> };
> 
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index 0ce006a..fceb83e 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -389,7 +389,7 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
> 
> 	if (state.global_error && (version == 1)) {
> 		/* Write back errors in second pass. */
> -		state.user_mfn = (xen_pfn_t *)m.arr;
> +		state.user_mfn = m.arr;
> 		state.err      = err_array;
> 		ret = traverse_pages(m.num, sizeof(xen_pfn_t),
> 				     &pagelist, mmap_return_errors_v1, &state);

Re: [Xen-devel] [patch 3/3] xen/privcmd: remove const modifier from declaration

[Jan Beulich]

>>> On 08.09.12 at 11:58, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> When we use this pointer, we cast away the const modifier and modify the
> data.  I think it was an accident to declare it as const.

NAK - the const is very valid here, as the v2 interface (as opposed
to the v1 one) does _not_ modify this array (or if it does, it's a
bug). This is a guarantee made to user mode, so it should also be
expressed that way in the interface.

But of course the cast used before this patch isn't right either, as
it indeed inappropriately discards the qualifier. Afaiu this was done
to simplify the internal workings of the code, but I don't think it's
desirable to sacrifice type safety for implementation simplicity.

Jan

> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/include/xen/privcmd.h b/include/xen/privcmd.h
> index a853168..58ed953 100644
> --- a/include/xen/privcmd.h
> +++ b/include/xen/privcmd.h
> @@ -69,7 +69,7 @@ struct privcmd_mmapbatch_v2 {
>  	unsigned int num; /* number of pages to populate */
>  	domid_t dom;      /* target domain */
>  	__u64 addr;       /* virtual address */
> -	const xen_pfn_t __user *arr; /* array of mfns */
> +	xen_pfn_t __user *arr; /* array of mfns */
>  	int __user *err;  /* array of error codes */
>  };
>  
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index 0ce006a..fceb83e 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -389,7 +389,7 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, 
> int version)
>  
>  	if (state.global_error && (version == 1)) {
>  		/* Write back errors in second pass. */
> -		state.user_mfn = (xen_pfn_t *)m.arr;
> +		state.user_mfn = m.arr;
>  		state.err      = err_array;
>  		ret = traverse_pages(m.num, sizeof(xen_pfn_t),
>  				     &pagelist, mmap_return_errors_v1, &state);
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

[patch 3/3 v2] xen/privcmd: add a __user annotation to a cast

[Dan Carpenter]

Sparse complains that we lose the __user annotation in the cast here.

drivers/xen/privcmd.c:388:35: warning: cast removes address space of expression
drivers/xen/privcmd.c:388:32: warning: incorrect type in assignment (different address spaces)
drivers/xen/privcmd.c:388:32:    expected unsigned long [noderef] [usertype] <asn:1>*[addressable] [assigned] user_mfn
drivers/xen/privcmd.c:388:32:    got unsigned long [usertype] *<noident>

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: In v1 I removed the const from the declaration but now I just removed it
from the cast.  This data can either be a v1 or a v2 type struct.  The m.arr
data is const in version 2 but not in version 1.

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 0ce006a..fceb83e 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -389,7 +389,7 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
 
 	if (state.global_error && (version == 1)) {
 		/* Write back errors in second pass. */
-		state.user_mfn = (xen_pfn_t *)m.arr;
+		state.user_mfn = (xen_pfn_t __user *)m.arr;
 		state.err      = err_array;
 		ret = traverse_pages(m.num, sizeof(xen_pfn_t),
 				     &pagelist, mmap_return_errors_v1, &state);

Re: [Xen-devel] [patch 3/3] xen/privcmd: remove const modifier from declaration

[David Vrabel]

On 10/09/12 10:03, Jan Beulich wrote:
>>>> On 08.09.12 at 11:58, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> When we use this pointer, we cast away the const modifier and modify the
>> data.  I think it was an accident to declare it as const.
> 
> NAK - the const is very valid here, as the v2 interface (as opposed
> to the v1 one) does _not_ modify this array (or if it does, it's a
> bug). This is a guarantee made to user mode, so it should also be
> expressed that way in the interface.
> 
> But of course the cast used before this patch isn't right either, as
> it indeed inappropriately discards the qualifier. Afaiu this was done
> to simplify the internal workings of the code, but I don't think it's
> desirable to sacrifice type safety for implementation simplicity.

m.arr here isn't const as m is really the V1 structure where m.arr is
non-const.

David