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

PATCH Superseded H Hartley Sweeten 4 participants 13 messages May 8, 2012 – May 10, 2012 Staging

Share & tools

H Hartley Sweeten

author

2012-05-08 23:41 UTC #0 lore raw

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

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.

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 {

H Hartley Sweeten

author

2012-05-08 23:56 UTC #1 lore raw

Quoted 18 lines · 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 <[email protected]>
Cc Ian Abbott <[email protected]>
Cc Mori Hess <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>

Quoted 4 lines

---
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

Ian Abbott

2012-05-09 10:20 UTC #2 lore raw

Quoted 19 lines · H Hartley Sweeten

Quoted 18 lines · 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<[email protected]>
Cc Ian Abbott<[email protected]>
Cc Mori Hess<[email protected]>
Cc Greg Kroah-Hartman<[email protected]>

Quoted 8 lines

Quoted 4 lines
---
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.

Signature · 2 lines

-=( Ian Abbott @ MEV Ltd.    E-mail: <[email protected]>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Dan Carpenter

2012-05-09 10:28 UTC #3 lore raw

Quoted 25 lines · Ian Abbott

Quoted 24 lines · H Hartley Sweeten
Quoted 22 lines · 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<[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.

Quoted 7 lines

Quoted 3 lines
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

Ian Abbott

2012-05-09 11:01 UTC #4 lore raw

Quoted 27 lines · Dan Carpenter

Quoted 18 lines · Ian Abbott
Quoted 17 lines · H Hartley Sweeten
Quoted 15 lines · 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
Note: This patch exposes some new warnings about different address space. These will be addressed.
Quoted 7 lines
Quoted 3 lines
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.

Quoted 6 lines

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?

Signature · 2 lines

-=( Ian Abbott @ MEV Ltd.    E-mail: <[email protected]>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Dan Carpenter

2012-05-09 14:16 UTC #5 lore raw

Quoted 7 lines · Ian Abbott

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

Ian Abbott

2012-05-10 11:05 UTC #6 lore raw

Quoted 23 lines · Dan Carpenter

Quoted 7 lines · Ian Abbott
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;
}

Quoted 2 lines

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.

Signature · 2 lines

-=( Ian Abbott @ MEV Ltd.    E-mail: <[email protected]>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

[email protected]

maintainer

2012-05-09 14:24 UTC #7 lore raw

In reply to Ian Abbott

Quoted 29 lines · Ian Abbott

Quoted 19 lines · H Hartley Sweeten
Quoted 18 lines · 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<[email protected]>
Cc Ian Abbott<[email protected]>
Cc Mori Hess<[email protected]>
Cc Greg Kroah-Hartman<[email protected]>
Quoted 8 lines
Quoted 4 lines
---
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.

Quoted 8 lines

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

H Hartley Sweeten

author

2012-05-09 15:52 UTC #8 lore raw

Quoted 39 lines · On Wednesday, May 09, 2012 7:25 AM, [email protected] wrote:

Quoted 29 lines · Ian Abbott
Quoted 19 lines · H Hartley Sweeten
Quoted 18 lines · 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<[email protected]>
Cc Ian Abbott<[email protected]>
Cc Mori Hess<[email protected]>
Cc Greg Kroah-Hartman<[email protected]>
Quoted 8 lines
Quoted 4 lines
---
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.
Quoted 8 lines
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.

Quoted 5 lines

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

[email protected]

maintainer

2012-05-09 15:57 UTC #9 lore raw

Quoted 43 lines · H Hartley Sweeten

Quoted 42 lines · On Wednesday, May 09, 2012 7:25 AM, [email protected] wrote:
Quoted 29 lines · Ian Abbott
Quoted 19 lines · H Hartley Sweeten
Quoted 18 lines · 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<[email protected]>
Cc Ian Abbott<[email protected]>
Cc Mori Hess<[email protected]>
Cc Greg Kroah-Hartman<[email protected]>
Quoted 8 lines
Quoted 4 lines
---
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.
Quoted 8 lines
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.

Quoted 7 lines

Quoted 2 lines
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.

Quoted 1 line

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

H Hartley Sweeten

author

2012-05-09 16:03 UTC #10 lore raw

Quoted 8 lines · On Wednesday, May 09, 2012 8:57 AM, [email protected] wrote:

Quoted 7 lines · H Hartley Sweeten
Quoted 5 lines · 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.

Quoted 6 lines

Quoted 4 lines
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!

Quoted 4 lines

Quoted 1 line
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

H Hartley Sweeten

author

2012-05-09 16:41 UTC #11 lore raw

In reply to [email protected]

Quoted 5 lines · On Wednesday, May 09, 2012 8:57 AM, [email protected] wrote:

Quoted 2 lines · H Hartley Sweeten
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

[email protected]

maintainer

2012-05-09 20:42 UTC #12 lore raw

Quoted 7 lines · H Hartley Sweeten

Quoted 5 lines · On Wednesday, May 09, 2012 8:57 AM, [email protected] wrote:
Quoted 2 lines · H Hartley Sweeten
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.

Quoted 7 lines

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