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.
diff --git a/drivers/staging/comedi/comedi.h b/drivers/staging/comedi/comedi.h
index 8ea55ae..2e2f366 100644
--- a/drivers/staging/comedi/comedi.h
+++ b/drivers/staging/comedi/comedi.h
@@ -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 {
On Wed, May 09, 2012 at 09:36:03AM -0700, H Hartley Sweeten wrote:
> 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:
That would be "Greg" :)
On Wed, May 09, 2012 at 09:36:03AM -0700, H Hartley Sweeten wrote:
> 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."
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
On Wednesday, May 09, 2012 1:42 PM, Greg KH wrote:
> On Wed, May 09, 2012 at 09:36:03AM -0700, H Hartley Sweeten wrote:
>> 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:
Sorry about that typo... ;-)
>>
>> "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...
>
> 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