Hi
On 14.01.20 03:31, Helen Koike wrote:
Hi Dafna,
On 1/13/20 7:55 PM, Dafna Hirschfeld wrote:
vb2_queue_release is called from vimc_cap_unregister.
`vb2_queue_release` stops the streaming in case
streaming is on and therefore it should be synchronized
with other streaming ioctls using the vdev's lock.
Currently the call is not synchronized and this cause
race conditions.
Using the following script:
while [ 1 ]; do
media-ctl -d platform:vimc -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480],"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
v4l2-ctl -d2 -v width=1920,height=1440
v4l2-ctl -d0 -v pixelformat=BA81
v4l2-ctl --stream-mmap -d /dev/video2 &
echo -n vimc.0 >/sys/bus/platform/drivers/vimc/unbind
echo -n vimc.0 >/sys/bus/platform/drivers/vimc/bind
done
The following crash appeared:
[ 101.909376] BUG: kernel NULL pointer dereference, address: 0000000000000009
[ 101.909661] #PF: supervisor read access in kernel mode
[ 101.909835] #PF: error_code(0x0000) - not-present page
[ 101.910048] PGD 0 P4D 0
[ 101.910223] Oops: 0000 [#1] SMP NOPTI
[ 101.910475] CPU: 0 PID: 1167 Comm: v4l2-ctl Not tainted 5.5.0-rc1+ #5
[ 101.910716] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
[ 101.911294] RIP: 0010:vb2_vmalloc_put_userptr+0x15/0x90 [videobuf2_vmalloc]
[ 101.911671] Code: 89 df 5b e9 0d e6 29 c6 0f 1f 00 66 2e 0f 1f 84 00 00 00 00 00 66 66 66 66 90 41 55 41 54 55 48 89 fd 53 4c 8b 65 08 48 8b 3f <41> 80 7c 24 09 00 75 65 48 81 e7 00 f0 ff ff 45 8b 6c 24 04 75 44
[ 101.912329] RSP: 0018:ffff9b0c42253df0 EFLAGS: 00000286
[ 101.912557] RAX: ffffffffc03bc1a0 RBX: ffff9095b37e1400 RCX: 0000000000000001
[ 101.912818] RDX: 0000000000000004 RSI: 0000000000000003 RDI: ffff9b0c4229d000
[ 101.913088] RBP: ffff9095b37d1480 R08: 0000000000000000 R09: ffff9b0c42253db8
[ 101.913352] R10: ffff9095b37df858 R11: ffff9095b3444b50 R12: 0000000000000000
[ 101.913598] R13: ffff9095b371c5b8 R14: 0000000000000004 R15: 0000000000000000
[ 101.913896] FS: 00007fe62d779240(0000) GS:ffff9095bfc00000(0000) knlGS:0000000000000000
[ 101.914202] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 101.914418] CR2: 0000000000000009 CR3: 0000000233392000 CR4: 00000000000006f0
[ 101.914738] Call Trace:
[ 101.915604] __vb2_queue_free+0xf8/0x210 [videobuf2_common]
[ 101.915876] vb2_core_queue_release+0x34/0x40 [videobuf2_common]
[ 101.916086] _vb2_fop_release+0x7d/0x90 [videobuf2_v4l2]
[ 101.916307] v4l2_release+0x9e/0xf0 [videodev]
[ 101.916499] __fput+0xb6/0x250
[ 101.916688] task_work_run+0x7e/0xa0
[ 101.916842] exit_to_usermode_loop+0xaa/0xb0
[ 101.917018] do_syscall_64+0x10b/0x160
[ 101.917175] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 101.917463] RIP: 0033:0x7fe62cf4c421
[ 101.917575] Code: f7 d8 64 89 02 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 8b 05 ea cf 20 00 85 c0 75 16 b8 03 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 3f f3 c3 0f 1f 44 00 00 53 89 fb 48 83 ec 10
Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx>
---
drivers/media/platform/vimc/vimc-capture.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index c5a645f98c66..314fda6db112 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -339,7 +339,9 @@ void vimc_cap_unregister(struct vimc_ent_device *ved)
struct vimc_cap_device *vcap =
container_of(ved, struct vimc_cap_device, ved);
+ mutex_lock(&vcap->lock);
vb2_queue_release(&vcap->queue);
+ mutex_unlock(&vcap->lock);
video_unregister_device(&vcap->vdev);
}
Thanks for the patch.
But now I'm wondering how other drivers do it, I didn't find any other driver
dealing with concurrency between unbinding and the ioctls like this.
hmm, I guess in many drivers it is not so important to handle the case
of userspace doing `echo blah-device >
/sys/bus/platform/drivers/blah-driver/unbind` but in vimc when hopefully
the configfs feature will be implemented it will be part of the API to
unregister with `echo 0 > configfs/vimc/dev/hotplug` so the case of
unregistering while streaming might be more common in vimc.
Maybe we can decide it is impossible to unregistered the device through
the configfs while it is streaming.
Also, I see that vivid doesn't even call vb2_queue_release() (is this a bug?).
The problem with not calling vb2_queue_release from vimc_cap_unregister
is that later when the last fh closes the call to media_pipeline_stop
crashes since it it called with a media entity that is not registered.
The vivid driver does not call media_pipeline_stop so this crash does
not occur in vivid.
Another note is that video_unregister_device() should be called before vb2_queue_release()
to avoid any other ioctls from userspace.
Helen
