Em Tue, 14 Jan 2014 21:48:16 +0100
Frank Schäfer <fschaefer.oss@xxxxxxxxxxxxxx> escreveu:
> Am 14.01.2014 19:55, schrieb Mauro Carvalho Chehab:
> > Em Tue, 14 Jan 2014 19:13:00 +0100
> > Frank Schäfer <fschaefer.oss@xxxxxxxxxxxxxx> escreveu:
> >
...
> >> At first glance it seems there are at least 2 issues:
> >> 1.) use after freeing in v4l-extension (happens when the device is
> >> closed after the usb disconnect)
> > That's basically what this patch fixes. Both V4L2 and ALSA handles it
> > well, if you warn the subsystem that a device will be removed.
> >
> > If are there still any issues, we may add a kref_get() at device open,
> > an a kref_put() at device close on the affected sub-driver.
> Ok, I've tested it and I was right here.
> This is what happens when closing a disconnected device:
>
> [ 144.045691] usb 1-8: USB disconnect, device number 7
> [ 144.047387] em2765 #0: disconnecting em2765 #0 video
> [ 144.047403] em2765 #0: V4L2 device video1 deregistered
> [ 144.050197] em2765 #0: Deregistering snapshot button
> [ 144.058336] em2765 #0: Freeing device
> [ 147.525267] : em28xx_v4l2_close() called
> [ 147.525287] : em28xx_videodevice_release() called
>
>
> I will make some tests tomorrow, but here is a first suggestion how to
> fix this:
>
> Remove the kref_put() call from em28xx_v4l2_fini().
> Instead, add the following lines to em28xx_videodevice_release():
>
> if (dev->users == 0) {
> dev->users--; /* avoid multiple kref_put() calls when the
> devices are unregistered and no device is open */
> kref_put(&dev->ref, em28xx_free_device);
> }
>
> That should fix it.
What I actually did on version 2 (already submitted) is that it is calling
kref_get() at open, and kref_put() at close.
> Interestingly no oops happens. I will have to take a closer look at this
> tomorrow, but I suspect that the dev we obtain from struct file filp is
> an outdated copy of the original device struct.
Likely.
> If that would be true and no bad things can happen in the close()
> function we actually wouldn't need kref for the v4l extension at all.
Still, it will be writing on a deallocated buffer, and this can be
making some memory used by some other part of the Kernel dirty.
> Of course, the ideal solution would be, if we could just clear the
> device struct at the end of the usb disconnect handler, because we can
> be sure that the fini() functions have already made sure that dev isn't
> used anymore.
>
> Btw, what happens in em28xx-audio ?
> Does the ALSA core also allow to call the close() function when the
> device is already gone ?
I suspect so. This is what happens when I remove HVR-950 while both
audio and video are still streaming:
[ 4313.540907] usb 3-4: USB disconnect, device number 7
[ 4313.541280] em2882/3 #0: Disconnecting em2882/3 #0
[ 4313.541313] em2882/3 #0: Closing video extension
[ 4313.541352] em2882/3 #0: V4L2 device vbi0 deregistered
[ 4313.541635] em2882/3 #0: V4L2 device video0 deregistered
[ 4313.542188] em2882/3 #0: Closing audio extension
(I waited for ~5 secs before removing the driver)
[ 4317.470747] em2882/3 #0: couldn't setup AC97 register 2
[ 4317.470772] em2882/3 #0: couldn't setup AC97 register 4
[ 4317.470785] em2882/3 #0: couldn't setup AC97 register 6
[ 4317.470797] em2882/3 #0: couldn't setup AC97 register 54
[ 4317.470810] em2882/3 #0: couldn't setup AC97 register 56
[ 4317.470950] em2882/3 #0: Closing DVB extension
[ 4317.471890] xc2028 19-0061: destroying instance
[ 4317.471913] em2882/3 #0: Closing input extension
[ 4317.489374] em2882/3 #0: Freeing device
As the code now have a kref for open/close on both audio and video
extensions, that means that em28xx close was called after device
removal, as otherwise, we won't see the "Freeing device" print.
--
Cheers,
Mauro
--
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
