* [PATCH] staging: comedi: cast to (unsigned int *) @ 2021-02-17 16:59 Atul Gopinathan 2021-02-17 17:35 ` Greg KH 0 siblings, 1 reply; 9+ messages in thread From: Atul Gopinathan @ 2021-02-17 16:59 UTC (permalink / raw) To: gregkh; +Cc: devel, abbotti, linux-kernel, Atul Gopinathan Resolve the following warning generated by sparse: drivers/staging//comedi/comedi_fops.c:2956:23: warning: incorrect type in assignment (different address spaces) drivers/staging//comedi/comedi_fops.c:2956:23: expected unsigned int *chanlist drivers/staging//comedi/comedi_fops.c:2956:23: got void [noderef] <asn:1> * compat_ptr() has a return type of "void __user *" as defined in "include/linux/compat.h" cmd->chanlist is of type "unsigned int *" as defined in drivers/staging/comedi/comedi.h" in struct comedi_cmd. Signed-off-by: Atul Gopinathan <atulgopinathan@gmail.com> --- drivers/staging/comedi/comedi_fops.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c index e85a99b68f31..fc4ec38012b4 100644 --- a/drivers/staging/comedi/comedi_fops.c +++ b/drivers/staging/comedi/comedi_fops.c @@ -2953,7 +2953,7 @@ static int get_compat_cmd(struct comedi_cmd *cmd, cmd->scan_end_arg = v32.scan_end_arg; cmd->stop_src = v32.stop_src; cmd->stop_arg = v32.stop_arg; - cmd->chanlist = compat_ptr(v32.chanlist); + cmd->chanlist = (unsigned int __force *)compat_ptr(v32.chanlist); cmd->chanlist_len = v32.chanlist_len; cmd->data = compat_ptr(v32.data); cmd->data_len = v32.data_len; -- 2.27.0 _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: comedi: cast to (unsigned int *) 2021-02-17 16:59 [PATCH] staging: comedi: cast to (unsigned int *) Atul Gopinathan @ 2021-02-17 17:35 ` Greg KH 2021-02-17 18:10 ` Atul Gopinathan 0 siblings, 1 reply; 9+ messages in thread From: Greg KH @ 2021-02-17 17:35 UTC (permalink / raw) To: Atul Gopinathan; +Cc: devel, abbotti, linux-kernel On Wed, Feb 17, 2021 at 10:29:08PM +0530, Atul Gopinathan wrote: > Resolve the following warning generated by sparse: > > drivers/staging//comedi/comedi_fops.c:2956:23: warning: incorrect type in assignment (different address spaces) > drivers/staging//comedi/comedi_fops.c:2956:23: expected unsigned int *chanlist > drivers/staging//comedi/comedi_fops.c:2956:23: got void [noderef] <asn:1> * > > compat_ptr() has a return type of "void __user *" > as defined in "include/linux/compat.h" > > cmd->chanlist is of type "unsigned int *" as defined > in drivers/staging/comedi/comedi.h" in struct > comedi_cmd. > > Signed-off-by: Atul Gopinathan <atulgopinathan@gmail.com> > --- > drivers/staging/comedi/comedi_fops.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c > index e85a99b68f31..fc4ec38012b4 100644 > --- a/drivers/staging/comedi/comedi_fops.c > +++ b/drivers/staging/comedi/comedi_fops.c > @@ -2953,7 +2953,7 @@ static int get_compat_cmd(struct comedi_cmd *cmd, > cmd->scan_end_arg = v32.scan_end_arg; > cmd->stop_src = v32.stop_src; > cmd->stop_arg = v32.stop_arg; > - cmd->chanlist = compat_ptr(v32.chanlist); > + cmd->chanlist = (unsigned int __force *)compat_ptr(v32.chanlist); __force? That feels wrong, something is odd if that is ever needed. Are you _sure_ this is correct? greg k-h _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: comedi: cast to (unsigned int *) 2021-02-17 17:35 ` Greg KH @ 2021-02-17 18:10 ` Atul Gopinathan 2021-02-17 18:26 ` Greg KH 0 siblings, 1 reply; 9+ messages in thread From: Atul Gopinathan @ 2021-02-17 18:10 UTC (permalink / raw) To: Greg KH; +Cc: devel, abbotti, linux-kernel On Wed, Feb 17, 2021 at 06:35:15PM +0100, Greg KH wrote: > On Wed, Feb 17, 2021 at 10:29:08PM +0530, Atul Gopinathan wrote: > > Resolve the following warning generated by sparse: > > > > drivers/staging//comedi/comedi_fops.c:2956:23: warning: incorrect type in assignment (different address spaces) > > drivers/staging//comedi/comedi_fops.c:2956:23: expected unsigned int *chanlist > > drivers/staging//comedi/comedi_fops.c:2956:23: got void [noderef] <asn:1> * > > > > compat_ptr() has a return type of "void __user *" > > as defined in "include/linux/compat.h" > > > > cmd->chanlist is of type "unsigned int *" as defined > > in drivers/staging/comedi/comedi.h" in struct > > comedi_cmd. > > > > Signed-off-by: Atul Gopinathan <atulgopinathan@gmail.com> > > --- > > drivers/staging/comedi/comedi_fops.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c > > index e85a99b68f31..fc4ec38012b4 100644 > > --- a/drivers/staging/comedi/comedi_fops.c > > +++ b/drivers/staging/comedi/comedi_fops.c > > @@ -2953,7 +2953,7 @@ static int get_compat_cmd(struct comedi_cmd *cmd, > > cmd->scan_end_arg = v32.scan_end_arg; > > cmd->stop_src = v32.stop_src; > > cmd->stop_arg = v32.stop_arg; > > - cmd->chanlist = compat_ptr(v32.chanlist); > > + cmd->chanlist = (unsigned int __force *)compat_ptr(v32.chanlist); > > __force? That feels wrong, something is odd if that is ever needed. > > Are you _sure_ this is correct? The same file has instances of "(usigned int __force *)" cast being used on the same "cmd->chanlist". For reference: At line 1797 of comedi_fops.c: 1796 /* restore chanlist pointer before copying back */ 1797 cmd->chanlist = (unsigned int __force *)user_chanlist; 1798 cmd->data = NULL; At line 1880: 1879 /* restore chanlist pointer before copying back */ 1880 cmd->chanlist = (unsigned int __force *)user_chanlist; 1881 *copy = true; Here "user_chanlist" is of type "unsigned int __user *". Or perhaps, I shouldn't be relying on them? Thanks! Atul _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: comedi: cast to (unsigned int *) 2021-02-17 18:10 ` Atul Gopinathan @ 2021-02-17 18:26 ` Greg KH 2021-02-18 11:04 ` Ian Abbott 0 siblings, 1 reply; 9+ messages in thread From: Greg KH @ 2021-02-17 18:26 UTC (permalink / raw) To: Atul Gopinathan, Ian Abbott; +Cc: devel, linux-kernel On Wed, Feb 17, 2021 at 11:40:00PM +0530, Atul Gopinathan wrote: > On Wed, Feb 17, 2021 at 06:35:15PM +0100, Greg KH wrote: > > On Wed, Feb 17, 2021 at 10:29:08PM +0530, Atul Gopinathan wrote: > > > Resolve the following warning generated by sparse: > > > > > > drivers/staging//comedi/comedi_fops.c:2956:23: warning: incorrect type in assignment (different address spaces) > > > drivers/staging//comedi/comedi_fops.c:2956:23: expected unsigned int *chanlist > > > drivers/staging//comedi/comedi_fops.c:2956:23: got void [noderef] <asn:1> * > > > > > > compat_ptr() has a return type of "void __user *" > > > as defined in "include/linux/compat.h" > > > > > > cmd->chanlist is of type "unsigned int *" as defined > > > in drivers/staging/comedi/comedi.h" in struct > > > comedi_cmd. > > > > > > Signed-off-by: Atul Gopinathan <atulgopinathan@gmail.com> > > > --- > > > drivers/staging/comedi/comedi_fops.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c > > > index e85a99b68f31..fc4ec38012b4 100644 > > > --- a/drivers/staging/comedi/comedi_fops.c > > > +++ b/drivers/staging/comedi/comedi_fops.c > > > @@ -2953,7 +2953,7 @@ static int get_compat_cmd(struct comedi_cmd *cmd, > > > cmd->scan_end_arg = v32.scan_end_arg; > > > cmd->stop_src = v32.stop_src; > > > cmd->stop_arg = v32.stop_arg; > > > - cmd->chanlist = compat_ptr(v32.chanlist); > > > + cmd->chanlist = (unsigned int __force *)compat_ptr(v32.chanlist); > > > > __force? That feels wrong, something is odd if that is ever needed. > > > > Are you _sure_ this is correct? > > The same file has instances of "(usigned int __force *)" cast being > used on the same "cmd->chanlist". For reference: > > At line 1797 of comedi_fops.c: > 1796 /* restore chanlist pointer before copying back */ > 1797 cmd->chanlist = (unsigned int __force *)user_chanlist; > 1798 cmd->data = NULL; > > At line 1880: > 1879 /* restore chanlist pointer before copying back */ > 1880 cmd->chanlist = (unsigned int __force *)user_chanlist; > 1881 *copy = true; > > Here "user_chanlist" is of type "unsigned int __user *". > > > Or perhaps, I shouldn't be relying on them? I don't know, it still feels wrong. Ian, any thoughts? thanks, greg k-h _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: comedi: cast to (unsigned int *) 2021-02-17 18:26 ` Greg KH @ 2021-02-18 11:04 ` Ian Abbott 2021-02-19 9:03 ` David Laight 0 siblings, 1 reply; 9+ messages in thread From: Ian Abbott @ 2021-02-18 11:04 UTC (permalink / raw) To: Greg KH, Atul Gopinathan; +Cc: devel, linux-kernel On 17/02/2021 18:26, Greg KH wrote: > On Wed, Feb 17, 2021 at 11:40:00PM +0530, Atul Gopinathan wrote: >> On Wed, Feb 17, 2021 at 06:35:15PM +0100, Greg KH wrote: >>> On Wed, Feb 17, 2021 at 10:29:08PM +0530, Atul Gopinathan wrote: >>>> Resolve the following warning generated by sparse: >>>> >>>> drivers/staging//comedi/comedi_fops.c:2956:23: warning: incorrect type in assignment (different address spaces) >>>> drivers/staging//comedi/comedi_fops.c:2956:23: expected unsigned int *chanlist >>>> drivers/staging//comedi/comedi_fops.c:2956:23: got void [noderef] <asn:1> * >>>> >>>> compat_ptr() has a return type of "void __user *" >>>> as defined in "include/linux/compat.h" >>>> >>>> cmd->chanlist is of type "unsigned int *" as defined >>>> in drivers/staging/comedi/comedi.h" in struct >>>> comedi_cmd. >>>> >>>> Signed-off-by: Atul Gopinathan <atulgopinathan@gmail.com> >>>> --- >>>> drivers/staging/comedi/comedi_fops.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c >>>> index e85a99b68f31..fc4ec38012b4 100644 >>>> --- a/drivers/staging/comedi/comedi_fops.c >>>> +++ b/drivers/staging/comedi/comedi_fops.c >>>> @@ -2953,7 +2953,7 @@ static int get_compat_cmd(struct comedi_cmd *cmd, >>>> cmd->scan_end_arg = v32.scan_end_arg; >>>> cmd->stop_src = v32.stop_src; >>>> cmd->stop_arg = v32.stop_arg; >>>> - cmd->chanlist = compat_ptr(v32.chanlist); >>>> + cmd->chanlist = (unsigned int __force *)compat_ptr(v32.chanlist); >>> >>> __force? That feels wrong, something is odd if that is ever needed. >>> >>> Are you _sure_ this is correct? >> >> The same file has instances of "(usigned int __force *)" cast being >> used on the same "cmd->chanlist". For reference: >> >> At line 1797 of comedi_fops.c: >> 1796 /* restore chanlist pointer before copying back */ >> 1797 cmd->chanlist = (unsigned int __force *)user_chanlist; >> 1798 cmd->data = NULL; >> >> At line 1880: >> 1879 /* restore chanlist pointer before copying back */ >> 1880 cmd->chanlist = (unsigned int __force *)user_chanlist; >> 1881 *copy = true; >> >> Here "user_chanlist" is of type "unsigned int __user *". >> >> >> Or perhaps, I shouldn't be relying on them? > > I don't know, it still feels wrong. > > Ian, any thoughts? It's kind of moot anyway because the patch is outdated. But the reason for the ___force is that the same `struct comedi_cmd` is used in both user and kernel contexts. In user contexts, the `chanlist` member points to user memory and in kernel contexts it points to kernel memory (copied from userspace). The sparse tagging of this member has flip-flopped a bit over the years: * commit 92d0127c9d24 ("Staging: comedi: __user markup on comedi_fops.c") (May 2010) tagged it as `__user`. * commit 9be56c643263 ("staging: comedi: comedi.h: remove __user tag from chanlist") (Sep 2012) removed the `__user` tag. It is mostly used in a kernel context, for example all the low-level drivers with `do_cmd` and `do_cmdtest` handlers use it in kernel context. The alternative would be to have a separate kernel version of this struct, but it would be mostly identical to the user version apart from the sparse tagging of this member and perhaps the removal of the unused `data` and `data_len` members (which need to be kept in the user version of the struct for compatibility reasons). -- -=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company )=- -=( registered in England & Wales. Regd. number: 02862268. )=- -=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=- -=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=- _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] staging: comedi: cast to (unsigned int *) 2021-02-18 11:04 ` Ian Abbott @ 2021-02-19 9:03 ` David Laight 2021-02-19 9:26 ` Dan Carpenter 2021-02-19 9:31 ` Ian Abbott 0 siblings, 2 replies; 9+ messages in thread From: David Laight @ 2021-02-19 9:03 UTC (permalink / raw) To: 'Ian Abbott', Greg KH, Atul Gopinathan; +Cc: devel, linux-kernel > It's kind of moot anyway because the patch is outdated. But the reason > for the ___force is that the same `struct comedi_cmd` is used in both > user and kernel contexts. In user contexts, the `chanlist` member > points to user memory and in kernel contexts it points to kernel memory > (copied from userspace). Can't you use a union of the user and kernel pointers? (Possibly even anonymous?) Although, ideally, keeping them in separate fields is better. 8 bytes for a pointer isn't going make a fat lot of difference. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: comedi: cast to (unsigned int *) 2021-02-19 9:03 ` David Laight @ 2021-02-19 9:26 ` Dan Carpenter 2021-02-19 9:36 ` David Laight 2021-02-19 9:31 ` Ian Abbott 1 sibling, 1 reply; 9+ messages in thread From: Dan Carpenter @ 2021-02-19 9:26 UTC (permalink / raw) To: David Laight Cc: devel, Greg KH, 'Ian Abbott', Atul Gopinathan, linux-kernel On Fri, Feb 19, 2021 at 09:03:59AM +0000, David Laight wrote: > > It's kind of moot anyway because the patch is outdated. But the reason > > for the ___force is that the same `struct comedi_cmd` is used in both > > user and kernel contexts. In user contexts, the `chanlist` member > > points to user memory and in kernel contexts it points to kernel memory > > (copied from userspace). > > Can't you use a union of the user and kernel pointers? > (Possibly even anonymous?) > Although, ideally, keeping them in separate fields is better. > 8 bytes for a pointer isn't going make a fat lot of difference. > Creating a union is worse than adding casts. With the casts, at least you know that you're doing something dangerous. It's good that it looks scary because it is scary. Keeping them in separate fields is a good idea, but this is part of the user space API so it's not possible. The best we can do is adding some more comments so people know why we are doing the scary casts. regards, dan carpenter _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] staging: comedi: cast to (unsigned int *) 2021-02-19 9:26 ` Dan Carpenter @ 2021-02-19 9:36 ` David Laight 0 siblings, 0 replies; 9+ messages in thread From: David Laight @ 2021-02-19 9:36 UTC (permalink / raw) To: 'Dan Carpenter' Cc: devel, Greg KH, 'Ian Abbott', Atul Gopinathan, linux-kernel From: Dan Carpenter > Sent: 19 February 2021 09:26 > > On Fri, Feb 19, 2021 at 09:03:59AM +0000, David Laight wrote: > > > It's kind of moot anyway because the patch is outdated. But the reason > > > for the ___force is that the same `struct comedi_cmd` is used in both > > > user and kernel contexts. In user contexts, the `chanlist` member > > > points to user memory and in kernel contexts it points to kernel memory > > > (copied from userspace). > > > > Can't you use a union of the user and kernel pointers? > > (Possibly even anonymous?) > > Although, ideally, keeping them in separate fields is better. > > 8 bytes for a pointer isn't going make a fat lot of difference. > > > > Creating a union is worse than adding casts. With the casts, at least > you know that you're doing something dangerous. It's good that it looks > scary because it is scary. > > Keeping them in separate fields is a good idea, but this is part of the > user space API so it's not possible. > > The best we can do is adding some more comments so people know why we > are doing the scary casts. Another option is to use a longer structure in the kernel with the kernel pointer in the 'extension'. So you could have: struct kernel_foo { struct foo; void *kernel_pointer; }; David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] staging: comedi: cast to (unsigned int *) 2021-02-19 9:03 ` David Laight 2021-02-19 9:26 ` Dan Carpenter @ 2021-02-19 9:31 ` Ian Abbott 1 sibling, 0 replies; 9+ messages in thread From: Ian Abbott @ 2021-02-19 9:31 UTC (permalink / raw) To: David Laight, Greg KH, Atul Gopinathan; +Cc: devel, linux-kernel On 19/02/2021 09:03, David Laight wrote: >> It's kind of moot anyway because the patch is outdated. But the reason >> for the ___force is that the same `struct comedi_cmd` is used in both >> user and kernel contexts. In user contexts, the `chanlist` member >> points to user memory and in kernel contexts it points to kernel memory >> (copied from userspace). > > Can't you use a union of the user and kernel pointers? > (Possibly even anonymous?) > Although, ideally, keeping them in separate fields is better. > 8 bytes for a pointer isn't going make a fat lot of difference. This is for a UAPI header (eventually), so cannot add a new field. For an anonymous union, one tagged with __user and one not, the __user tag would be removed during conversion from UAPI headers to /usr/include/linux headers, leaving a union of two identically typed members, which would look a bit odd. The union also kind of hides the problem. -- -=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company )=- -=( registered in England & Wales. Regd. number: 02862268. )=- -=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=- -=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=- _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-02-19 9:37 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-17 16:59 [PATCH] staging: comedi: cast to (unsigned int *) Atul Gopinathan 2021-02-17 17:35 ` Greg KH 2021-02-17 18:10 ` Atul Gopinathan 2021-02-17 18:26 ` Greg KH 2021-02-18 11:04 ` Ian Abbott 2021-02-19 9:03 ` David Laight 2021-02-19 9:26 ` Dan Carpenter 2021-02-19 9:36 ` David Laight 2021-02-19 9:31 ` Ian Abbott
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).