* [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.