[PATCH] staging: comedi: remove __user annotation inside of struct's

[PATCH] staging: comedi: remove __user annotation inside of struct's

[H Hartley Sweeten]

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 <hsweeten@visionengravers.com>
Cc: Ian Abbott <abbotti@mev.co.uk>
Cc: Mori Hess <fmhess@users.sourceforge.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---

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 {

RE: [PATCH] staging: comedi: remove __user annotation inside of struct's

[H Hartley Sweeten]

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 <hsweeten@visionengravers.com>
> Cc: Ian Abbott <abbotti@mev.co.uk>
> Cc: Mori Hess <fmhess@users.sourceforge.net>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> ---
>
> 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

Re: [PATCH] staging: comedi: remove __user annotation inside of struct's

[Ian Abbott]

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<hsweeten@visionengravers.com>
>> Cc: Ian Abbott<abbotti@mev.co.uk>
>> Cc: Mori Hess<fmhess@users.sourceforge.net>
>> Cc: Greg Kroah-Hartman<gregkh@linuxfoundation.org>
>>
>> ---
>>
>> 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: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH] staging: comedi: remove __user annotation inside of struct's

[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<hsweeten@visionengravers.com>
> >>Cc: Ian Abbott<abbotti@mev.co.uk>
> >>Cc: Mori Hess<fmhess@users.sourceforge.net>
> >>Cc: Greg Kroah-Hartman<gregkh@linuxfoundation.org>
> >>
> >>---
> >>
> >>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

Re: [PATCH] staging: comedi: remove __user annotation inside of struct's

[Ian Abbott]

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: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH] staging: comedi: remove __user annotation inside of struct's

[Dan Carpenter]

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

Re: [PATCH] staging: comedi: remove __user annotation inside of struct's

[gregkh]

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<hsweeten@visionengravers.com>
> >>Cc: Ian Abbott<abbotti@mev.co.uk>
> >>Cc: Mori Hess<fmhess@users.sourceforge.net>
> >>Cc: Greg Kroah-Hartman<gregkh@linuxfoundation.org>
> >>
> >>---
> >>
> >>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

RE: [PATCH] staging: comedi: remove __user annotation inside of struct's

[H Hartley Sweeten]

On Wednesday, May 09, 2012 7:25 AM, gregkh@linuxfoundation.org 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<hsweeten@visionengravers.com>
>>>> Cc: Ian Abbott<abbotti@mev.co.uk>
>>>> Cc: Mori Hess<fmhess@users.sourceforge.net>
>>>> Cc: Greg Kroah-Hartman<gregkh@linuxfoundation.org>
>>>>
>>>> ---
>>>>
>>>> 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

Re: [PATCH] staging: comedi: remove __user annotation inside of struct's

[gregkh]

On Wed, May 09, 2012 at 10:52:34AM -0500, H Hartley Sweeten wrote:
> On Wednesday, May 09, 2012 7:25 AM, gregkh@linuxfoundation.org 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<hsweeten@visionengravers.com>
> >>>> Cc: Ian Abbott<abbotti@mev.co.uk>
> >>>> Cc: Mori Hess<fmhess@users.sourceforge.net>
> >>>> Cc: Greg Kroah-Hartman<gregkh@linuxfoundation.org>
> >>>>
> >>>> ---
> >>>>
> >>>> 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

RE: [PATCH] staging: comedi: remove __user annotation inside of struct's

[H Hartley Sweeten]

On Wednesday, May 09, 2012 8:57 AM, gregkh@linuxfoundation.org wrote:
> On Wed, May 09, 2012 at 10:52:34AM -0500, H Hartley Sweeten wrote:
>> On Wednesday, May 09, 2012 7:25 AM, gregkh@linuxfoundation.org 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

RE: [PATCH] staging: comedi: remove __user annotation inside of struct's

[H Hartley Sweeten]

On Wednesday, May 09, 2012 8:57 AM, gregkh@linuxfoundation.org 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

Re: [PATCH] staging: comedi: remove __user annotation inside of struct's

[gregkh]

On Wed, May 09, 2012 at 11:41:26AM -0500, H Hartley Sweeten wrote:
> On Wednesday, May 09, 2012 8:57 AM, gregkh@linuxfoundation.org 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

Re: [PATCH] staging: comedi: remove __user annotation inside of struct's

[Ian Abbott]

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: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-