Skip to contentSkip to conversation

staging: comedi: remove __user annotation inside of struct's

PATCH In review H Hartley Sweeten 2 participants 4 messages May 9, 2012 Staging

lore
Share & tools
lore

H Hartley Sweeten

author
#0 lore raw

PATCH v2 1/1 staging: comedi: remove __user annotation inside of struct's

Remove the __user annotations in the struct definitions in comedi.h.

These structs are used to pass information from user-space to kernel-space. The copy_from_user and copy_to_user functions are used to transfer the data between the address spaces.

The drivers then use the information internally under the assumption that they are kernel-space objects. Having the __user annotations inside the structs produces a number of sparse warnings of the type:

warning: dereference of noderef expression

According to Grek Kroah-Hartman:

"When the kernel exports .h files, stuff like this should work "automatically", so there is no need to not put __user markings."

Signed-off-by H Hartley Sweeten <[email protected]>
Cc Ian Abbott <[email protected]>
Cc Mori Hess <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>

---

v2: update commit message

Note: This patch exposes some new warnings about different address space. These will be addressed.

drivers/staging/comedi/comedi.h +8 -8
index 8ea55ae..2e2f366 100644
@@ -335,7 +335,7 @@
 	struct comedi_insn {
 		unsigned int insn;
 		unsigned int n;
-		unsigned int __user *data;
+		unsigned int *data;
 		unsigned int subdev;
 		unsigned int chanspec;
 		unsigned int unused[3];

@@ -343,7 +343,7 @@
 
 	struct comedi_insnlist {
 		unsigned int n_insns;
-		struct comedi_insn __user *insns;
+		struct comedi_insn *insns;
 	};
 
 	struct comedi_cmd {

@@ -365,24 +365,24 @@
 		unsigned int stop_src;
 		unsigned int stop_arg;
 
-		unsigned int __user *chanlist;	/* channel/range list */
+		unsigned int *chanlist;	/* channel/range list */
 		unsigned int chanlist_len;
 
-		short __user *data; /* data list, size depends on subd flags */
+		short *data; /* data list, size depends on subd flags */
 		unsigned int data_len;
 	};
 
 	struct comedi_chaninfo {
 		unsigned int subdev;
-		unsigned int __user *maxdata_list;
-		unsigned int __user *flaglist;
-		unsigned int __user *rangelist;
+		unsigned int *maxdata_list;
+		unsigned int *flaglist;
+		unsigned int *rangelist;
 		unsigned int unused[4];
 	};
 
 	struct comedi_rangeinfo {
 		unsigned int range_type;
-		void __user *range_ptr;
+		void *range_ptr;
 	};
 
 	struct comedi_krange {
maintainer
#1 lore raw
Quoted 7 lines · H Hartley Sweeten

Remove the __user annotations in the struct definitions in comedi.h.

These structs are used to pass information from user-space to kernel-space. The copy_from_user and copy_to_user functions are used to transfer the data between the address spaces.

Quoted 8 lines

The drivers then use the information internally under the assumption that they are kernel-space objects. Having the __user annotations inside the structs produces a number of sparse warnings of the type:

warning: dereference of noderef expression

According to Grek Kroah-Hartman:

That would be "Greg" :)

maintainer
#2 lore raw
In reply to H Hartley Sweeten
Quoted 12 lines · H Hartley Sweeten

Remove the __user annotations in the struct definitions in comedi.h.

These structs are used to pass information from user-space to kernel-space. The copy_from_user and copy_to_user functions are used to transfer the data between the address spaces.

The drivers then use the information internally under the assumption that they are kernel-space objects. Having the __user annotations inside the structs produces a number of sparse warnings of the type:

Quoted 6 lines

warning: dereference of noderef expression

According to Grek Kroah-Hartman:

"When the kernel exports .h files, stuff like this should work "automatically", so there is no need to not put __user markings."

Wait, for some reason I thought you were adding __user markings, not removing them from the structures. That is why I said the above statement.

Why again are you removing them? Didn't you just cause more sparse warnings to pop up?

confused,

greg k-h

author
#3 lore raw
Quoted 8 lines · Greg KH
Quoted 7 lines · H Hartley Sweeten

Remove the __user annotations in the struct definitions in comedi.h.

These structs are used to pass information from user-space to kernel-space. The copy_from_user and copy_to_user functions are used to transfer the data between the address spaces.

Quoted 8 lines
Quoted 8 lines

The drivers then use the information internally under the assumption that they are kernel-space objects. Having the __user annotations inside the structs produces a number of sparse warnings of the type:

warning: dereference of noderef expression

According to Grek Kroah-Hartman:

Sorry about that typo... ;-)

Quoted 7 lines
Quoted 3 lines

"When the kernel exports .h files, stuff like this should work "automatically", so there is no need to not put __user markings."

Wait, for some reason I thought you were adding __user markings, not removing them from the structures. That is why I said the above statement.

Ah.. Missed the "not" in your comment...

Quoted 3 lines

Why again are you removing them? Didn't you just cause more sparse warnings to pop up?

I still think they should be removed but I need to dig thru it a bit.

The structs in question are used internally in the comedi drivers after the data has been passed into the kernel using the various ioctl's. The ioctl code does do the copy_from/to_user correctly (kind of, I did notice one place where the copy_from_user is referencing a kernel pointer). But, when the driver's try to use the data, assuming it's kernel space data, it's still annotated as __user so we get all the "warning: dereference of noderef expression" along with a couple other sparse warnings.

Ian Abbott did reply that he thought the patch was correct. With my miss-read of your comment I thought you were agreeing.

Regardless, I'll keep looking it over...

Regards,
Hartley