Skip to contentSkip to conversation

staging: lustre: libcfs: add __user annotation in libcfs_ioctl_data

PATCH In review David Decotigny 3 participants 5 messages May 26, 2015 – Jun 2, 2015 Staging

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 staging: lustre: libcfs: add __user annotation in libcfs_ioctl_data

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

---
 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(-)
drivers/staging/lustre/include/linux/libcfs/libcfs_ioctl.h +2 -2
index 3ee3878..aa687b7 100644
@@ -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];
 };
drivers/staging/lustre/include/linux/lnet/lib-lnet.h +1 -1
index 0038d29..7f06b9f7 100644
@@ -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);
drivers/staging/lustre/lnet/lnet/api-ni.c +3 -2
index 4a14e51..1a0cd57 100644
@@ -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;
Signature · 1 line
2.2.0.rc0.207.ga3a616c
#1 lore raw
Quoted 52 lines · 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

---
 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(-)
drivers/staging/lustre/include/linux/libcfs/libcfs_ioctl.h +2 -2
index 3ee3878..aa687b7 100644
@@ -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];
 };
drivers/staging/lustre/include/linux/lnet/lib-lnet.h +1 -1
index 0038d29..7f06b9f7 100644
@@ -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);
drivers/staging/lustre/lnet/lnet/api-ni.c +0 -0
index 4a14e51..1a0cd57 100644

drivers/staging/lustre/lnet/lnet/api-ni.c LNetCtl(unsigned int cmd, void *arg) · line 1473

Quoted 5 lines
drivers/staging/lustre/lnet/lnet/api-ni.c +1 -1
@@ -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

#2 lore raw

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.

Quoted 65 lines · Greg Kroah-Hartman
Quoted 52 lines · 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

---
 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(-)
drivers/staging/lustre/include/linux/libcfs/libcfs_ioctl.h +2 -2
index 3ee3878..aa687b7 100644
@@ -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];
 };
drivers/staging/lustre/include/linux/lnet/lib-lnet.h +1 -1
index 0038d29..7f06b9f7 100644
@@ -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);
drivers/staging/lustre/lnet/lnet/api-ni.c +0 -0
index 4a14e51..1a0cd57 100644

drivers/staging/lustre/lnet/lnet/api-ni.c LNetCtl(unsigned int cmd, void *arg) · line 1473

Quoted 5 lines
drivers/staging/lustre/lnet/lnet/api-ni.c +1 -1
@@ -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

#3 lore raw
Quoted 6 lines · 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.

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

#4 lore raw
Quoted 9 lines
Quoted 8 lines · 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.

Quoted 6 lines

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.