Re: [PATCH 3/7] em28xx: Only deallocate struct em28xx after finishing all extensions

[Mauro Carvalho Chehab <m.chehab@samsung.com>]

Em Tue, 14 Jan 2014 21:48:16 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> 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@googlemail.com> 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