[PATCH v1] staging: lustre: libcfs: add __user annotation in libcfs_ioctl_data

[PATCH v1] staging: lustre: libcfs: add __user annotation in libcfs_ioctl_data

[David Decotigny]

This fixes the following sparse warnings:
   drivers/staging/lustre/lnet/lnet/api-ni.c:1926:38: warning: incorrect type in argument 1 (different address spaces)
   drivers/staging/lustre/lnet/lnet/api-ni.c:1926:38:    expected void [noderef] <asn:1>*to
   drivers/staging/lustre/lnet/lnet/api-ni.c:1926:38:    got struct lnet_process_id_t [usertype] *
   drivers/staging/lustre/lnet/selftest/conctl.c:833:37: warning: incorrect type in argument 2 (different address spaces)
   drivers/staging/lustre/lnet/selftest/conctl.c:833:37:    expected void const [noderef] <asn:1>*from
   drivers/staging/lustre/lnet/selftest/conctl.c:833:37:    got char *ioc_pbuf1
   drivers/staging/lustre/lnet/selftest/conctl.c:918:30: warning: incorrect type in argument 1 (different address spaces)
   drivers/staging/lustre/lnet/selftest/conctl.c:918:30:    expected void [noderef] <asn:1>*to
   drivers/staging/lustre/lnet/selftest/conctl.c:918:30:    got char *ioc_pbuf2

Signed-off-by: David Decotigny <ddecotig@gmail.com>
---
 drivers/staging/lustre/include/linux/libcfs/libcfs_ioctl.h | 4 ++--
 drivers/staging/lustre/include/linux/lnet/lib-lnet.h       | 2 +-
 drivers/staging/lustre/lnet/lnet/api-ni.c                  | 5 +++--
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs_ioctl.h b/drivers/staging/lustre/include/linux/libcfs/libcfs_ioctl.h
index 3ee3878..aa687b7 100644
--- a/drivers/staging/lustre/include/linux/libcfs/libcfs_ioctl.h
+++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_ioctl.h
@@ -61,9 +61,9 @@ struct libcfs_ioctl_data {
 	char *ioc_inlbuf2;
 
 	__u32 ioc_plen1; /* buffers in userspace */
-	char *ioc_pbuf1;
+	char __user *ioc_pbuf1;
 	__u32 ioc_plen2; /* buffers in userspace */
-	char *ioc_pbuf2;
+	char __user *ioc_pbuf2;
 
 	char ioc_bulk[0];
 };
diff --git a/drivers/staging/lustre/include/linux/lnet/lib-lnet.h b/drivers/staging/lustre/include/linux/lnet/lib-lnet.h
index 0038d29..7f06b9f7 100644
--- a/drivers/staging/lustre/include/linux/lnet/lib-lnet.h
+++ b/drivers/staging/lustre/include/linux/lnet/lib-lnet.h
@@ -858,7 +858,7 @@ void lnet_swap_pinginfo(lnet_ping_info_t *info);
 int lnet_ping_target_init(void);
 void lnet_ping_target_fini(void);
 int lnet_ping(lnet_process_id_t id, int timeout_ms,
-	      lnet_process_id_t *ids, int n_ids);
+	      lnet_process_id_t __user *ids, int n_ids);
 
 int lnet_parse_ip2nets(char **networksp, char *ip2nets);
 int lnet_parse_routes(char *route_str, int *im_a_router);
diff --git a/drivers/staging/lustre/lnet/lnet/api-ni.c b/drivers/staging/lustre/lnet/lnet/api-ni.c
index 4a14e51..1a0cd57 100644
--- a/drivers/staging/lustre/lnet/lnet/api-ni.c
+++ b/drivers/staging/lustre/lnet/lnet/api-ni.c
@@ -1470,7 +1470,7 @@ LNetCtl(unsigned int cmd, void *arg)
 		id.nid = data->ioc_nid;
 		id.pid = data->ioc_u32[0];
 		rc = lnet_ping(id, data->ioc_u32[1], /* timeout */
-			       (lnet_process_id_t *)data->ioc_pbuf1,
+			       (lnet_process_id_t __user *)data->ioc_pbuf1,
 			       data->ioc_plen1/sizeof(lnet_process_id_t));
 		if (rc < 0)
 			return rc;
