Crash on unplug with the uvc driver in linuxtv/staging/for_v3.1

Crash on unplug with the uvc driver in linuxtv/staging/for_v3.1

[Hans de Goede]

Hi,

When I unplug a uvc camera *while streaming* I get:

Jun  9 13:20:02 shalem kernel: [15824.809741] BUG: unable to handle kernel NULL pointer dereference at           (null)
Jun  9 13:20:02 shalem kernel: [15824.809816] IP: [<ffffffffa0309eae>] media_entity_put+0x12/0x2c [media]
Jun  9 13:20:02 shalem kernel: [15824.809877] PGD 0
Jun  9 13:20:02 shalem kernel: [15824.809898] Oops: 0000 [#1] SMP
Jun  9 13:20:02 shalem kernel: [15824.809933] CPU 1
Jun  9 13:20:02 shalem kernel: [15824.809952] Modules linked in: uvcvideo videodev media v4l2_compat_ioctl32 nf_conntrack_ipv4 nf_defrag_ipv4 vfat fat tcp_lp tun fuse ebtable_nat ebtables cpufreq_ondemand acpi_cpufreq freq_table mperf bridge stp llc be2iscsi iscsi_boot_sysfs bnx2i cnic uio cxgb3i libcxgbi iw_cxgb3 cxgb3 mdio ib_iser rdma_cm ib_cm iw_cm ib_sa ib_mad ib_core ib_addr iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi ip6t_REJECT nf_conntrack_ipv6 coretemp xt_physdev nf_defrag_ipv6 ip6table_filter xt_state ip6_tables nf_conntrack snd_hda_codec_hdmi snd_hda_codec_conexant 
snd_hda_intel snd_hda_codec snd_bt87x usb_storage snd_seq snd_usb_audio uas snd_pcm snd_hwdep snd_usbmidi_lib ppdev snd_rawmidi microcode e1000e snd_seq_device serio_raw i2c_i801 snd_timer tpm_infineon snd iTCO_wdt parport_pc parport soundcore mei(C) iTCO_vendor_support snd_page_alloc virtio_net kvm_intel kvm ipv6 i915 drm_kms_helper drm i2c_algo_bit i2c_core video [last unloaded: tuner_types]
Jun  9 13:20:02 shalem kernel: [15824.810794]
Jun  9 13:20:02 shalem kernel: [15824.810811] Pid: 4944, comm: camorama Tainted: G         C  3.0.0-rc1+ #5 FUJITSU D3071-S1/D3071-S1
Jun  9 13:20:02 shalem kernel: [15824.810888] RIP: 0010:[<ffffffffa0309eae>]  [<ffffffffa0309eae>] media_entity_put+0x12/0x2c [media]
Jun  9 13:20:02 shalem kernel: [15824.810961] RSP: 0018:ffff88011da23d98  EFLAGS: 00010286
Jun  9 13:20:02 shalem kernel: [15824.811003] RAX: 0000000000000000 RBX: ffff88006a864400 RCX: 0000000000000118
Jun  9 13:20:02 shalem kernel: [15824.811057] RDX: 0000000000000001 RSI: 0000000000000004 RDI: ffff88006a864400
Jun  9 13:20:02 shalem kernel: [15824.811112] RBP: ffff88011da23d98 R08: ffffea0002896e28 R09: ffff88011da23d38
Jun  9 13:20:02 shalem kernel: [15824.811165] R10: ffffffffa0526026 R11: ffffffff81a58210 R12: ffff880027d43840
Jun  9 13:20:02 shalem kernel: [15824.811219] R13: 0000000000000008 R14: ffff880133310dc0 R15: ffff88012db76300
Jun  9 13:20:02 shalem kernel: [15824.811274] FS:  00007f7942ff49c0(0000) GS:ffff88013e280000(0000) knlGS:0000000000000000
Jun  9 13:20:02 shalem kernel: [15824.811336] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
Jun  9 13:20:02 shalem kernel: [15824.811381] CR2: 0000000000000000 CR3: 0000000001a03000 CR4: 00000000000406e0
Jun  9 13:20:02 shalem kernel: [15824.811435] DR0: 0000000000000003 DR1: 00000000000000b0 DR2: 0000000000000001
Jun  9 13:20:02 shalem kernel: [15824.811491] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Jun  9 13:20:02 shalem kernel: [15824.811546] Process camorama (pid: 4944, threadinfo ffff88011da22000, task ffff880113618000)
Jun  9 13:20:02 shalem kernel: [15824.811609] Stack:
Jun  9 13:20:02 shalem kernel: [15824.811629]  ffff88011da23db8 ffffffffa0510203 ffff880027d43840 ffff880133310dc0
Jun  9 13:20:02 shalem kernel: [15824.811695]  ffff88011da23e08 ffffffff811233fe ffff880027d43850 ffff880134530500
Jun  9 13:20:02 shalem kernel: [15824.811763]  ffffffff811e3397 ffff880027d43840 ffff8801347a5b80 0000000000000000
Jun  9 13:20:02 shalem kernel: [15824.811829] Call Trace:
Jun  9 13:20:02 shalem kernel: [15824.811856]  [<ffffffffa0510203>] v4l2_release+0x7b/0x8e [videodev]
Jun  9 13:20:02 shalem kernel: [15824.811910]  [<ffffffff811233fe>] fput+0x121/0x1e3
Jun  9 13:20:02 shalem kernel: [15824.811953]  [<ffffffff811e3397>] ? exit_sem+0x1c7/0x1d8
Jun  9 13:20:02 shalem kernel: [15824.811998]  [<ffffffff811208a7>] filp_close+0x6e/0x7a
Jun  9 13:20:02 shalem kernel: [15824.812041]  [<ffffffff81131b33>] ? __d_free+0x53/0x58
Jun  9 13:20:02 shalem kernel: [15824.812084]  [<ffffffff810567e1>] put_files_struct+0x6e/0xd5
Jun  9 13:20:02 shalem kernel: [15824.812130]  [<ffffffff810568d5>] exit_files+0x41/0x46
Jun  9 13:20:02 shalem kernel: [15824.812172]  [<ffffffff81056e55>] do_exit+0x2aa/0x738
Jun  9 13:20:02 shalem kernel: [15824.812214]  [<ffffffff8104be73>] ? wake_up_state+0x10/0x12
Jun  9 13:20:02 shalem kernel: [15824.812259]  [<ffffffff81062ba2>] ? signal_wake_up+0x32/0x43
Jun  9 13:20:02 shalem kernel: [15824.812304]  [<ffffffff810638b4>] ? zap_other_threads+0x59/0x82
Jun  9 13:20:02 shalem kernel: [15824.812352]  [<ffffffff81057568>] do_group_exit+0x7a/0xa2
Jun  9 13:20:02 shalem kernel: [15824.812395]  [<ffffffff810575a7>] sys_exit_group+0x17/0x17
Jun  9 13:20:02 shalem kernel: [15824.812440]  [<ffffffff81487802>] system_call_fastpath+0x16/0x1b
Jun  9 13:20:02 shalem kernel: [15824.812486] Code: 10 66 41 ff 44 24 40 48 83 c4 18 44 89 f0 5b 41 5c 41 5d 41 5e 41 5f 5d c3 55 48 89 e5 66 66 66 66 90 48 85 ff 74 1c 48 8b 47 10
Jun  9 13:20:02 shalem kernel: [15824.812783] RIP  [<ffffffffa0309eae>] media_entity_put+0x12/0x2c [media]
Jun  9 13:20:02 shalem kernel: [15824.812839]  RSP <ffff88011da23d98>
Jun  9 13:20:02 shalem kernel: [15824.812867] CR2: 0000000000000000
Jun  9 13:20:02 shalem kernel: [15824.873494] ---[ end trace bfc278787db8cbfb ]---
Jun  9 13:20:02 shalem kernel: [15824.873496] Fixing recursive fault but reboot is needed!

I've not tested if this also impacts 3.0!!

I also get the following during building linuxtv/staging/for_v3.1:

   CC [M]  drivers/media/video/uvc/uvc_entity.o
drivers/media/video/uvc/uvc_entity.c: In function ‘uvc_mc_register_entities’:
drivers/media/video/uvc/uvc_entity.c:110:6: warning: ‘ret’ may be used uninitialized in this function [-Wuninitialized]

Regards,

Hans

Re: Crash on unplug with the uvc driver in linuxtv/staging/for_v3.1

[Laurent Pinchart]

Hi Hans,

On Thursday 09 June 2011 13:22:03 Hans de Goede wrote:
> Hi,
> 
> When I unplug a uvc camera *while streaming* I get:
> 
> [15824.809741] BUG: unable to handle kernel NULL pointer dereference at          
> (null)

[snip]

> I've not tested if this also impacts 3.0!!

It probably does. Thanks for the report. I'll fix it.

> I also get the following during building linuxtv/staging/for_v3.1:
> 
>    CC [M]  drivers/media/video/uvc/uvc_entity.o
> drivers/media/video/uvc/uvc_entity.c: In function
> ‘uvc_mc_register_entities’: drivers/media/video/uvc/uvc_entity.c:110:6:
> warning: ‘ret’ may be used uninitialized in this function
> [-Wuninitialized]

Mauro sent a patch to fix that.

-- 
Regards,

Laurent Pinchart

Re: Crash on unplug with the uvc driver in linuxtv/staging/for_v3.1

[Laurent Pinchart]

Hi Hans (and Hans),

On Saturday 11 June 2011 11:16:10 Laurent Pinchart wrote:
> On Thursday 09 June 2011 13:22:03 Hans de Goede wrote:
> > Hi,
> > 
> > When I unplug a uvc camera *while streaming* I get:
> > 
> > [15824.809741] BUG: unable to handle kernel NULL pointer dereference at
> > (null)
> 
> [snip]
> 
> > I've not tested if this also impacts 3.0!!
> 
> It probably does. Thanks for the report. I'll fix it.

It does. Fixing the problem turns to be more complex than expected.

The crash is caused by media entities life time management issues.

Entities associated with video device nodes are unregistered in 
video_unregister_device(). This removes the entity from its parent's entities 
list, and sets the entity's parent to NULL.

Entities also get/put references to their parent's module through 
media_entity_get() and media_entity_put(). Those functions are called in the 
open and release handlers of video device nodes and subdev device nodes. The 
reason behind this is to avoid a parent module from being removed while a 
subdev is opened, as closing a subdev can call to the parent's module through 
board code.

When a UVC device is unplugged while streaming, the uvcvideo driver will call 
video_unregister_device() in the disconnect handler. This will in turn call 
media_device_unregister_entity() which sets the entity's parent to NULL. When 
the user then closes open video device nodes, v4l2_release() calls 
media_entity_put() which tries to dereference entity->parent, and oopses.

I've tried to move the media_device_unregister_entity() call from 
video_unregister_device() to v4l2_device_release() (called when the last 
reference to the video device is released). media_entity_put() is then called 
before the entity is unregistered, but that results in a different oops: as 
this happens after the USB disconnect callback is called, entity->parent->dev-
>driver is now NULL, and trying to access entity->parent->dev->driver->owner 
to decrement the module use count oopses.

One possible workaround is to remove media_entity_get()/media_entity_put() 
calls from v4l2-dev.c. As the original purpose of those functions was to avoid 
a parent module from being removed while still accessible through board code, 
and all existing MC-enabled drivers register video device nodes with the owner 
equal to the entity's parent's module, we can safely do it.

I'd rather implement a proper solution though, but that's not straightforward. 
We short-circuit the kernel reference management by going through board code. 
There's something fundamentally wrong in the way we manage subdevs and 
device/module reference counts. I'm not sure where the proper fix should go to 
though.

-- 
Regards,

Laurent Pinchart

Re: Crash on unplug with the uvc driver in linuxtv/staging/for_v3.1

[Hans Verkuil]

On Monday, June 13, 2011 11:41:42 Laurent Pinchart wrote:
> Hi Hans (and Hans),
> 
> On Saturday 11 June 2011 11:16:10 Laurent Pinchart wrote:
> > On Thursday 09 June 2011 13:22:03 Hans de Goede wrote:
> > > Hi,
> > > 
> > > When I unplug a uvc camera *while streaming* I get:
> > > 
> > > [15824.809741] BUG: unable to handle kernel NULL pointer dereference at
> > > (null)
> > 
> > [snip]
> > 
> > > I've not tested if this also impacts 3.0!!
> > 
> > It probably does. Thanks for the report. I'll fix it.
> 
> It does. Fixing the problem turns to be more complex than expected.
> 
> The crash is caused by media entities life time management issues.
> 
> Entities associated with video device nodes are unregistered in 
> video_unregister_device(). This removes the entity from its parent's entities 
> list, and sets the entity's parent to NULL.
> 
> Entities also get/put references to their parent's module through 
> media_entity_get() and media_entity_put(). Those functions are called in the 
> open and release handlers of video device nodes and subdev device nodes. The 
> reason behind this is to avoid a parent module from being removed while a 
> subdev is opened, as closing a subdev can call to the parent's module through 
> board code.
> 
> When a UVC device is unplugged while streaming, the uvcvideo driver will call 
> video_unregister_device() in the disconnect handler. This will in turn call 
> media_device_unregister_entity() which sets the entity's parent to NULL. When 
> the user then closes open video device nodes, v4l2_release() calls 
> media_entity_put() which tries to dereference entity->parent, and oopses.
> 
> I've tried to move the media_device_unregister_entity() call from 
> video_unregister_device() to v4l2_device_release() (called when the last 
> reference to the video device is released). media_entity_put() is then called 
> before the entity is unregistered, but that results in a different oops: as 
> this happens after the USB disconnect callback is called, entity->parent->dev-
> >driver is now NULL, and trying to access entity->parent->dev->driver->owner 
> to decrement the module use count oopses.
> 
> One possible workaround is to remove media_entity_get()/media_entity_put() 
> calls from v4l2-dev.c. As the original purpose of those functions was to avoid 
> a parent module from being removed while still accessible through board code, 
> and all existing MC-enabled drivers register video device nodes with the owner 
> equal to the entity's parent's module, we can safely do it.
> 
> I'd rather implement a proper solution though, but that's not straightforward. 
> We short-circuit the kernel reference management by going through board code. 
> There's something fundamentally wrong in the way we manage subdevs and 
> device/module reference counts. I'm not sure where the proper fix should go to 
> though.

Hmm. Tricky.

media_device_unregister_entity() is definitely called in the wrong place. It
should move to v4l2_device_release(). I wonder why media_entity_get/put are
called in v4l2_open and v4l2_release. That should be in __video_register_device
and v4l2_device_release as far as I can see.

Just closing a device node shouldn't be cause for changing the module's refcount,
that device node registration and the release after unregistration.

I also wonder whether instead of refcounting the module in media_entity_get/put
you should refcount the device (entity->parent->dev).

I have to admit that I don't quite understand why the USB disconnect zeroes
entity->parent->dev->driver.

I hope this gives some ideas...

Regards,

	Hans

Re: Crash on unplug with the uvc driver in linuxtv/staging/for_v3.1

[Laurent Pinchart]

Hi Hans,

On Monday 13 June 2011 13:10:57 Hans Verkuil wrote:
> On Monday, June 13, 2011 11:41:42 Laurent Pinchart wrote:
> > On Saturday 11 June 2011 11:16:10 Laurent Pinchart wrote:
> > > On Thursday 09 June 2011 13:22:03 Hans de Goede wrote:
> > > > Hi,
> > > > 
> > > > When I unplug a uvc camera *while streaming* I get:
> > > > 
> > > > [15824.809741] BUG: unable to handle kernel NULL pointer dereference
> > > > at (null)
> > > 
> > > [snip]
> > > 
> > > > I've not tested if this also impacts 3.0!!
> > > 
> > > It probably does. Thanks for the report. I'll fix it.
> > 
> > It does. Fixing the problem turns to be more complex than expected.
> > 
> > The crash is caused by media entities life time management issues.
> > 
> > Entities associated with video device nodes are unregistered in
> > video_unregister_device(). This removes the entity from its parent's
> > entities list, and sets the entity's parent to NULL.
> > 
> > Entities also get/put references to their parent's module through
> > media_entity_get() and media_entity_put(). Those functions are called in
> > the open and release handlers of video device nodes and subdev device
> > nodes. The reason behind this is to avoid a parent module from being
> > removed while a subdev is opened, as closing a subdev can call to the
> > parent's module through board code.
> > 
> > When a UVC device is unplugged while streaming, the uvcvideo driver will
> > call video_unregister_device() in the disconnect handler. This will in
> > turn call media_device_unregister_entity() which sets the entity's
> > parent to NULL. When the user then closes open video device nodes,
> > v4l2_release() calls media_entity_put() which tries to dereference
> > entity->parent, and oopses.
> > 
> > I've tried to move the media_device_unregister_entity() call from
> > video_unregister_device() to v4l2_device_release() (called when the last
> > reference to the video device is released). media_entity_put() is then
> > called before the entity is unregistered, but that results in a
> > different oops: as this happens after the USB disconnect callback is
> > called, entity->parent->dev-
> > 
> > >driver is now NULL, and trying to access
> > >entity->parent->dev->driver->owner
> > 
> > to decrement the module use count oopses.
> > 
> > One possible workaround is to remove
> > media_entity_get()/media_entity_put() calls from v4l2-dev.c. As the
> > original purpose of those functions was to avoid a parent module from
> > being removed while still accessible through board code, and all
> > existing MC-enabled drivers register video device nodes with the owner
> > equal to the entity's parent's module, we can safely do it.
> > 
> > I'd rather implement a proper solution though, but that's not
> > straightforward. We short-circuit the kernel reference management by
> > going through board code. There's something fundamentally wrong in the
> > way we manage subdevs and device/module reference counts. I'm not sure
> > where the proper fix should go to though.
> 
> Hmm. Tricky.
> 
> media_device_unregister_entity() is definitely called in the wrong place.
> It should move to v4l2_device_release().

Agreed.

> I wonder why media_entity_get/put are called in v4l2_open and v4l2_release.
> That should be in __video_register_device and v4l2_device_release as far as
> I can see.
> 
> Just closing a device node shouldn't be cause for changing the module's
> refcount, that device node registration and the release after
> unregistration.

If you moved media_entity_get/put in the registration and unregistration 
functions, removing the uvcvideo module would become completely impossible if 
a driver is plugged in. uvcvideo registers a video device node, which would 
then increase the refcount on the module and lock it in in memory until the 
device is unplugged.

> I also wonder whether instead of refcounting the module in
> media_entity_get/put you should refcount the device (entity->parent->dev).

Increasing the device parent won't prevent the module from being unloaded. You 
can unload a module even if devices are bound to it.

> I have to admit that I don't quite understand why the USB disconnect zeroes
> entity->parent->dev->driver.
> 
> I hope this gives some ideas...

The easiest workaround is still to remove the media_entity_get()/put() calls 
from v4l2-dev.c. Those function have been introduced to fix a subdev-specific 
problem and are used for video nodes as well, but they're not needed there for 
now. This isn't a long-term fix though.

-- 
Regards,

Laurent Pinchart

[PATCH] v4l: Don't access media entity after is has been destroyed

[Laurent Pinchart]

Entities associated with video device nodes are unregistered in
video_unregister_device(). This destroys the entity even though it can
still be accessed through open video device nodes.

Move the media_device_unregister_entity() call from
video_unregister_device() to v4l2_device_release() to ensure that the
entity isn't unregistered until the last reference to the video device
is released.

Also remove the media_entity_get()/put() calls from v4l2-dev.c. Those
functions were designed for subdevs, to avoid a parent module from being
removed while still accessible through board code. They're not currently
needed for video device nodes, and will oops when a hotpluggable device
is disconnected during streaming, as media_entity_put() called in
v4l2_device_release() tries to access entity->parent->dev->driver which
is set to NULL when the device is disconnected.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/v4l2-dev.c |   36 +++++++-----------------------------
 1 files changed, 7 insertions(+), 29 deletions(-)

diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 19d5ae2..5dc1fa1 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -167,6 +167,12 @@ static void v4l2_device_release(struct device *cd)
 
 	mutex_unlock(&videodev_lock);
 
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	if (vdev->v4l2_dev && vdev->v4l2_dev->mdev &&
+	    vdev->vfl_type != VFL_TYPE_SUBDEV)
+		media_device_unregister_entity(&vdev->entity);
+#endif
+
 	/* Release video_device and perform other
 	   cleanups as needed. */
 	vdev->release(vdev);
@@ -405,17 +411,6 @@ static int v4l2_open(struct inode *inode, struct file *filp)
 	/* and increase the device refcount */
 	video_get(vdev);
 	mutex_unlock(&videodev_lock);
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	if (vdev->v4l2_dev && vdev->v4l2_dev->mdev &&
-	    vdev->vfl_type != VFL_TYPE_SUBDEV) {
-		entity = media_entity_get(&vdev->entity);
-		if (!entity) {
-			ret = -EBUSY;
-			video_put(vdev);
-			return ret;
-		}
-	}
-#endif
 	if (vdev->fops->open) {
 		if (vdev->lock && mutex_lock_interruptible(vdev->lock)) {
 			ret = -ERESTARTSYS;
@@ -431,14 +426,8 @@ static int v4l2_open(struct inode *inode, struct file *filp)
 
 err:
 	/* decrease the refcount in case of an error */
-	if (ret) {
-#if defined(CONFIG_MEDIA_CONTROLLER)
-		if (vdev->v4l2_dev && vdev->v4l2_dev->mdev &&
-		    vdev->vfl_type != VFL_TYPE_SUBDEV)
-			media_entity_put(entity);
-#endif
+	if (ret)
 		video_put(vdev);
-	}
 	return ret;
 }
 
@@ -455,11 +444,6 @@ static int v4l2_release(struct inode *inode, struct file *filp)
 		if (vdev->lock)
 			mutex_unlock(vdev->lock);
 	}
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	if (vdev->v4l2_dev && vdev->v4l2_dev->mdev &&
-	    vdev->vfl_type != VFL_TYPE_SUBDEV)
-		media_entity_put(&vdev->entity);
-#endif
 	/* decrease the refcount unconditionally since the release()
 	   return value is ignored. */
 	video_put(vdev);
@@ -754,12 +738,6 @@ void video_unregister_device(struct video_device *vdev)
 	if (!vdev || !video_is_registered(vdev))
 		return;
 
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	if (vdev->v4l2_dev && vdev->v4l2_dev->mdev &&
-	    vdev->vfl_type != VFL_TYPE_SUBDEV)
-		media_device_unregister_entity(&vdev->entity);
-#endif
-
 	mutex_lock(&videodev_lock);
 	/* This must be in a critical section to prevent a race with v4l2_open.
 	 * Once this bit has been cleared video_get may never be called again.
-- 
1.7.3.4

Re: [PATCH] v4l: Don't access media entity after is has been destroyed

[Laurent Pinchart]

Mauro,

I forgot to mention in the subject line that this fix should go to 3.0. It 
fixes a kernel oops when a UVC webcam is disconnected during streaming.

I'll send a pull request in the next few days after getting an ack on this 
patch.

Hans, could you please test if it fixes your issue ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] v4l: Don't access media entity after is has been destroyed

[Sakari Ailus]

Laurent Pinchart wrote:
> Entities associated with video device nodes are unregistered in
> video_unregister_device(). This destroys the entity even though it can
> still be accessed through open video device nodes.
>
> Move the media_device_unregister_entity() call from
> video_unregister_device() to v4l2_device_release() to ensure that the
> entity isn't unregistered until the last reference to the video device
> is released.
>
> Also remove the media_entity_get()/put() calls from v4l2-dev.c. Those
> functions were designed for subdevs, to avoid a parent module from being
> removed while still accessible through board code. They're not currently
> needed for video device nodes, and will oops when a hotpluggable device
> is disconnected during streaming, as media_entity_put() called in
> v4l2_device_release() tries to access entity->parent->dev->driver which
> is set to NULL when the device is disconnected.

Thanks for the patch, Laurent!

Acked-by: Sakari Ailus <sakari.ailus@iki.fi>

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCH] v4l: Don't access media entity after is has been destroyed

[Hans Verkuil]

On Wednesday, June 15, 2011 10:36:26 Laurent Pinchart wrote:
> Entities associated with video device nodes are unregistered in
> video_unregister_device(). This destroys the entity even though it can
> still be accessed through open video device nodes.
> 
> Move the media_device_unregister_entity() call from
> video_unregister_device() to v4l2_device_release() to ensure that the
> entity isn't unregistered until the last reference to the video device
> is released.
> 
> Also remove the media_entity_get()/put() calls from v4l2-dev.c. Those
> functions were designed for subdevs, to avoid a parent module from being
> removed while still accessible through board code. They're not currently
> needed for video device nodes, and will oops when a hotpluggable device
> is disconnected during streaming, as media_entity_put() called in
> v4l2_device_release() tries to access entity->parent->dev->driver which
> is set to NULL when the device is disconnected.

Looks good.

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/video/v4l2-dev.c |   36 +++++++-----------------------------
>  1 files changed, 7 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
> index 19d5ae2..5dc1fa1 100644
> --- a/drivers/media/video/v4l2-dev.c
> +++ b/drivers/media/video/v4l2-dev.c
> @@ -167,6 +167,12 @@ static void v4l2_device_release(struct device *cd)
>  
>  	mutex_unlock(&videodev_lock);
>  
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +	if (vdev->v4l2_dev && vdev->v4l2_dev->mdev &&
> +	    vdev->vfl_type != VFL_TYPE_SUBDEV)
> +		media_device_unregister_entity(&vdev->entity);
> +#endif
> +
>  	/* Release video_device and perform other
>  	   cleanups as needed. */
>  	vdev->release(vdev);
> @@ -405,17 +411,6 @@ static int v4l2_open(struct inode *inode, struct file 
*filp)
>  	/* and increase the device refcount */
>  	video_get(vdev);
>  	mutex_unlock(&videodev_lock);
> -#if defined(CONFIG_MEDIA_CONTROLLER)
> -	if (vdev->v4l2_dev && vdev->v4l2_dev->mdev &&
> -	    vdev->vfl_type != VFL_TYPE_SUBDEV) {
> -		entity = media_entity_get(&vdev->entity);
> -		if (!entity) {
> -			ret = -EBUSY;
> -			video_put(vdev);
> -			return ret;
> -		}
> -	}
> -#endif
>  	if (vdev->fops->open) {
>  		if (vdev->lock && mutex_lock_interruptible(vdev->lock)) {
>  			ret = -ERESTARTSYS;
> @@ -431,14 +426,8 @@ static int v4l2_open(struct inode *inode, struct file 
*filp)
>  
>  err:
>  	/* decrease the refcount in case of an error */
> -	if (ret) {
> -#if defined(CONFIG_MEDIA_CONTROLLER)
> -		if (vdev->v4l2_dev && vdev->v4l2_dev->mdev &&
> -		    vdev->vfl_type != VFL_TYPE_SUBDEV)
> -			media_entity_put(entity);
> -#endif
> +	if (ret)
>  		video_put(vdev);
> -	}
>  	return ret;
>  }
>  
> @@ -455,11 +444,6 @@ static int v4l2_release(struct inode *inode, struct 
file *filp)
>  		if (vdev->lock)
>  			mutex_unlock(vdev->lock);
>  	}
> -#if defined(CONFIG_MEDIA_CONTROLLER)
> -	if (vdev->v4l2_dev && vdev->v4l2_dev->mdev &&
> -	    vdev->vfl_type != VFL_TYPE_SUBDEV)
> -		media_entity_put(&vdev->entity);
> -#endif
>  	/* decrease the refcount unconditionally since the release()
>  	   return value is ignored. */
>  	video_put(vdev);
> @@ -754,12 +738,6 @@ void video_unregister_device(struct video_device *vdev)
>  	if (!vdev || !video_is_registered(vdev))
>  		return;
>  
> -#if defined(CONFIG_MEDIA_CONTROLLER)
> -	if (vdev->v4l2_dev && vdev->v4l2_dev->mdev &&
> -	    vdev->vfl_type != VFL_TYPE_SUBDEV)
> -		media_device_unregister_entity(&vdev->entity);
> -#endif
> -
>  	mutex_lock(&videodev_lock);
>  	/* This must be in a critical section to prevent a race with v4l2_open.
>  	 * Once this bit has been cleared video_get may never be called again.
> -- 
> 1.7.3.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
>