* [PATCH 1/2] [media] dib0700: Drop useless check when remote key is pressed
@ 2012-03-13 17:50 Jean Delvare
2012-03-13 17:57 ` Devin Heitmueller
0 siblings, 1 reply; 6+ messages in thread
From: Jean Delvare @ 2012-03-13 17:50 UTC (permalink / raw)
To: LMML; +Cc: Mauro Carvalho Chehab, Devin Heitmueller
struct dvb_usb_device *d can never be NULL so don't waste time
checking for this.
Rationale: the urb's context is set when usb_fill_bulk_urb() is called
in dib0700_rc_setup(), and never changes after that. d is dereferenced
unconditionally in dib0700_rc_setup() so it can't be NULL or the
driver would crash right away.
Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
Cc: Devin Heitmueller <dheitmueller@kernellabs.com>
---
Devin, am I missing something?
drivers/media/dvb/dvb-usb/dib0700_core.c | 3 ---
1 file changed, 3 deletions(-)
--- linux-3.3-rc7.orig/drivers/media/dvb/dvb-usb/dib0700_core.c 2012-03-13 11:09:13.000000000 +0100
+++ linux-3.3-rc7/drivers/media/dvb/dvb-usb/dib0700_core.c 2012-03-13 18:37:05.785953845 +0100
@@ -677,9 +677,6 @@ static void dib0700_rc_urb_completion(st
u8 toggle;
deb_info("%s()\n", __func__);
- if (d == NULL)
- return;
-
if (d->rc_dev == NULL) {
/* This will occur if disable_rc_polling=1 */
usb_free_urb(purb);
--
Jean Delvare
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] [media] dib0700: Drop useless check when remote key is pressed 2012-03-13 17:50 [PATCH 1/2] [media] dib0700: Drop useless check when remote key is pressed Jean Delvare @ 2012-03-13 17:57 ` Devin Heitmueller 2012-03-19 22:26 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 6+ messages in thread From: Devin Heitmueller @ 2012-03-13 17:57 UTC (permalink / raw) To: Jean Delvare; +Cc: LMML, Mauro Carvalho Chehab On Tue, Mar 13, 2012 at 1:50 PM, Jean Delvare <khali@linux-fr.org> wrote: > struct dvb_usb_device *d can never be NULL so don't waste time > checking for this. > > Rationale: the urb's context is set when usb_fill_bulk_urb() is called > in dib0700_rc_setup(), and never changes after that. d is dereferenced > unconditionally in dib0700_rc_setup() so it can't be NULL or the > driver would crash right away. > > Signed-off-by: Jean Delvare <khali@linux-fr.org> > Cc: Mauro Carvalho Chehab <mchehab@infradead.org> > Cc: Devin Heitmueller <dheitmueller@kernellabs.com> > --- > Devin, am I missing something? I think this was just a case of defensive coding where I didn't want to dereference something without validating the pointer first (out of fear that it got called through some other code path that I didn't consider). Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] [media] dib0700: Drop useless check when remote key is pressed 2012-03-13 17:57 ` Devin Heitmueller @ 2012-03-19 22:26 ` Mauro Carvalho Chehab 2012-03-20 7:20 ` Jean Delvare 0 siblings, 1 reply; 6+ messages in thread From: Mauro Carvalho Chehab @ 2012-03-19 22:26 UTC (permalink / raw) To: Devin Heitmueller; +Cc: Jean Delvare, LMML, Mauro Carvalho Chehab Em 13-03-2012 14:57, Devin Heitmueller escreveu: > On Tue, Mar 13, 2012 at 1:50 PM, Jean Delvare <khali@linux-fr.org> wrote: >> struct dvb_usb_device *d can never be NULL so don't waste time >> checking for this. >> >> Rationale: the urb's context is set when usb_fill_bulk_urb() is called >> in dib0700_rc_setup(), and never changes after that. d is dereferenced >> unconditionally in dib0700_rc_setup() so it can't be NULL or the >> driver would crash right away. >> >> Signed-off-by: Jean Delvare <khali@linux-fr.org> >> Cc: Mauro Carvalho Chehab <mchehab@infradead.org> >> Cc: Devin Heitmueller <dheitmueller@kernellabs.com> >> --- >> Devin, am I missing something? > > I think this was just a case of defensive coding where I didn't want > to dereference something without validating the pointer first (out of > fear that it got called through some other code path that I didn't > consider). >> --- linux-3.3-rc7.orig/drivers/media/dvb/dvb-usb/dib0700_core.c 2012-03-13 11:09:13.000000000 +0100 >> +++ linux-3.3-rc7/drivers/media/dvb/dvb-usb/dib0700_core.c 2012-03-13 18:37:05.785953845 +0100 >> @@ -677,9 +677,6 @@ static void dib0700_rc_urb_completion(st >> u8 toggle; >> >> deb_info("%s()\n", __func__); >> - if (d == NULL) >> - return; >> - Well, usb_free_urb() is not called when d == NULL, so, if this condition ever happens, it will keep URB's allocated. Anyway, if struct dvb_usb_device *d is NULL, the driver has something very wrong happening on it, and nothing will work on it. I agree with Jean: it is better to just remove this code there. Yet, I'd be more happy if Jean's patch could check first if the status is below 0, in order to prevent a possible race condition at device disconnect. Regards, Mauro ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] [media] dib0700: Drop useless check when remote key is pressed 2012-03-19 22:26 ` Mauro Carvalho Chehab @ 2012-03-20 7:20 ` Jean Delvare 2012-03-20 12:17 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 6+ messages in thread From: Jean Delvare @ 2012-03-20 7:20 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: Devin Heitmueller, LMML, Mauro Carvalho Chehab Hi Mauro, On Mon, 19 Mar 2012 19:26:11 -0300, Mauro Carvalho Chehab wrote: > On Tue, Mar 13, 2012 at 1:50 PM, Jean Delvare <khali@linux-fr.org> wrote: > > --- linux-3.3-rc7.orig/drivers/media/dvb/dvb-usb/dib0700_core.c 2012-03-13 11:09:13.000000000 +0100 > > +++ linux-3.3-rc7/drivers/media/dvb/dvb-usb/dib0700_core.c 2012-03-13 18:37:05.785953845 +0100 > > @@ -677,9 +677,6 @@ static void dib0700_rc_urb_completion(st > > u8 toggle; > > > > deb_info("%s()\n", __func__); > > - if (d == NULL) > > - return; > > - > > Well, usb_free_urb() is not called when d == NULL, so, if this condition > ever happens, it will keep URB's allocated. > > Anyway, if struct dvb_usb_device *d is NULL, the driver has something very > wrong happening on it, and nothing will work on it. > > I agree with Jean: it is better to just remove this code there. > > Yet, I'd be more happy if Jean's patch could check first if the status is > below 0, in order to prevent a possible race condition at device disconnect. I'm not sure I see the race condition you're seeing. Do you believe purb->context would be NULL (or point to already-freed memory) when dib0700_rc_urb_completion is called as part of device disconnect? Or is it something else? I'll be happy to resubmit my patch series with a fix if you explain where you think there is a race condition. -- Jean Delvare ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] [media] dib0700: Drop useless check when remote key is pressed 2012-03-20 7:20 ` Jean Delvare @ 2012-03-20 12:17 ` Mauro Carvalho Chehab 2012-03-21 20:09 ` Jean Delvare 0 siblings, 1 reply; 6+ messages in thread From: Mauro Carvalho Chehab @ 2012-03-20 12:17 UTC (permalink / raw) To: Jean Delvare; +Cc: Devin Heitmueller, LMML, Mauro Carvalho Chehab Em 20-03-2012 04:20, Jean Delvare escreveu: > Hi Mauro, > > On Mon, 19 Mar 2012 19:26:11 -0300, Mauro Carvalho Chehab wrote: >> On Tue, Mar 13, 2012 at 1:50 PM, Jean Delvare <khali@linux-fr.org> wrote: >>> --- linux-3.3-rc7.orig/drivers/media/dvb/dvb-usb/dib0700_core.c 2012-03-13 11:09:13.000000000 +0100 >>> +++ linux-3.3-rc7/drivers/media/dvb/dvb-usb/dib0700_core.c 2012-03-13 18:37:05.785953845 +0100 >>> @@ -677,9 +677,6 @@ static void dib0700_rc_urb_completion(st >>> u8 toggle; >>> >>> deb_info("%s()\n", __func__); >>> - if (d == NULL) >>> - return; >>> - >> >> Well, usb_free_urb() is not called when d == NULL, so, if this condition >> ever happens, it will keep URB's allocated. >> >> Anyway, if struct dvb_usb_device *d is NULL, the driver has something very >> wrong happening on it, and nothing will work on it. >> >> I agree with Jean: it is better to just remove this code there. >> >> Yet, I'd be more happy if Jean's patch could check first if the status is >> below 0, in order to prevent a possible race condition at device disconnect. > > I'm not sure I see the race condition you're seeing. Do you believe > purb->context would be NULL (or point to already-freed memory) when > dib0700_rc_urb_completion is called as part of device disconnect? Or is > it something else? I'll be happy to resubmit my patch series with a fix > if you explain where you think there is a race condition. > What I'm saying is that the only potential chance of having a NULL value for d is at the device disconnect/removal, if is there any bug when waiting for the URB's to be killed. So, it would be better to invert the error test logic to: static void dib0700_rc_urb_completion(struct urb *purb) { struct dvb_usb_device *d = purb->context; struct dib0700_rc_response *poll_reply; u32 uninitialized_var(keycode); u8 toggle; poll_reply = purb->transfer_buffer; if (purb->status < 0) { deb_info("discontinuing polling\n"); kfree(purb->transfer_buffer); usb_free_urb(purb); return; } deb_info("%s()\n", __func__); if (d->rc_dev == NULL) { /* This will occur if disable_rc_polling=1 */ kfree(purb->transfer_buffer); usb_free_urb(purb); return; } As, at device disconnect/completion, the status will indicate an error, and the function will return before trying to de-referenciate rc_dev. Regards, Mauro ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] [media] dib0700: Drop useless check when remote key is pressed 2012-03-20 12:17 ` Mauro Carvalho Chehab @ 2012-03-21 20:09 ` Jean Delvare 0 siblings, 0 replies; 6+ messages in thread From: Jean Delvare @ 2012-03-21 20:09 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: Devin Heitmueller, LMML, Mauro Carvalho Chehab Hi Mauro, On Tue, 20 Mar 2012 09:17:54 -0300, Mauro Carvalho Chehab wrote: > Em 20-03-2012 04:20, Jean Delvare escreveu: > > On Mon, 19 Mar 2012 19:26:11 -0300, Mauro Carvalho Chehab wrote: > >> Yet, I'd be more happy if Jean's patch could check first if the status is > >> below 0, in order to prevent a possible race condition at device disconnect. > > > > I'm not sure I see the race condition you're seeing. Do you believe > > purb->context would be NULL (or point to already-freed memory) when > > dib0700_rc_urb_completion is called as part of device disconnect? Or is > > it something else? I'll be happy to resubmit my patch series with a fix > > if you explain where you think there is a race condition. > > What I'm saying is that the only potential chance of having a NULL value > for d is at the device disconnect/removal, if is there any bug when waiting > for the URB's to be killed. > > So, it would be better to invert the error test logic to: > > static void dib0700_rc_urb_completion(struct urb *purb) > { > struct dvb_usb_device *d = purb->context; > struct dib0700_rc_response *poll_reply; > u32 uninitialized_var(keycode); > u8 toggle; > > poll_reply = purb->transfer_buffer; > if (purb->status < 0) { > deb_info("discontinuing polling\n"); > kfree(purb->transfer_buffer); > usb_free_urb(purb); > return; > } > > deb_info("%s()\n", __func__); > if (d->rc_dev == NULL) { > /* This will occur if disable_rc_polling=1 */ > kfree(purb->transfer_buffer); > usb_free_urb(purb); > return; > } > > As, at device disconnect/completion, the status will indicate an error, and > the function will return before trying to de-referenciate rc_dev. Hmm. I couldn't find any code that would reset purb->context. I tested 2000 rmmod dvb-usb-dib0700 on a 3.3.0 kernel with my two patches applied, compiled with CONFIG_DEBUG_SLAB=y and CONFIG_DEBUG_VM=y, and it did not crash nor report any problem. I don't think there is any race here, so I see no point in changing the code. We just got rid of a paranoid check, it is not to apply another paranoid patch. -- Jean Delvare ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-03-21 20:09 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-03-13 17:50 [PATCH 1/2] [media] dib0700: Drop useless check when remote key is pressed Jean Delvare 2012-03-13 17:57 ` Devin Heitmueller 2012-03-19 22:26 ` Mauro Carvalho Chehab 2012-03-20 7:20 ` Jean Delvare 2012-03-20 12:17 ` Mauro Carvalho Chehab 2012-03-21 20:09 ` Jean Delvare
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.