Linux Virtualization - Sep 2012 - [patch 3/3] xen/privcmd: remove const modifier from declaration

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 at 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);

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 at oracle.com>
Acked-by: Andres Lagar-Cavilla <andres at 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);

>>> On 08.09.12 at 11:58, Dan Carpenter <dan.carpenter at
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 at 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 at lists.xen.org 
> http://lists.xen.org/xen-devel

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 at 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);