* staging: vc04_services: Need suggestions on trying to fix sparse warning in vchiq_arm.c
@ 2021-06-01 20:05 Ojaswin Mujoo
2021-06-01 20:23 ` Dan Carpenter
0 siblings, 1 reply; 7+ messages in thread
From: Ojaswin Mujoo @ 2021-06-01 20:05 UTC (permalink / raw)
To: nsaenz
Cc: gregkh, arnd, dan.carpenter, phil, bcm-kernel-feedback-list,
linux-arm-kernel, linux-staging, linux-kernel
Hello,
I was trying to address the following TODO item in
"drivers/staging/vc04_services/interface/TODO" and would love some comments and
suggestions on this:
> 14) Clean up Sparse warnings from __user annotations. See
> vchiq_irq_queue_bulk_tx_rx(). Ensure that the address of "&waiter->bulk_waiter"
> is never disclosed to userspace.
More specifically, I was looking at ways to fix the following sparse warning:
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:1001:26:
warning: incorrect type in assignment (different address spaces)
expected void *[assigned] userdata
got void [noderef] __user *userdata
From my understanding, the issue seems to be that the (void *)userdata local
variable in vchiq_irq_queue_bulk_tx_rx() can be assigned a (void __user *) or a
(struct bulk_waiter *). This makes the assignment tricky since it can either be
a userspace pointer or a kernel pointer.
Right now, we are just ignoring the sparse warning which is essentially
resulting in the __user attribute being lost when we assign args->userdata to
userdata. This can be dangerous as it might result in illegal access of
userspace memory, without any sparse warning, down the line.
Further, this issue seems to boil down to the fact that the (void *)userdata
field in struct vchiq_bulk (in vc04_services/interface/vchiq_arm/vchiq_core.h)
can, again, either be a (stuct bulk_waiter *) or a (void __user *). To fix this,
I was playing with the idea of modifying this userdata field of struct
vchiq_bulk to be something like the following:
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
b/ drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -227,10 +227,16 @@ enum vchiq_bulk_dir {
typedef void (*vchiq_userdata_term)(void *userdata);
+struct vchiq_userdata_or_waiter {
+ void __user *userdata;
+ struct bulk_waiter *bulk_waiter;
+
+};
+
struct vchiq_bulk {
short mode;
short dir;
- void *userdata;
+ struct vchiq_userdata_or_waiter *userdata_or_waiter;
dma_addr_t data;
int size;
void *remote_data;
I was then planning on modifying all the code that works with
vchiq_bulk->userdata accordingly. I believe this can help us overcome the sparse
warnings, as well as preserve the __user attribute in cases when the userspace
pointer does get assigned here.
However, since I'm not very familiar with the codebase, I just wanted to confirm
if this is an acceptable workaround and to make sure I'm not breaking anything
or overlooked anything here. I noticed that we also want to make sure that
bulk_waiter's address is not exposed to userspace. Will it be possible to provide
some pointers on how/where this might happen, so I can see if I can try to extend
this patch to avoid that.
I would love to hear you suggestions and thoughts on this.
PS: I'm new here so please do correct me incase I missed anything.
Regards,
Ojaswin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: staging: vc04_services: Need suggestions on trying to fix sparse warning in vchiq_arm.c 2021-06-01 20:05 staging: vc04_services: Need suggestions on trying to fix sparse warning in vchiq_arm.c Ojaswin Mujoo @ 2021-06-01 20:23 ` Dan Carpenter 2021-06-02 14:50 ` Ojaswin Mujoo 0 siblings, 1 reply; 7+ messages in thread From: Dan Carpenter @ 2021-06-01 20:23 UTC (permalink / raw) To: Ojaswin Mujoo Cc: nsaenz, gregkh, arnd, phil, bcm-kernel-feedback-list, linux-arm-kernel, linux-staging, linux-kernel The problem is not the Sparse warning, the problem is that this code is a mess. It used to very clearly buggy and I reported the bug. I think Arnd found the bug again independently and fixed it. A couple weeks ago Al Viro looked at this code. Here is his write up: https://www.spinics.net/lists/kernel/msg3952745.html It shouldn't take Al Viro dozens of pages of detailed analysis to try figure out if the code is safe or not. Your idea silences the warning but would make the code even more subtle and complicated. The right thing to do is to re-write the code to be simpler. regards, dan carpenter _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: staging: vc04_services: Need suggestions on trying to fix sparse warning in vchiq_arm.c 2021-06-01 20:23 ` Dan Carpenter @ 2021-06-02 14:50 ` Ojaswin Mujoo 2021-06-04 6:13 ` Stefan Wahren 0 siblings, 1 reply; 7+ messages in thread From: Ojaswin Mujoo @ 2021-06-02 14:50 UTC (permalink / raw) To: Dan Carpenter Cc: nsaenz, gregkh, arnd, phil, bcm-kernel-feedback-list, linux-arm-kernel, linux-staging, linux-kernel On Tue, Jun 01, 2021 at 11:23:07PM +0300, Dan Carpenter wrote: > The problem is not the Sparse warning, the problem is that this code is > a mess. It used to very clearly buggy and I reported the bug. I think > Arnd found the bug again independently and fixed it. > > A couple weeks ago Al Viro looked at this code. Here is his write up: > > https://www.spinics.net/lists/kernel/msg3952745.html > > It shouldn't take Al Viro dozens of pages of detailed analysis to try > figure out if the code is safe or not. Your idea silences the warning > but would make the code even more subtle and complicated. > > The right thing to do is to re-write the code to be simpler. > > regards, > dan carpenter > Thank you for the prompt reply and the link, it was very insightful. You are right, I was definitely going about this the wrong way and missing the larger picture. I'll spend some time trying to understand this codebase as I think that'd be a good start to understand how stuff works in the kernel (even though some of the things in this driver are anti patterns) and hopefully get some ideas on ways to clean this up. Anyways, thanks again for the help, cheers! Ojaswin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: staging: vc04_services: Need suggestions on trying to fix sparse warning in vchiq_arm.c 2021-06-02 14:50 ` Ojaswin Mujoo @ 2021-06-04 6:13 ` Stefan Wahren 2021-06-05 7:23 ` Ojaswin Mujoo 0 siblings, 1 reply; 7+ messages in thread From: Stefan Wahren @ 2021-06-04 6:13 UTC (permalink / raw) To: Ojaswin Mujoo, Dan Carpenter Cc: nsaenz, gregkh, arnd, phil, bcm-kernel-feedback-list, linux-arm-kernel, linux-staging, linux-kernel Hi Ojaswin, Am 02.06.21 um 16:50 schrieb Ojaswin Mujoo: > On Tue, Jun 01, 2021 at 11:23:07PM +0300, Dan Carpenter wrote: >> The problem is not the Sparse warning, the problem is that this code is >> a mess. It used to very clearly buggy and I reported the bug. I think >> Arnd found the bug again independently and fixed it. >> >> A couple weeks ago Al Viro looked at this code. Here is his write up: >> >> https://www.spinics.net/lists/kernel/msg3952745.html >> >> It shouldn't take Al Viro dozens of pages of detailed analysis to try >> figure out if the code is safe or not. Your idea silences the warning >> but would make the code even more subtle and complicated. >> >> The right thing to do is to re-write the code to be simpler. >> >> regards, >> dan carpenter >> > Thank you for the prompt reply and the link, it was very insightful. You > are right, I was definitely going about this the wrong way and missing > the larger picture. I'll spend some time trying to understand this > codebase as I think that'd be a good start to understand how stuff works in > the kernel (even though some of the things in this driver are anti patterns) > and hopefully get some ideas on ways to clean this up. > > Anyways, thanks again for the help, cheers! thanks for your interest in cleaning this up. Yes, it's not clear which points on the TODO list are the lower hanging fruits. In case you don't want to fix checkpatch issues, maybe you can look at points 8, 9, 10, 12 and 13. Most of them require testing with a Raspberry Pi, but feel free to ask if you have problems with it. Regards Stefan > > Ojaswin > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: staging: vc04_services: Need suggestions on trying to fix sparse warning in vchiq_arm.c 2021-06-04 6:13 ` Stefan Wahren @ 2021-06-05 7:23 ` Ojaswin Mujoo 2021-06-05 8:41 ` Stefan Wahren 0 siblings, 1 reply; 7+ messages in thread From: Ojaswin Mujoo @ 2021-06-05 7:23 UTC (permalink / raw) To: Stefan Wahren Cc: Dan Carpenter, nsaenz, gregkh, arnd, phil, bcm-kernel-feedback-list, linux-arm-kernel, linux-staging, linux-kernel On Fri, Jun 04, 2021 at 08:13:06AM +0200, Stefan Wahren wrote: > Hi Ojaswin, > > Am 02.06.21 um 16:50 schrieb Ojaswin Mujoo: > > On Tue, Jun 01, 2021 at 11:23:07PM +0300, Dan Carpenter wrote: > >> The problem is not the Sparse warning, the problem is that this code is > >> a mess. It used to very clearly buggy and I reported the bug. I think > >> Arnd found the bug again independently and fixed it. > >> > >> A couple weeks ago Al Viro looked at this code. Here is his write up: > >> > >> https://www.spinics.net/lists/kernel/msg3952745.html > >> > >> It shouldn't take Al Viro dozens of pages of detailed analysis to try > >> figure out if the code is safe or not. Your idea silences the warning > >> but would make the code even more subtle and complicated. > >> > >> The right thing to do is to re-write the code to be simpler. > >> > >> regards, > >> dan carpenter > >> > > Thank you for the prompt reply and the link, it was very insightful. You > > are right, I was definitely going about this the wrong way and missing > > the larger picture. I'll spend some time trying to understand this > > codebase as I think that'd be a good start to understand how stuff works in > > the kernel (even though some of the things in this driver are anti patterns) > > and hopefully get some ideas on ways to clean this up. > > > > Anyways, thanks again for the help, cheers! > > thanks for your interest in cleaning this up. Yes, it's not clear which > points on the TODO list are the lower hanging fruits. In case you don't > want to fix checkpatch issues, maybe you can look at points 8, 9, 10, 12 > and 13. Most of them require testing with a Raspberry Pi, but feel free > to ask if you have problems with it. > > Regards > Stefan > Got it, Task 10 (cdev to its own file) seems like a pretty good task to get started with. I'm planning to buy a Rpi 4 so I think I can run tests on that. Thank you so much for the help, I'll get back incase I face any issues down the line. Regards, Ojaswin > > > > Ojaswin > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: staging: vc04_services: Need suggestions on trying to fix sparse warning in vchiq_arm.c 2021-06-05 7:23 ` Ojaswin Mujoo @ 2021-06-05 8:41 ` Stefan Wahren 2021-06-06 9:26 ` Ojaswin Mujoo 0 siblings, 1 reply; 7+ messages in thread From: Stefan Wahren @ 2021-06-05 8:41 UTC (permalink / raw) To: Ojaswin Mujoo Cc: Dan Carpenter, nsaenz, gregkh, arnd, phil, bcm-kernel-feedback-list, linux-arm-kernel, linux-staging, linux-kernel Hi Ojaswin, Am 05.06.21 um 09:23 schrieb Ojaswin Mujoo: > On Fri, Jun 04, 2021 at 08:13:06AM +0200, Stefan Wahren wrote: >> Hi Ojaswin, >> >> Am 02.06.21 um 16:50 schrieb Ojaswin Mujoo: >>> On Tue, Jun 01, 2021 at 11:23:07PM +0300, Dan Carpenter wrote: >>>> The problem is not the Sparse warning, the problem is that this code is >>>> a mess. It used to very clearly buggy and I reported the bug. I think >>>> Arnd found the bug again independently and fixed it. >>>> >>>> A couple weeks ago Al Viro looked at this code. Here is his write up: >>>> >>>> https://www.spinics.net/lists/kernel/msg3952745.html >>>> >>>> It shouldn't take Al Viro dozens of pages of detailed analysis to try >>>> figure out if the code is safe or not. Your idea silences the warning >>>> but would make the code even more subtle and complicated. >>>> >>>> The right thing to do is to re-write the code to be simpler. >>>> >>>> regards, >>>> dan carpenter >>>> >>> Thank you for the prompt reply and the link, it was very insightful. You >>> are right, I was definitely going about this the wrong way and missing >>> the larger picture. I'll spend some time trying to understand this >>> codebase as I think that'd be a good start to understand how stuff works in >>> the kernel (even though some of the things in this driver are anti patterns) >>> and hopefully get some ideas on ways to clean this up. >>> >>> Anyways, thanks again for the help, cheers! >> thanks for your interest in cleaning this up. Yes, it's not clear which >> points on the TODO list are the lower hanging fruits. In case you don't >> want to fix checkpatch issues, maybe you can look at points 8, 9, 10, 12 >> and 13. Most of them require testing with a Raspberry Pi, but feel free >> to ask if you have problems with it. >> >> Regards >> Stefan >> > Got it, Task 10 (cdev to its own file) seems like a pretty good task to > get started with. I'm planning to buy a Rpi 4 so I think I can run tests > on that. okay, but the AFAIK the vchiq driver in the mainline kernel doesn't work with Rpi 4 yet. The Raspberry Pi 3 B Plus is currently the recommend devel platform, so you can test 32 and 64 bit kernel. Best regards > > Thank you so much for the help, I'll get back incase I face any issues down > the line. > > Regards, > Ojaswin > >>> Ojaswin >>> >>> _______________________________________________ >>> linux-arm-kernel mailing list >>> linux-arm-kernel@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: staging: vc04_services: Need suggestions on trying to fix sparse warning in vchiq_arm.c 2021-06-05 8:41 ` Stefan Wahren @ 2021-06-06 9:26 ` Ojaswin Mujoo 0 siblings, 0 replies; 7+ messages in thread From: Ojaswin Mujoo @ 2021-06-06 9:26 UTC (permalink / raw) To: Stefan Wahren Cc: Dan Carpenter, nsaenz, gregkh, arnd, phil, bcm-kernel-feedback-list, linux-arm-kernel, linux-staging, linux-kernel On Sat, Jun 05, 2021 at 10:41:17AM +0200, Stefan Wahren wrote: > Hi Ojaswin, > > Am 05.06.21 um 09:23 schrieb Ojaswin Mujoo: > > On Fri, Jun 04, 2021 at 08:13:06AM +0200, Stefan Wahren wrote: > >> Hi Ojaswin, > >> > >> Am 02.06.21 um 16:50 schrieb Ojaswin Mujoo: > >>> On Tue, Jun 01, 2021 at 11:23:07PM +0300, Dan Carpenter wrote: > >>>> The problem is not the Sparse warning, the problem is that this code is > >>>> a mess. It used to very clearly buggy and I reported the bug. I think > >>>> Arnd found the bug again independently and fixed it. > >>>> > >>>> A couple weeks ago Al Viro looked at this code. Here is his write up: > >>>> > >>>> https://www.spinics.net/lists/kernel/msg3952745.html > >>>> > >>>> It shouldn't take Al Viro dozens of pages of detailed analysis to try > >>>> figure out if the code is safe or not. Your idea silences the warning > >>>> but would make the code even more subtle and complicated. > >>>> > >>>> The right thing to do is to re-write the code to be simpler. > >>>> > >>>> regards, > >>>> dan carpenter > >>>> > >>> Thank you for the prompt reply and the link, it was very insightful. You > >>> are right, I was definitely going about this the wrong way and missing > >>> the larger picture. I'll spend some time trying to understand this > >>> codebase as I think that'd be a good start to understand how stuff works in > >>> the kernel (even though some of the things in this driver are anti patterns) > >>> and hopefully get some ideas on ways to clean this up. > >>> > >>> Anyways, thanks again for the help, cheers! > >> thanks for your interest in cleaning this up. Yes, it's not clear which > >> points on the TODO list are the lower hanging fruits. In case you don't > >> want to fix checkpatch issues, maybe you can look at points 8, 9, 10, 12 > >> and 13. Most of them require testing with a Raspberry Pi, but feel free > >> to ask if you have problems with it. > >> > >> Regards > >> Stefan > >> > > Got it, Task 10 (cdev to its own file) seems like a pretty good task to > > get started with. I'm planning to buy a Rpi 4 so I think I can run tests > > on that. > > okay, but the AFAIK the vchiq driver in the mainline kernel doesn't work > with Rpi 4 yet. The Raspberry Pi 3 B Plus is currently the recommend > devel platform, so you can test 32 and 64 bit kernel. > > Best regards Hey Stefan, Ahh got it. sure I can get a 3B+ instead, thanks for the heads up. > > > > > Thank you so much for the help, I'll get back incase I face any issues down > > the line. > > > > Regards, > > Ojaswin > > > >>> Ojaswin > >>> > >>> _______________________________________________ > >>> linux-arm-kernel mailing list > >>> linux-arm-kernel@lists.infradead.org > >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > Regards, Ojaswin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-06-06 9:28 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-01 20:05 staging: vc04_services: Need suggestions on trying to fix sparse warning in vchiq_arm.c Ojaswin Mujoo 2021-06-01 20:23 ` Dan Carpenter 2021-06-02 14:50 ` Ojaswin Mujoo 2021-06-04 6:13 ` Stefan Wahren 2021-06-05 7:23 ` Ojaswin Mujoo 2021-06-05 8:41 ` Stefan Wahren 2021-06-06 9:26 ` Ojaswin Mujoo
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).