@@ -1757,7 +1757,8 @@ lnet_ping_target_fini(void)
 }
 
 int
-lnet_ping(lnet_process_id_t id, int timeout_ms, lnet_process_id_t *ids, int n_ids)
+lnet_ping(lnet_process_id_t id, int timeout_ms,
+	  lnet_process_id_t __user *ids, int n_ids)
 {
 	lnet_handle_eq_t     eqh;
 	lnet_handle_md_t     mdh;
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH v1] staging: lustre: libcfs: add __user annotation in libcfs_ioctl_data

[Greg Kroah-Hartman]

On Mon, May 25, 2015 at 09:40:04PM -0700, David Decotigny wrote:
> This fixes the following sparse warnings:
>    drivers/staging/lustre/lnet/lnet/api-ni.c:1926:38: warning: incorrect type in argument 1 (different address spaces)
>    drivers/staging/lustre/lnet/lnet/api-ni.c:1926:38:    expected void [noderef] <asn:1>*to
>    drivers/staging/lustre/lnet/lnet/api-ni.c:1926:38:    got struct lnet_process_id_t [usertype] *
>    drivers/staging/lustre/lnet/selftest/conctl.c:833:37: warning: incorrect type in argument 2 (different address spaces)
>    drivers/staging/lustre/lnet/selftest/conctl.c:833:37:    expected void const [noderef] <asn:1>*from
>    drivers/staging/lustre/lnet/selftest/conctl.c:833:37:    got char *ioc_pbuf1
>    drivers/staging/lustre/lnet/selftest/conctl.c:918:30: warning: incorrect type in argument 1 (different address spaces)
>    drivers/staging/lustre/lnet/selftest/conctl.c:918:30:    expected void [noderef] <asn:1>*to
>    drivers/staging/lustre/lnet/selftest/conctl.c:918:30:    got char *ioc_pbuf2
> 
> Signed-off-by: David Decotigny <ddecotig@gmail.com>
> ---
>  drivers/staging/lustre/include/linux/libcfs/libcfs_ioctl.h | 4 ++--
>  drivers/staging/lustre/include/linux/lnet/lib-lnet.h       | 2 +-
>  drivers/staging/lustre/lnet/lnet/api-ni.c                  | 5 +++--
>  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs_ioctl.h b/drivers/staging/lustre/include/linux/libcfs/libcfs_ioctl.h
> index 3ee3878..aa687b7 100644
> --- a/drivers/staging/lustre/include/linux/libcfs/libcfs_ioctl.h
> +++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_ioctl.h
> @@ -61,9 +61,9 @@ struct libcfs_ioctl_data {
>  	char *ioc_inlbuf2;
>  
>  	__u32 ioc_plen1; /* buffers in userspace */
> -	char *ioc_pbuf1;
> +	char __user *ioc_pbuf1;
>  	__u32 ioc_plen2; /* buffers in userspace */
> -	char *ioc_pbuf2;
> +	char __user *ioc_pbuf2;
>  
>  	char ioc_bulk[0];
>  };
> diff --git a/drivers/staging/lustre/include/linux/lnet/lib-lnet.h b/drivers/staging/lustre/include/linux/lnet/lib-lnet.h
> index 0038d29..7f06b9f7 100644
> --- a/drivers/staging/lustre/include/linux/lnet/lib-lnet.h
> +++ b/drivers/staging/lustre/include/linux/lnet/lib-lnet.h
> @@ -858,7 +858,7 @@ void lnet_swap_pinginfo(lnet_ping_info_t *info);
>  int lnet_ping_target_init(void);
>  void lnet_ping_target_fini(void);
>  int lnet_ping(lnet_process_id_t id, int timeout_ms,
> -	      lnet_process_id_t *ids, int n_ids);
> +	      lnet_process_id_t __user *ids, int n_ids);
>  
>  int lnet_parse_ip2nets(char **networksp, char *ip2nets);
>  int lnet_parse_routes(char *route_str, int *im_a_router);
> diff --git a/drivers/staging/lustre/lnet/lnet/api-ni.c b/drivers/staging/lustre/lnet/lnet/api-ni.c
> index 4a14e51..1a0cd57 100644
> --- a/drivers/staging/lustre/lnet/lnet/api-ni.c
> +++ b/drivers/staging/lustre/lnet/lnet/api-ni.c
> @@ -1470,7 +1470,7 @@ LNetCtl(unsigned int cmd, void *arg)
>  		id.nid = data->ioc_nid;
>  		id.pid = data->ioc_u32[0];
>  		rc = lnet_ping(id, data->ioc_u32[1], /* timeout */
> -			       (lnet_process_id_t *)data->ioc_pbuf1,
> +			       (lnet_process_id_t __user *)data->ioc_pbuf1,

Why is this marking needed?  If so, something must be wrong as isn't
this variable already __user now due to the other part of this patch?

thanks,

greg k-h

Re: [PATCH v1] staging: lustre: libcfs: add __user annotation in libcfs_ioctl_data

[David Decotigny]

Thanks for reviewing.

The 2 struct members were not marked as __user, which this patch does
here. This was causing warnings with copy from/to user (see commit
description). This patch also propagates the annotation to the couple
of functions that are using those members.

On Sat, May 30, 2015 at 7:27 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, May 25, 2015 at 09:40:04PM -0700, David Decotigny wrote:
>> This fixes the following sparse warnings:
>>    drivers/staging/lustre/lnet/lnet/api-ni.c:1926:38: warning: incorrect type in argument 1 (different address spaces)
>>    drivers/staging/lustre/lnet/lnet/api-ni.c:1926:38:    expected void [noderef] <asn:1>*to
>>    drivers/staging/lustre/lnet/lnet/api-ni.c:1926:38:    got struct lnet_process_id_t [usertype] *
>>    drivers/staging/lustre/lnet/selftest/conctl.c:833:37: warning: incorrect type in argument 2 (different address spaces)
>>    drivers/staging/lustre/lnet/selftest/conctl.c:833:37:    expected void const [noderef] <asn:1>*from
>>    drivers/staging/lustre/lnet/selftest/conctl.c:833:37:    got char *ioc_pbuf1
>>    drivers/staging/lustre/lnet/selftest/conctl.c:918:30: warning: incorrect type in argument 1 (different address spaces)
>>    drivers/staging/lustre/lnet/selftest/conctl.c:918:30:    expected void [noderef] <asn:1>*to
>>    drivers/staging/lustre/lnet/selftest/conctl.c:918:30:    got char *ioc_pbuf2
>>
>> Signed-off-by: David Decotigny <ddecotig@gmail.com>
>> ---
>>  drivers/staging/lustre/include/linux/libcfs/libcfs_ioctl.h | 4 ++--
>>  drivers/staging/lustre/include/linux/lnet/lib-lnet.h       | 2 +-
>>  drivers/staging/lustre/lnet/lnet/api-ni.c                  | 5 +++--
>>  3 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs_ioctl.h b/drivers/staging/lustre/include/linux/libcfs/libcfs_ioctl.h
>> index 3ee3878..aa687b7 100644
>> --- a/drivers/staging/lustre/include/linux/libcfs/libcfs_ioctl.h
>> +++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_ioctl.h
>> @@ -61,9 +61,9 @@ struct libcfs_ioctl_data {
>>       char *ioc_inlbuf2;
>>
>>       __u32 ioc_plen1; /* buffers in userspace */
>> -     char *ioc_pbuf1;
>> +     char __user *ioc_pbuf1;
>>       __u32 ioc_plen2; /* buffers in userspace */
>> -     char *ioc_pbuf2;
>> +     char __user *ioc_pbuf2;
>>
>>       char ioc_bulk[0];
>>  };
>> diff --git a/drivers/staging/lustre/include/linux/lnet/lib-lnet.h b/drivers/staging/lustre/include/linux/lnet/lib-lnet.h
>> index 0038d29..7f06b9f7 100644
>> --- a/drivers/staging/lustre/include/linux/lnet/lib-lnet.h
>> +++ b/drivers/staging/lustre/include/linux/lnet/lib-lnet.h
>> @@ -858,7 +858,7 @@ void lnet_swap_pinginfo(lnet_ping_info_t *info);
>>  int lnet_ping_target_init(void);
>>  void lnet_ping_target_fini(void);
>>  int lnet_ping(lnet_process_id_t id, int timeout_ms,
>> -           lnet_process_id_t *ids, int n_ids);
>> +           lnet_process_id_t __user *ids, int n_ids);
>>
>>  int lnet_parse_ip2nets(char **networksp, char *ip2nets);
>>  int lnet_parse_routes(char *route_str, int *im_a_router);
>> diff --git a/drivers/staging/lustre/lnet/lnet/api-ni.c b/drivers/staging/lustre/lnet/lnet/api-ni.c
>> index 4a14e51..1a0cd57 100644
>> --- a/drivers/staging/lustre/lnet/lnet/api-ni.c
>> +++ b/drivers/staging/lustre/lnet/lnet/api-ni.c
>> @@ -1470,7 +1470,7 @@ LNetCtl(unsigned int cmd, void *arg)
>>               id.nid = data->ioc_nid;
>>               id.pid = data->ioc_u32[0];
>>               rc = lnet_ping(id, data->ioc_u32[1], /* timeout */
>> -                            (lnet_process_id_t *)data->ioc_pbuf1,
>> +                            (lnet_process_id_t __user *)data->ioc_pbuf1,
>
> Why is this marking needed?  If so, something must be wrong as isn't
> this variable already __user now due to the other part of this patch?
>
> thanks,
>
> greg k-h

Re: [PATCH v1] staging: lustre: libcfs: add __user annotation in libcfs_ioctl_data

[Greg Kroah-Hartman]

On Mon, Jun 01, 2015 at 12:21:30PM -0700, David Decotigny wrote:
> Thanks for reviewing.
> 
> The 2 struct members were not marked as __user, which this patch does
> here. This was causing warnings with copy from/to user (see commit
> description). This patch also propagates the annotation to the couple
> of functions that are using those members.

Lustre's structures are a total mess of kernel and user pointers and
trying to properly mark them as which they are supposed to be at what
point in time is a very difficult task.  People keep trying and get it
wrong, so I suggest just leaving this alone until the developers unwind
the structure mess as that will be necessary for this code to get merged
into the main part of the kernel.

sorry,

greg k-h

RE: [HPDD-discuss] [PATCH v1] staging: lustre: libcfs: add __user annotation in libcfs_ioctl_data

[Simmons, James A.]

>>On Mon, Jun 01, 2015 at 12:21:30PM -0700, David Decotigny wrote:
>> Thanks for reviewing.
>> 
>> The 2 struct members were not marked as __user, which this patch does
>> here. This was causing warnings with copy from/to user (see commit
>> description). This patch also propagates the annotation to the couple
>> of functions that are using those members.
>
>Lustre's structures are a total mess of kernel and user pointers and
>trying to properly mark them as which they are supposed to be at what
>point in time is a very difficult task.  People keep trying and get it
>wrong, so I suggest just leaving this alone until the developers unwind
>the structure mess as that will be necessary for this code to get merged
>into the main part of the kernel.

Greg is right. The earlier patch set I sent out for the LNet headers
address this issue for the LNet layer. I also having patches coming that
fix libcfs ioctl handling as well. I see Shuey's patches made it in first so
I'm going to have to rebase. I will send out the new patch sets later today.
This will be v3 of the patch set.