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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
