The structs' comedi_insn, coomedi_insnlist, comedi_cmd,
comedi_chaninfo, and comedi_rangeinfo are all passed to
the kernel from user space using ioctl commands. They
are then copied to kernel space using copy_from_user()
before the data is passed to the drivers.
The __user annotation should not be used with variables
inside the struct. This produces a lot of sparse warnings
like:
warning: dereference of noderef expression
Signed-off-by: H Hartley Sweeten <[email protected]>
Cc: Ian Abbott <[email protected]>
Cc: Mori Hess <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
---
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 Tuesday, May 08, 2012 4:41 PM, H Hartley Sweeten wrote:
>
> The structs' comedi_insn, coomedi_insnlist, comedi_cmd,
> comedi_chaninfo, and comedi_rangeinfo are all passed to
> the kernel from user space using ioctl commands. They
> are then copied to kernel space using copy_from_user()
> before the data is passed to the drivers.
>
> The __user annotation should not be used with variables
> inside the struct. This produces a lot of sparse warnings
> like:
>
> warning: dereference of noderef expression
>
> Signed-off-by: H Hartley Sweeten <[email protected]>
> Cc: Ian Abbott <[email protected]>
> Cc: Mori Hess <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
>
> ---
>
> Note: This patch exposes some new warnings about different
> address space. These will be addressed.
Please ignore this patch.
It appears the annotations in the struct definitions are correct.
The initial copy_from_user will only copy the data from the
struct __user *arg to the kernel's struct * it doesn't copy the
data that the struct variable points to, just the pointer. To get
that data another copy_from_user needs to be done.
The sparse warnings above will need to be addressed a
different way.
Sorry for the noise.
Regards,
Hartley
On 2012-05-09 00:55, H Hartley Sweeten wrote:
> On Tuesday, May 08, 2012 4:41 PM, H Hartley Sweeten wrote:
>>
>> The structs' comedi_insn, coomedi_insnlist, comedi_cmd,
>> comedi_chaninfo, and comedi_rangeinfo are all passed to
>> the kernel from user space using ioctl commands. They
>> are then copied to kernel space using copy_from_user()
>> before the data is passed to the drivers.
>>
>> The __user annotation should not be used with variables
>> inside the struct. This produces a lot of sparse warnings
>> like:
>>
>> warning: dereference of noderef expression
>>
>> Signed-off-by: H Hartley Sweeten<[email protected]>
>> Cc: Ian Abbott<[email protected]>
>> Cc: Mori Hess<[email protected]>
>> Cc: Greg Kroah-Hartman<[email protected]>
>>
>> ---
>>
>> Note: This patch exposes some new warnings about different
>> address space. These will be addressed.
>
> Please ignore this patch.
>
> It appears the annotations in the struct definitions are correct.
Personally, I think you were on the mark with the patch. It's better to
avoid using __user in comedi.h so it can be used as-is in user-space.
All of the structures in comedi.h are used in user-space (although
Comedilib uses its own version of comedi.h without all the typedef
eliminations that have been done in "staging") and some of them are also
deep-copied into kernel-space objects of the same type, where the
pointers in the structs would no longer be user-space pointers.
--
-=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
On Wed, May 09, 2012 at 11:20:07AM +0100, Ian Abbott wrote:
> On 2012-05-09 00:55, H Hartley Sweeten wrote:
> >On Tuesday, May 08, 2012 4:41 PM, H Hartley Sweeten wrote:
> >>
> >>The structs' comedi_insn, coomedi_insnlist, comedi_cmd,
> >>comedi_chaninfo, and comedi_rangeinfo are all passed to
> >>the kernel from user space using ioctl commands. They
> >>are then copied to kernel space using copy_from_user()
> >>before the data is passed to the drivers.
> >>
> >>The __user annotation should not be used with variables
> >>inside the struct. This produces a lot of sparse warnings
> >>like:
> >>
> >>warning: dereference of noderef expression
> >>
> >>Signed-off-by: H Hartley Sweeten<[email protected]>
> >>Cc: Ian Abbott<[email protected]>
> >>Cc: Mori Hess<[email protected]>
> >>Cc: Greg Kroah-Hartman<[email protected]>
> >>
> >>---
> >>
> >>Note: This patch exposes some new warnings about different
> >>address space. These will be addressed.
> >
> >Please ignore this patch.
> >
> >It appears the annotations in the struct definitions are correct.
>
> Personally, I think you were on the mark with the patch. It's
> better to avoid using __user in comedi.h so it can be used as-is in
> user-space.
Sparse is useful so we shouldn't break it. I always run sparse over
my patches before submission and look at the warnings. Except if
they scroll off the page. In that case, I just figure that the
author deserves the bugs.
We could just do some ifdeferry to fix it for userspace.
regards,
dan carpenter
On 2012-05-09 11:31, Dan Carpenter wrote:
> On Wed, May 09, 2012 at 11:20:07AM +0100, Ian Abbott wrote:
>> On 2012-05-09 00:55, H Hartley Sweeten wrote:
>>> On Tuesday, May 08, 2012 4:41 PM, H Hartley Sweeten wrote:
>>>>
>>>> The structs' comedi_insn, coomedi_insnlist, comedi_cmd,
>>>> comedi_chaninfo, and comedi_rangeinfo are all passed to
>>>> the kernel from user space using ioctl commands. They
>>>> are then copied to kernel space using copy_from_user()
>>>> before the data is passed to the drivers.
>>>>
>>>> The __user annotation should not be used with variables
>>>> inside the struct. This produces a lot of sparse warnings
>>>> like:
>>>>
>>>> warning: dereference of noderef expression
>>>>
>>>> Note: This patch exposes some new warnings about different
>>>> address space. These will be addressed.
>>>
>>> Please ignore this patch.
>>>
>>> It appears the annotations in the struct definitions are correct.
>>
>> Personally, I think you were on the mark with the patch. It's
>> better to avoid using __user in comedi.h so it can be used as-is in
>> user-space.
>
> Sparse is useful so we shouldn't break it. I always run sparse over
> my patches before submission and look at the warnings. Except if
> they scroll off the page. In that case, I just figure that the
> author deserves the bugs.
>
> We could just do some ifdeferry to fix it for userspace.
That doesn't help in cases such as 'struct comedi_insn' where the 'data'
pointer is a user-space pointer in the user-space copy of the object and
a kernel-space pointer in the kernel-space copy of the object. The only
fix for that is to have separate "k" versions of the struct or to do a
load of casting, which is slightly error-prone and makes the code less
readable.
Are there any handy macros for casting pointers to __user pointers,
something like
#define _user(p) ((typeof(*(p)) __user *)(p))
but preferably without the repeated expansion of 'p' in case of
side-effects?
--
-=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
On Wed, May 09, 2012 at 12:01:25PM +0100, Ian Abbott wrote:
> Are there any handy macros for casting pointers to __user pointers,
> something like
>
> #define _user(p) ((typeof(*(p)) __user *)(p))
>
> but preferably without the repeated expansion of 'p' in case of
> side-effects?
typeof() doesn't have side effects.
#include <stdio.h>
int main(void)
{
int x = 0;
typeof(x++) y;
printf("%d\n", x);
return 0;
}
regards,
dan carpenter
On Wed, May 09, 2012 at 11:20:07AM +0100, Ian Abbott wrote:
> On 2012-05-09 00:55, H Hartley Sweeten wrote:
> >On Tuesday, May 08, 2012 4:41 PM, H Hartley Sweeten wrote:
> >>
> >>The structs' comedi_insn, coomedi_insnlist, comedi_cmd,
> >>comedi_chaninfo, and comedi_rangeinfo are all passed to
> >>the kernel from user space using ioctl commands. They
> >>are then copied to kernel space using copy_from_user()
> >>before the data is passed to the drivers.
> >>
> >>The __user annotation should not be used with variables
> >>inside the struct. This produces a lot of sparse warnings
> >>like:
> >>
> >>warning: dereference of noderef expression
> >>
> >>Signed-off-by: H Hartley Sweeten<[email protected]>
> >>Cc: Ian Abbott<[email protected]>
> >>Cc: Mori Hess<[email protected]>
> >>Cc: Greg Kroah-Hartman<[email protected]>
> >>
> >>---
> >>
> >>Note: This patch exposes some new warnings about different
> >>address space. These will be addressed.
> >
> >Please ignore this patch.
> >
> >It appears the annotations in the struct definitions are correct.
>
> Personally, I think you were on the mark with the patch. It's
> better to avoid using __user in comedi.h so it can be used as-is in
> user-space. All of the structures in comedi.h are used in user-space
> (although Comedilib uses its own version of comedi.h without all the
> typedef eliminations that have been done in "staging") and some of
> them are also deep-copied into kernel-space objects of the same
> type, where the pointers in the structs would no longer be
> user-space pointers.
When the kernel exports .h files, stuff like this should work
"automatically", so there is no need to not put __user markings.
So please, yes, continue to work to get this correct, it is very
essencial to ensure we don't mess stuff up.
thanks,
greg k-h
On Wednesday, May 09, 2012 7:25 AM, [email protected] wrote:
> On Wed, May 09, 2012 at 11:20:07AM +0100, Ian Abbott wrote:
>> On 2012-05-09 00:55, H Hartley Sweeten wrote:
>>> On Tuesday, May 08, 2012 4:41 PM, H Hartley Sweeten wrote:
>>>>
>>>> The structs' comedi_insn, coomedi_insnlist, comedi_cmd,
>>>> comedi_chaninfo, and comedi_rangeinfo are all passed to
>>>> the kernel from user space using ioctl commands. They
>>>> are then copied to kernel space using copy_from_user()
>>>> before the data is passed to the drivers.
>>>>
>>>> The __user annotation should not be used with variables
>>>> inside the struct. This produces a lot of sparse warnings
>>>> like:
>>>>
>>>>warning: dereference of noderef expression
>>>>
>>>> Signed-off-by: H Hartley Sweeten<[email protected]>
>>>> Cc: Ian Abbott<[email protected]>
>>>> Cc: Mori Hess<[email protected]>
>>>> Cc: Greg Kroah-Hartman<[email protected]>
>>>>
>>>> ---
>>>>
>>>> Note: This patch exposes some new warnings about different
>>>> address space. These will be addressed.
>>>
>>> Please ignore this patch.
>>>
>>> It appears the annotations in the struct definitions are correct.
>>
>> Personally, I think you were on the mark with the patch. It's
>> better to avoid using __user in comedi.h so it can be used as-is in
>> user-space. All of the structures in comedi.h are used in user-space
>> (although Comedilib uses its own version of comedi.h without all the
>> typedef eliminations that have been done in "staging") and some of
>> them are also deep-copied into kernel-space objects of the same
>> type, where the pointers in the structs would no longer be
>> user-space pointers.
>
> When the kernel exports .h files, stuff like this should work
> "automatically", so there is no need to not put __user markings.
>
> So please, yes, continue to work to get this correct, it is very
> essencial to ensure we don't mess stuff up.
Greg,
Based on the discussion I assume this patch is ok then. Do you
want me to repost it?
I'll work on fixing the other issues with the user <-> kernel transitions.
Regards,
Hartley
On Wed, May 09, 2012 at 10:52:34AM -0500, H Hartley Sweeten wrote:
> On Wednesday, May 09, 2012 7:25 AM, [email protected] wrote:
> > On Wed, May 09, 2012 at 11:20:07AM +0100, Ian Abbott wrote:
> >> On 2012-05-09 00:55, H Hartley Sweeten wrote:
> >>> On Tuesday, May 08, 2012 4:41 PM, H Hartley Sweeten wrote:
> >>>>
> >>>> The structs' comedi_insn, coomedi_insnlist, comedi_cmd,
> >>>> comedi_chaninfo, and comedi_rangeinfo are all passed to
> >>>> the kernel from user space using ioctl commands. They
> >>>> are then copied to kernel space using copy_from_user()
> >>>> before the data is passed to the drivers.
> >>>>
> >>>> The __user annotation should not be used with variables
> >>>> inside the struct. This produces a lot of sparse warnings
> >>>> like:
> >>>>
> >>>>warning: dereference of noderef expression
> >>>>
> >>>> Signed-off-by: H Hartley Sweeten<[email protected]>
> >>>> Cc: Ian Abbott<[email protected]>
> >>>> Cc: Mori Hess<[email protected]>
> >>>> Cc: Greg Kroah-Hartman<[email protected]>
> >>>>
> >>>> ---
> >>>>
> >>>> Note: This patch exposes some new warnings about different
> >>>> address space. These will be addressed.
> >>>
> >>> Please ignore this patch.
> >>>
> >>> It appears the annotations in the struct definitions are correct.
> >>
> >> Personally, I think you were on the mark with the patch. It's
> >> better to avoid using __user in comedi.h so it can be used as-is in
> >> user-space. All of the structures in comedi.h are used in user-space
> >> (although Comedilib uses its own version of comedi.h without all the
> >> typedef eliminations that have been done in "staging") and some of
> >> them are also deep-copied into kernel-space objects of the same
> >> type, where the pointers in the structs would no longer be
> >> user-space pointers.
> >
> > When the kernel exports .h files, stuff like this should work
> > "automatically", so there is no need to not put __user markings.
> >
> > So please, yes, continue to work to get this correct, it is very
> > essencial to ensure we don't mess stuff up.
>
> Greg,
>
> Based on the discussion I assume this patch is ok then. Do you
> want me to repost it?
Sure, I'll review it with the other patches you have sent later today.
> I'll work on fixing the other issues with the user <-> kernel transitions.
Wonderful, thanks for the work you are doing on this code, it's greatly
appreciated.
greg k-h
On Wednesday, May 09, 2012 8:57 AM, [email protected] wrote:
> On Wed, May 09, 2012 at 10:52:34AM -0500, H Hartley Sweeten wrote:
>> On Wednesday, May 09, 2012 7:25 AM, [email protected] wrote:
>>> When the kernel exports .h files, stuff like this should work
>>> "automatically", so there is no need to not put __user markings.
>>>
>>> So please, yes, continue to work to get this correct, it is very
>>> essencial to ensure we don't mess stuff up.
>>
>> Greg,
>>
>> Based on the discussion I assume this patch is ok then. Do you
>> want me to repost it?
>
> Sure, I'll review it with the other patches you have sent later today.
Will do!
>> I'll work on fixing the other issues with the user <-> kernel transitions.
>
> Wonderful, thanks for the work you are doing on this code, it's greatly
> appreciated.
Glad I can help!
Regards,
Hartley
On Wednesday, May 09, 2012 8:57 AM, [email protected] wrote:
> On Wed, May 09, 2012 at 10:52:34AM -0500, H Hartley Sweeten wrote:
>> Based on the discussion I assume this patch is ok then. Do you
>> want me to repost it?
>
> Sure, I'll review it with the other patches you have sent later today.
Greg,
I have re-posted the patch with an updated commit message. I
also posted a v2 patch for the sysfs change (cut-paste issue).
I'll hold off on any other changes until you have had a chance to
review all the others.
Regards,
Hartley
On Wed, May 09, 2012 at 11:41:26AM -0500, H Hartley Sweeten wrote:
> On Wednesday, May 09, 2012 8:57 AM, [email protected] wrote:
> > On Wed, May 09, 2012 at 10:52:34AM -0500, H Hartley Sweeten wrote:
> >> Based on the discussion I assume this patch is ok then. Do you
> >> want me to repost it?
> >
> > Sure, I'll review it with the other patches you have sent later today.
>
> Greg,
>
> I have re-posted the patch with an updated commit message. I
> also posted a v2 patch for the sysfs change (cut-paste issue).
>
> I'll hold off on any other changes until you have had a chance to
> review all the others.
I've applied all of your patches, with the exception of the v2 version
of this patch, see my comments on it for why.
thanks,
greg k-h
On 2012-05-09 15:19, Dan Carpenter wrote:
> On Wed, May 09, 2012 at 12:01:25PM +0100, Ian Abbott wrote:
>> Are there any handy macros for casting pointers to __user pointers,
>> something like
>>
>> #define _user(p) ((typeof(*(p)) __user *)(p))
>>
>> but preferably without the repeated expansion of 'p' in case of
>> side-effects?
>
> typeof() doesn't have side effects.
>
> #include<stdio.h>
>
> int main(void)
> {
> int x = 0;
> typeof(x++) y;
>
> printf("%d\n", x);
>
> return 0;
> }
>
> regards,
> dan carpenter
Yes, you're correct of course. I was unnecessarily worried about the
double-expansion of the macro parameter but it's safe in this case.
BTW, the intention of this macro is so you can write things like:
copy_from_user(data, _user(insn.data), insn.n * sizeof(insn.data[0]));
instead of:
copy_from_user(data, (unsigned int __user *)insn.data, insn.n *
sizeof(insn.data[0]));
where the member insn.data is a 'unsigned int *' but is used for both
__user pointers and kernel pointers.
--
-=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-