* [PATCH v4 0/6] media: vimc: race condition fixes
@ 2020-01-13 21:55 Dafna Hirschfeld
2020-01-13 21:55 ` [PATCH v4 1/6] media: vimc: replace vimc->pdev.dev with vimc->mdev.dev Dafna Hirschfeld
` (5 more replies)
0 siblings, 6 replies; 20+ messages in thread
From: Dafna Hirschfeld @ 2020-01-13 21:55 UTC (permalink / raw)
To: linux-media
Cc: dafna.hirschfeld, helen.koike, ezequiel, skhan, hverkuil, kernel, dafna3
This is a v4 of the formerly called patchset "media: vimc: release vimc in release cb of v4l2_device"
It solves several crashes and memory leaks that were detected in vimc when running a code
that constantly tries to stream the device and in parallel a code that constantly binds
and unbinds the device.
2 bugs are solved in this patchset:
1. Currently when the device is unbounded, the vimc entities are
released immediately. If the device is streaming,
the function `vimc_streamer_pipeline_terminate` is called when
unregistering the streaming capture device and it references the
already freed vimc entities.
The patchset solves this by deferring the release
to the release callback of v4l2_device. This ensures that
the vimc entities will be released after the last fh is closed
and so the streaming terminates before.
The detail of how to reproduce this bug are described in the patch:
"use-after-free fix - release vimc in the v4l_device release"
The bug is easily detected with a KASAN report.
2. Another bug occurs when calling vb2_queue_release from
vimc_cap_unregister. The call to `vb2_queue_release` cause
the streaming to stop in case streaming started, this cause race condition
when it is called in parallel to other system calls that are related to streaming.
I was able to detect two crashes. One is reported in the commit log of
"media: vimc: capture: crash fix - synchronize call to vb2_queue_release when unregistering"
[ 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
This is because the call to `vb2_core_queue_release` is called in parallel both
from v4l2_release and from vimc_cap_unregister.
Another is
[ 648.499742] vimc vimc.0: first entity in the pipe 'Scaler' is not a source
[ 648.501570] BUG: kernel NULL pointer dereference, address: 0000000000000118
[ 648.501904] #PF: supervisor read access in kernel mode
[ 648.502108] #PF: error_code(0x0000) - not-present page
[ 648.502412] PGD 0 P4D 0
[ 648.502863] Oops: 0000 [#1] SMP NOPTI
[ 648.503163] CPU: 0 PID: 7039 Comm: v4l2-ctl Not tainted 5.5.0-rc1+ #5
[ 648.503403] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
[ 648.504158] RIP: 0010:mutex_lock+0x19/0x30
[ 648.504416] Code: 00 66 66 66 66 90 be 02 00 00 00 e9 31 f8 ff ff 90 66 66 66 66 90 53 48 89 fb e8 32 e0 ff ff 65 48 8b 14 25 80 7d 01 00 31 c0 <3e> 48 0f b1 13 75 02 5b c3 48 89 df 5b eb c8 0f 1f 84 00 00 00 00
[ 648.504953] RSP: 0018:ffffbdc2c08efc68 EFLAGS: 00000246
[ 648.505140] RAX: 0000000000000000 RBX: 0000000000000118 RCX: ffff9e01b3bd7000
[ 648.505363] RDX: ffff9e01b5fc9980 RSI: 0000000000000001 RDI: 0000000000000118
[ 648.505601] RBP: ffff9e01b3bd7020 R08: 0000000000000000 R09: ffffffffc0198500
[ 648.505815] R10: ffff9e01b532a4c0 R11: 0000000000000001 R12: ffff9e01b3bd7020
[ 648.506029] R13: ffff9e01b4ff2dc0 R14: ffff9e01b3bd7810 R15: 0000000000000000
[ 648.506270] FS: 00007fbd56f14240(0000) GS:ffff9e01b7c00000(0000) knlGS:0000000000000000
[ 648.506515] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 648.506686] CR2: 0000000000000118 CR3: 0000000232ff4000 CR4: 00000000000006f0
[ 648.506957] Call Trace:
[ 648.508242] media_pipeline_stop+0x1c/0x30 [mc]
[ 648.508526] vimc_cap_start_streaming+0x62/0x80 [vimc]
[ 648.508689] vb2_start_streaming+0x5c/0x110 [videobuf2_common]
[ 648.508892] vb2_core_streamon+0xf0/0x110 [videobuf2_common]
[ 648.509116] __video_do_ioctl+0x194/0x4b0 [videodev]
[ 648.509392] ? video_put_user+0x210/0x210 [videodev]
[ 648.509561] video_usercopy+0x1b3/0x510 [videodev]
[ 648.509798] v4l2_ioctl+0x45/0x50 [videodev]
[ 648.510038] do_vfs_ioctl+0xa0/0x670
[ 648.510169] ksys_ioctl+0x70/0x80
[ 648.510282] __x64_sys_ioctl+0x16/0x20
[ 648.510410] do_syscall_64+0x48/0x160
[ 648.510537] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 648.510944] RIP: 0033:0x7fbd55e5a5d7
Caused when the call to vb2_core_queue_release is in parallel to the streamon
ioctl.
The last two patches:
media: mc-devnode.c: set devnode->media_dev to NULL upon release instead of unregister
media: vimc: Track the media device by calling v4l2_device_get/put
Make sure that the release callback of v4l2_dev will be called after the last
fh of the /dev/media* is closed. This is due to Hans Verkuil's comment:
https://lore.kernel.org/linux-media/a713212b-c760-f0c5-da5e-965d4fd87789@xs4all.nl/
The crashes are reproducible by running:
modprobe vimc
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
It can be executed with
git clone https://gitlab.collabora.com/dafna/v4l2-crash-reproducers.git
cd cd v4l2-crash-reproducers/
./unbind-while-streaming.sh
Another thing I did in order to increase parallel execution is to add a patch to v4l-utils that deactivates the link
between "Debayer A" and the Scaler and activates the link between "RGB/YUV Input" and the Scaler.
https://gitlab.collabora.com/dafna/v4l-utils/commit/da0aa1802ea0b8e75cc1cf0d3bba3a379d58ba2e
and also load vimc with 'sca_mult=1'
This way the "RGB/YUV Capture" device is ready to stream right away without any configuration and so the script can be:
modprobe vimc sca_mult=1
while [ 1 ]; do
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
Changes since v3:
- add a more precise description of the first bug in the third patch and replace the crash report with the KASAN report
- add a fix to race condition when calling vb2_queue_release from unregistration while streaming.
- add a patch that makes sure that the release cb of v4l2_dev will execute after the last media fh in closed.
Patches Summary:
- The first patch replaces the usage of vimc_device.pdev.dev with vimc.mdev.dev
- The second patch allocates the vimc_device dynamically.
This is needed since the release of the device is deferred
and might run after the device is initialized again.
- The third patch solves the use-after-free bug by moving the release of the vimc_device
and all the vimc entities to the release callback of v4l2_device.
- The 4th patch locks vcap->lock before calling vb2_queue_release in the unregistration of the capture device
- The 5th patch is a preparation for the 8th patch - it makes sure that the devnode->media_dev is
set to null only after the mdev's release callback is called.
- the 6th patch implements a release callback for the media device that calls v4l2_device_put,
This make sure that the release callback of the v4l2_dev is called after the last /dev/media0 fh is closed.
Dafna Hirschfeld (6):
media: vimc: replace vimc->pdev.dev with vimc->mdev.dev
media: vimc: allocate vimc_device dynamically
media: vimc: use-after-free fix - release vimc in the v4l_device
release
media: vimc: capture: crash fix - synchronize call to
vb2_queue_release when unregistering
media: mc-devnode.c: set devnode->media_dev to NULL upon release
instead of unregister
media: vimc: Track the media device by calling v4l2_device_get/put
drivers/media/mc/mc-devnode.c | 3 +-
drivers/media/platform/vimc/vimc-capture.c | 20 ++--
drivers/media/platform/vimc/vimc-common.c | 2 -
drivers/media/platform/vimc/vimc-common.h | 30 +++---
drivers/media/platform/vimc/vimc-core.c | 111 +++++++++++++++------
drivers/media/platform/vimc/vimc-debayer.c | 19 ++--
drivers/media/platform/vimc/vimc-scaler.c | 19 ++--
drivers/media/platform/vimc/vimc-sensor.c | 18 ++--
8 files changed, 128 insertions(+), 94 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 1/6] media: vimc: replace vimc->pdev.dev with vimc->mdev.dev
2020-01-13 21:55 [PATCH v4 0/6] media: vimc: race condition fixes Dafna Hirschfeld
@ 2020-01-13 21:55 ` Dafna Hirschfeld
2020-01-13 21:55 ` [PATCH v4 2/6] media: vimc: allocate vimc_device dynamically Dafna Hirschfeld
` (4 subsequent siblings)
5 siblings, 0 replies; 20+ messages in thread
From: Dafna Hirschfeld @ 2020-01-13 21:55 UTC (permalink / raw)
To: linux-media
Cc: dafna.hirschfeld, helen.koike, ezequiel, skhan, hverkuil, kernel, dafna3
replace 'vimc->pdev.dev' with 'vimc->mdev.dev'
in debug prints and in assignment to
vimc_ent_device.dev. This helps to unify the debug
statements. This will also eliminate the need to use
the pdev field in vimc_device in future patch.
Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
drivers/media/platform/vimc/vimc-capture.c | 6 +++---
drivers/media/platform/vimc/vimc-core.c | 4 ++--
drivers/media/platform/vimc/vimc-debayer.c | 2 +-
drivers/media/platform/vimc/vimc-scaler.c | 2 +-
drivers/media/platform/vimc/vimc-sensor.c | 2 +-
5 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index 76c015898cfd..9a78bb7826a8 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -423,7 +423,7 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
ret = vb2_queue_init(q);
if (ret) {
- dev_err(&vimc->pdev.dev, "%s: vb2 queue init failed (err=%d)\n",
+ dev_err(vimc->mdev.dev, "%s: vb2 queue init failed (err=%d)\n",
vcfg_name, ret);
goto err_clean_m_ent;
}
@@ -443,7 +443,7 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
vcap->ved.ent = &vcap->vdev.entity;
vcap->ved.process_frame = vimc_cap_process_frame;
vcap->ved.vdev_get_format = vimc_cap_get_format;
- vcap->ved.dev = &vimc->pdev.dev;
+ vcap->ved.dev = vimc->mdev.dev;
/* Initialize the video_device struct */
vdev = &vcap->vdev;
@@ -462,7 +462,7 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
/* Register the video_device with the v4l2 and the media framework */
ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
if (ret) {
- dev_err(&vimc->pdev.dev, "%s: video register failed (err=%d)\n",
+ dev_err(vimc->mdev.dev, "%s: video register failed (err=%d)\n",
vcap->vdev.name, ret);
goto err_release_queue;
}
diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
index 97a272f3350a..51c89fc79d90 100644
--- a/drivers/media/platform/vimc/vimc-core.c
+++ b/drivers/media/platform/vimc/vimc-core.c
@@ -162,12 +162,12 @@ static int vimc_add_subdevs(struct vimc_device *vimc)
unsigned int i;
for (i = 0; i < vimc->pipe_cfg->num_ents; i++) {
- dev_dbg(&vimc->pdev.dev, "new entity for %s\n",
+ dev_dbg(vimc->mdev.dev, "new entity for %s\n",
vimc->pipe_cfg->ents[i].name);
vimc->ent_devs[i] = vimc->pipe_cfg->ents[i].add(vimc,
vimc->pipe_cfg->ents[i].name);
if (!vimc->ent_devs[i]) {
- dev_err(&vimc->pdev.dev, "add new entity for %s\n",
+ dev_err(vimc->mdev.dev, "add new entity for %s\n",
vimc->pipe_cfg->ents[i].name);
return -EINVAL;
}
diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
index 5d1b67d684bb..34b98b235880 100644
--- a/drivers/media/platform/vimc/vimc-debayer.c
+++ b/drivers/media/platform/vimc/vimc-debayer.c
@@ -569,7 +569,7 @@ struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
goto err_free_hdl;
vdeb->ved.process_frame = vimc_deb_process_frame;
- vdeb->ved.dev = &vimc->pdev.dev;
+ vdeb->ved.dev = vimc->mdev.dev;
vdeb->mean_win_size = vimc_deb_ctrl_mean_win_size.def;
/* Initialize the frame format */
diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
index e2e551bc3ded..cb2dc24c3e59 100644
--- a/drivers/media/platform/vimc/vimc-scaler.c
+++ b/drivers/media/platform/vimc/vimc-scaler.c
@@ -512,7 +512,7 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
}
vsca->ved.process_frame = vimc_sca_process_frame;
- vsca->ved.dev = &vimc->pdev.dev;
+ vsca->ved.dev = vimc->mdev.dev;
/* Initialize the frame format */
vsca->sink_fmt = sink_fmt_default;
diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
index 32380f504591..9788fe291193 100644
--- a/drivers/media/platform/vimc/vimc-sensor.c
+++ b/drivers/media/platform/vimc/vimc-sensor.c
@@ -370,7 +370,7 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
goto err_free_tpg;
vsen->ved.process_frame = vimc_sen_process_frame;
- vsen->ved.dev = &vimc->pdev.dev;
+ vsen->ved.dev = vimc->mdev.dev;
/* Initialize the frame format */
vsen->mbus_format = fmt_default;
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v4 2/6] media: vimc: allocate vimc_device dynamically
2020-01-13 21:55 [PATCH v4 0/6] media: vimc: race condition fixes Dafna Hirschfeld
2020-01-13 21:55 ` [PATCH v4 1/6] media: vimc: replace vimc->pdev.dev with vimc->mdev.dev Dafna Hirschfeld
@ 2020-01-13 21:55 ` Dafna Hirschfeld
2020-01-13 21:55 ` [PATCH v4 3/6] media: vimc: use-after-free fix - release vimc in the v4l_device release Dafna Hirschfeld
` (3 subsequent siblings)
5 siblings, 0 replies; 20+ messages in thread
From: Dafna Hirschfeld @ 2020-01-13 21:55 UTC (permalink / raw)
To: linux-media
Cc: dafna.hirschfeld, helen.koike, ezequiel, skhan, hverkuil, kernel, dafna3
In future patch, the release of the device will move
to the release callback of v4l2_device. Therefore the
device will be released only when the last fh will be
closed. Dynamic allocation will then be needed since
when the device is unbounded and then bounded again,
it might be that the probe callback will run before
the release of the last device is finished. In that
case both operations will run on the same memory
concurrently and cause memory corruption.
This patch also removes the pdev field of
vimc_device since it is not needed anymore.
Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
drivers/media/platform/vimc/vimc-common.h | 2 --
drivers/media/platform/vimc/vimc-core.c | 31 +++++++++++++----------
2 files changed, 18 insertions(+), 15 deletions(-)
diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
index 87eb8259c2a8..1b6ef7196f3c 100644
--- a/drivers/media/platform/vimc/vimc-common.h
+++ b/drivers/media/platform/vimc/vimc-common.h
@@ -106,14 +106,12 @@ struct vimc_ent_device {
/**
* struct vimc_device - main device for vimc driver
*
- * @pdev pointer to the platform device
* @pipe_cfg pointer to the vimc pipeline configuration structure
* @ent_devs array of vimc_ent_device pointers
* @mdev the associated media_device parent
* @v4l2_dev Internal v4l2 parent device
*/
struct vimc_device {
- struct platform_device pdev;
const struct vimc_pipeline_config *pipe_cfg;
struct vimc_ent_device **ent_devs;
struct media_device mdev;
diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
index 51c89fc79d90..1c55e0382f09 100644
--- a/drivers/media/platform/vimc/vimc-core.c
+++ b/drivers/media/platform/vimc/vimc-core.c
@@ -252,16 +252,21 @@ static void vimc_unregister(struct vimc_device *vimc)
media_device_cleanup(&vimc->mdev);
v4l2_device_unregister(&vimc->v4l2_dev);
kfree(vimc->ent_devs);
+ kfree(vimc);
}
static int vimc_probe(struct platform_device *pdev)
{
- struct vimc_device *vimc = container_of(pdev, struct vimc_device, pdev);
+ struct vimc_device *vimc;
int ret;
dev_dbg(&pdev->dev, "probe");
- memset(&vimc->mdev, 0, sizeof(vimc->mdev));
+ vimc = kzalloc(sizeof(*vimc), GFP_KERNEL);
+ if (!vimc)
+ return -ENOMEM;
+
+ vimc->pipe_cfg = &pipe_cfg;
/* Link the media device within the v4l2_device */
vimc->v4l2_dev.mdev = &vimc->mdev;
@@ -277,15 +282,17 @@ static int vimc_probe(struct platform_device *pdev)
ret = vimc_register_devices(vimc);
if (ret) {
media_device_cleanup(&vimc->mdev);
+ kfree(vimc);
return ret;
}
+ platform_set_drvdata(pdev, vimc);
return 0;
}
static int vimc_remove(struct platform_device *pdev)
{
- struct vimc_device *vimc = container_of(pdev, struct vimc_device, pdev);
+ struct vimc_device *vimc = platform_get_drvdata(pdev);
dev_dbg(&pdev->dev, "remove");
@@ -299,12 +306,10 @@ static void vimc_dev_release(struct device *dev)
{
}
-static struct vimc_device vimc_dev = {
- .pipe_cfg = &pipe_cfg,
- .pdev = {
- .name = VIMC_PDEV_NAME,
- .dev.release = vimc_dev_release,
- }
+
+static struct platform_device vimc_pdev = {
+ .name = VIMC_PDEV_NAME,
+ .dev.release = vimc_dev_release,
};
static struct platform_driver vimc_pdrv = {
@@ -319,16 +324,16 @@ static int __init vimc_init(void)
{
int ret;
- ret = platform_device_register(&vimc_dev.pdev);
+ ret = platform_device_register(&vimc_pdev);
if (ret) {
- dev_err(&vimc_dev.pdev.dev,
+ dev_err(&vimc_pdev.dev,
"platform device registration failed (err=%d)\n", ret);
return ret;
}
ret = platform_driver_register(&vimc_pdrv);
if (ret) {
- dev_err(&vimc_dev.pdev.dev,
+ dev_err(&vimc_pdev.dev,
"platform driver registration failed (err=%d)\n", ret);
platform_driver_unregister(&vimc_pdrv);
return ret;
@@ -341,7 +346,7 @@ static void __exit vimc_exit(void)
{
platform_driver_unregister(&vimc_pdrv);
- platform_device_unregister(&vimc_dev.pdev);
+ platform_device_unregister(&vimc_pdev);
}
module_init(vimc_init);
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v4 3/6] media: vimc: use-after-free fix - release vimc in the v4l_device release
2020-01-13 21:55 [PATCH v4 0/6] media: vimc: race condition fixes Dafna Hirschfeld
2020-01-13 21:55 ` [PATCH v4 1/6] media: vimc: replace vimc->pdev.dev with vimc->mdev.dev Dafna Hirschfeld
2020-01-13 21:55 ` [PATCH v4 2/6] media: vimc: allocate vimc_device dynamically Dafna Hirschfeld
@ 2020-01-13 21:55 ` Dafna Hirschfeld
2020-01-14 0:22 ` Helen Koike
2020-01-13 21:55 ` [PATCH v4 4/6] media: vimc: capture: crash fix - synchronize call to vb2_queue_release when unregistering Dafna Hirschfeld
` (2 subsequent siblings)
5 siblings, 1 reply; 20+ messages in thread
From: Dafna Hirschfeld @ 2020-01-13 21:55 UTC (permalink / raw)
To: linux-media
Cc: dafna.hirschfeld, helen.koike, ezequiel, skhan, hverkuil, kernel, dafna3
A use-after-free bug occures when unbinding the device while it streams.
The 'struct vimc_ent_device' allocated for the 'Sensor A' is freed
when calling the sensor's 'rm' callback but the freed pointer is
later accessed in the function 'vimc_streamer_pipeline_terminate'.
To fix this bug, move the release callback of the vimc entities
and vimc_device to the release callback of v4l2_device.
The .rm callback of vimc_ent_config is replaced by two callbacks:
.unregister - this is called upon removing the device and
it unregisters the entity.
.release - this is called from the release callback of v4l2_device
and it frees the entity.
This ensures that the entities will be released when the last fh
of any of the devices is closed.
The commands that cause the crash and the KASAN report:
media-ctl -d platform:vimc -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
media-ctl -d platform:vimc -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=1920,height=1440
v4l2-ctl -z platform:vimc -d "Raw Capture 0" -v pixelformat=BA81
v4l2-ctl --stream-mmap --stream-count=1000 -d /dev/video2 &
sleep 1
echo -n vimc.0 >/sys/bus/platform/drivers/vimc/unbind
[ 188.417934] BUG: KASAN: use-after-free in vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
[ 188.420182] Read of size 8 at addr ffff8881e9c26008 by task bash/185
[ 188.421800]
[ 188.422223] CPU: 0 PID: 185 Comm: bash Not tainted 5.5.0-rc1+ #1
[ 188.423681] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
[ 188.425938] Call Trace:
[ 188.426610] dump_stack+0x75/0xa0
[ 188.427519] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
[ 188.429057] print_address_description.constprop.6+0x16/0x220
[ 188.430462] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
[ 188.431979] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
[ 188.433455] __kasan_report.cold.9+0x1a/0x40
[ 188.434518] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
[ 188.436010] kasan_report+0xe/0x20
[ 188.436859] vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
[ 188.438339] vimc_streamer_s_stream+0x8b/0x3c0 [vimc]
[ 188.439576] vimc_cap_stop_streaming+0x22/0x40 [vimc]
[ 188.440863] __vb2_queue_cancel+0x65/0x560 [videobuf2_common]
[ 188.442391] vb2_core_queue_release+0x19/0x50 [videobuf2_common]
[ 188.443974] vimc_cap_rm+0x10/0x20 [vimc]
[ 188.444986] vimc_rm_subdevs+0x9e/0xe0 [vimc]
[ 188.446179] vimc_remove+0x19/0x70 [vimc]
[ 188.447301] platform_drv_remove+0x2f/0x50
[ 188.448468] device_release_driver_internal+0x133/0x260
[ 188.449814] unbind_store+0x121/0x150
[ 188.450726] kernfs_fop_write+0x142/0x230
[ 188.451724] ? sysfs_kf_bin_read+0x100/0x100
[ 188.452826] vfs_write+0xdc/0x230
[ 188.453760] ksys_write+0xaf/0x140
[ 188.454702] ? __ia32_sys_read+0x40/0x40
[ 188.455773] ? __do_page_fault+0x473/0x620
[ 188.456780] do_syscall_64+0x5e/0x1a0
[ 188.457711] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 188.459079] RIP: 0033:0x7f80f1f13504
[ 188.459969] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 48 8d 05 f9 61 0d 00 8b 00 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 41 54 49 89 d4 55 48 89 f5 53
[ 188.464445] RSP: 002b:00007ffd7e843b58 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
[ 188.466276] RAX: ffffffffffffffda RBX: 0000000000000006 RCX: 00007f80f1f13504
[ 188.467999] RDX: 0000000000000006 RSI: 000055ef2eb21b10 RDI: 0000000000000001
[ 188.469708] RBP: 000055ef2eb21b10 R08: 00007f80f1fe68c0 R09: 00007f80f1e26740
[ 188.471407] R10: 000055ef2eade010 R11: 0000000000000246 R12: 00007f80f1fe5760
[ 188.473381] R13: 0000000000000006 R14: 00007f80f1fe0760 R15: 0000000000000006
[ 188.475107]
[ 188.475500] Allocated by task 473:
[ 188.476351] save_stack+0x19/0x80
[ 188.477201] __kasan_kmalloc.constprop.6+0xc1/0xd0
[ 188.478507] vimc_sen_add+0x36/0x309 [vimc]
[ 188.479649] vimc_probe+0x1e2/0x530 [vimc]
[ 188.480776] platform_drv_probe+0x46/0xa0
[ 188.481829] really_probe+0x16c/0x520
[ 188.482732] driver_probe_device+0x114/0x170
[ 188.483783] device_driver_attach+0x85/0x90
[ 188.484800] __driver_attach+0xa8/0x190
[ 188.485734] bus_for_each_dev+0xe4/0x140
[ 188.486702] bus_add_driver+0x223/0x2d0
[ 188.487715] driver_register+0xca/0x140
[ 188.488767] 0xffffffffc037003d
[ 188.489635] do_one_initcall+0x86/0x28f
[ 188.490702] do_init_module+0xf8/0x340
[ 188.491773] load_module+0x3766/0x3a10
[ 188.492811] __do_sys_finit_module+0x11a/0x1b0
[ 188.494059] do_syscall_64+0x5e/0x1a0
[ 188.495079] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 188.496481]
[ 188.496893] Freed by task 185:
[ 188.497670] save_stack+0x19/0x80
[ 188.498493] __kasan_slab_free+0x125/0x170
[ 188.499486] kfree+0x8c/0x230
[ 188.500254] v4l2_subdev_release+0x64/0x70 [videodev]
[ 188.501498] v4l2_device_release_subdev_node+0x1c/0x30 [videodev]
[ 188.502976] device_release+0x3c/0xd0
[ 188.503867] kobject_put+0xf4/0x240
[ 188.507802] vimc_rm_subdevs+0x9e/0xe0 [vimc]
[ 188.508846] vimc_remove+0x19/0x70 [vimc]
[ 188.509792] platform_drv_remove+0x2f/0x50
[ 188.510752] device_release_driver_internal+0x133/0x260
[ 188.512006] unbind_store+0x121/0x150
[ 188.512899] kernfs_fop_write+0x142/0x230
[ 188.513874] vfs_write+0xdc/0x230
[ 188.514698] ksys_write+0xaf/0x140
[ 188.515523] do_syscall_64+0x5e/0x1a0
[ 188.516543] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 188.517710]
[ 188.518034] The buggy address belongs to the object at ffff8881e9c26000
[ 188.518034] which belongs to the cache kmalloc-4k of size 4096
[ 188.520528] The buggy address is located 8 bytes inside of
[ 188.520528] 4096-byte region [ffff8881e9c26000, ffff8881e9c27000)
[ 188.523015] The buggy address belongs to the page:
[ 188.524357] page:ffffea0007a70800 refcount:1 mapcount:0 mapping:ffff8881f6402140 index:0x0 compound_mapcount: 0
[ 188.527058] raw: 0200000000010200 dead000000000100 dead000000000122 ffff8881f6402140
[ 188.528983] raw: 0000000000000000 0000000000040004 00000001ffffffff 0000000000000000
[ 188.530883] page dumped because: kasan: bad access detected
[ 188.532336]
[ 188.532720] Memory state around the buggy address:
[ 188.533871] ffff8881e9c25f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 188.535631] ffff8881e9c25f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 188.537370] >ffff8881e9c26000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 188.538996] ^
[ 188.539812] ffff8881e9c26080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 188.541549] ffff8881e9c26100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
drivers/media/platform/vimc/vimc-capture.c | 12 ++--
drivers/media/platform/vimc/vimc-common.c | 2 -
drivers/media/platform/vimc/vimc-common.h | 28 +++++----
drivers/media/platform/vimc/vimc-core.c | 68 ++++++++++++++++------
drivers/media/platform/vimc/vimc-debayer.c | 17 ++----
drivers/media/platform/vimc/vimc-scaler.c | 17 ++----
drivers/media/platform/vimc/vimc-sensor.c | 16 ++---
7 files changed, 90 insertions(+), 70 deletions(-)
diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index 9a78bb7826a8..c5a645f98c66 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -325,20 +325,20 @@ static const struct media_entity_operations vimc_cap_mops = {
.link_validate = vimc_vdev_link_validate,
};
-static void vimc_cap_release(struct video_device *vdev)
+void vimc_cap_release(struct vimc_ent_device *ved)
{
struct vimc_cap_device *vcap =
- container_of(vdev, struct vimc_cap_device, vdev);
+ container_of(ved, struct vimc_cap_device, ved);
media_entity_cleanup(vcap->ved.ent);
kfree(vcap);
}
-void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
+void vimc_cap_unregister(struct vimc_ent_device *ved)
{
- struct vimc_cap_device *vcap;
+ struct vimc_cap_device *vcap =
+ container_of(ved, struct vimc_cap_device, ved);
- vcap = container_of(ved, struct vimc_cap_device, ved);
vb2_queue_release(&vcap->queue);
video_unregister_device(&vcap->vdev);
}
@@ -449,7 +449,7 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
vdev = &vcap->vdev;
vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
vdev->entity.ops = &vimc_cap_mops;
- vdev->release = vimc_cap_release;
+ vdev->release = video_device_release_empty;
vdev->fops = &vimc_cap_fops;
vdev->ioctl_ops = &vimc_cap_ioctl_ops;
vdev->lock = &vcap->lock;
diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
index 16ce9f3b7c75..c95c17c048f2 100644
--- a/drivers/media/platform/vimc/vimc-common.c
+++ b/drivers/media/platform/vimc/vimc-common.c
@@ -327,7 +327,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
u32 function,
u16 num_pads,
struct media_pad *pads,
- const struct v4l2_subdev_internal_ops *sd_int_ops,
const struct v4l2_subdev_ops *sd_ops)
{
int ret;
@@ -337,7 +336,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
/* Initialize the subdev */
v4l2_subdev_init(sd, sd_ops);
- sd->internal_ops = sd_int_ops;
sd->entity.function = function;
sd->entity.ops = &vimc_ent_sd_mops;
sd->owner = THIS_MODULE;
diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
index 1b6ef7196f3c..99beb2134d40 100644
--- a/drivers/media/platform/vimc/vimc-common.h
+++ b/drivers/media/platform/vimc/vimc-common.h
@@ -125,16 +125,18 @@ struct vimc_device {
* @name entity name
* @ved pointer to vimc_ent_device (a node in the
* topology)
- * @add subdev add hook - initializes and registers
- * subdev called from vimc-core
- * @rm subdev rm hook - unregisters and frees
- * subdev called from vimc-core
+ * @add initializes and registers
+ * vim entity - called from vimc-core
+ * @unregister unregisters vimc entity - called from vimc-core
+ * @release releases vimc entity - called from the v4l2_dev
+ * release callback
*/
struct vimc_ent_config {
const char *name;
struct vimc_ent_device *(*add)(struct vimc_device *vimc,
const char *vcfg_name);
- void (*rm)(struct vimc_device *vimc, struct vimc_ent_device *ved);
+ void (*unregister)(struct vimc_ent_device *ved);
+ void (*release)(struct vimc_ent_device *ved);
};
/**
@@ -145,22 +147,26 @@ struct vimc_ent_config {
*/
bool vimc_is_source(struct media_entity *ent);
-/* prototypes for vimc_ent_config add and rm hooks */
+/* prototypes for vimc_ent_config hooks */
struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
const char *vcfg_name);
-void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
+void vimc_cap_unregister(struct vimc_ent_device *ved);
+void vimc_cap_release(struct vimc_ent_device *ved);
struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
const char *vcfg_name);
-void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
+void vimc_deb_unregister(struct vimc_ent_device *ved);
+void vimc_deb_release(struct vimc_ent_device *ved);
struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
const char *vcfg_name);
-void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
+void vimc_sca_unregister(struct vimc_ent_device *ved);
+void vimc_sca_release(struct vimc_ent_device *ved);
struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
const char *vcfg_name);
-void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
+void vimc_sen_unregister(struct vimc_ent_device *ved);
+void vimc_sen_release(struct vimc_ent_device *ved);
/**
* vimc_pix_map_by_index - get vimc_pix_map struct by its index
@@ -195,7 +201,6 @@ const struct vimc_pix_map *vimc_pix_map_by_pixelformat(u32 pixelformat);
* @num_pads: number of pads to initialize
* @pads: the array of pads of the entity, the caller should set the
flags of the pads
- * @sd_int_ops: pointer to &struct v4l2_subdev_internal_ops
* @sd_ops: pointer to &struct v4l2_subdev_ops.
*
* Helper function initialize and register the struct vimc_ent_device and struct
@@ -208,7 +213,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
u32 function,
u16 num_pads,
struct media_pad *pads,
- const struct v4l2_subdev_internal_ops *sd_int_ops,
const struct v4l2_subdev_ops *sd_ops);
/**
diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
index 1c55e0382f09..9d4e8bc89620 100644
--- a/drivers/media/platform/vimc/vimc-core.c
+++ b/drivers/media/platform/vimc/vimc-core.c
@@ -48,48 +48,57 @@ static struct vimc_ent_config ent_config[] = {
{
.name = "Sensor A",
.add = vimc_sen_add,
- .rm = vimc_sen_rm,
+ .unregister = vimc_sen_unregister,
+ .release = vimc_sen_release,
},
{
.name = "Sensor B",
.add = vimc_sen_add,
- .rm = vimc_sen_rm,
+ .unregister = vimc_sen_unregister,
+ .release = vimc_sen_release,
},
{
.name = "Debayer A",
.add = vimc_deb_add,
- .rm = vimc_deb_rm,
+ .unregister = vimc_deb_unregister,
+ .release = vimc_deb_release,
},
{
.name = "Debayer B",
.add = vimc_deb_add,
- .rm = vimc_deb_rm,
+ .unregister = vimc_deb_unregister,
+ .release = vimc_deb_release,
},
{
.name = "Raw Capture 0",
.add = vimc_cap_add,
- .rm = vimc_cap_rm,
+ .unregister = vimc_cap_unregister,
+ .release = vimc_cap_release,
},
{
.name = "Raw Capture 1",
.add = vimc_cap_add,
- .rm = vimc_cap_rm,
+ .unregister = vimc_cap_unregister,
+ .release = vimc_cap_release,
},
{
/* TODO: change this to vimc-input when it is implemented */
.name = "RGB/YUV Input",
.add = vimc_sen_add,
- .rm = vimc_sen_rm,
+ .unregister = vimc_sen_unregister,
+ .release = vimc_sen_release,
},
{
.name = "Scaler",
.add = vimc_sca_add,
- .rm = vimc_sca_rm,
+ .unregister = vimc_sca_unregister,
+ .release = vimc_sca_release,
},
{
.name = "RGB/YUV Capture",
.add = vimc_cap_add,
- .rm = vimc_cap_rm,
+ .unregister = vimc_cap_unregister,
+ .release = vimc_cap_release,
},
};
@@ -175,13 +184,34 @@ static int vimc_add_subdevs(struct vimc_device *vimc)
return 0;
}
-static void vimc_rm_subdevs(struct vimc_device *vimc)
+static void vimc_release_subdevs(struct vimc_device *vimc)
{
unsigned int i;
for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
if (vimc->ent_devs[i])
- vimc->pipe_cfg->ents[i].rm(vimc, vimc->ent_devs[i]);
+ vimc->pipe_cfg->ents[i].release(vimc->ent_devs[i]);
+}
+
+
+static void vimc_unregister_subdevs(struct vimc_device *vimc)
+{
+ unsigned int i;
+
+ for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
+ if (vimc->ent_devs[i])
+ vimc->pipe_cfg->ents[i].unregister(vimc->ent_devs[i]);
+}
+
+static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev)
+{
+ struct vimc_device *vimc =
+ container_of(v4l2_dev, struct vimc_device, v4l2_dev);
+
+ vimc_release_subdevs(vimc);
+ media_device_cleanup(&vimc->mdev);
+ kfree(vimc->ent_devs);
+ kfree(vimc);
}
static int vimc_register_devices(struct vimc_device *vimc)
@@ -195,7 +225,6 @@ static int vimc_register_devices(struct vimc_device *vimc)
"v4l2 device register failed (err=%d)\n", ret);
return ret;
}
-
/* allocate ent_devs */
vimc->ent_devs = kcalloc(vimc->pipe_cfg->num_ents,
sizeof(*vimc->ent_devs), GFP_KERNEL);
@@ -236,9 +265,9 @@ static int vimc_register_devices(struct vimc_device *vimc)
err_mdev_unregister:
media_device_unregister(&vimc->mdev);
- media_device_cleanup(&vimc->mdev);
err_rm_subdevs:
- vimc_rm_subdevs(vimc);
+ vimc_unregister_subdevs(vimc);
+ vimc_release_subdevs(vimc);
kfree(vimc->ent_devs);
err_v4l2_unregister:
v4l2_device_unregister(&vimc->v4l2_dev);
@@ -248,11 +277,9 @@ static int vimc_register_devices(struct vimc_device *vimc)
static void vimc_unregister(struct vimc_device *vimc)
{
+ vimc_unregister_subdevs(vimc);
media_device_unregister(&vimc->mdev);
- media_device_cleanup(&vimc->mdev);
v4l2_device_unregister(&vimc->v4l2_dev);
- kfree(vimc->ent_devs);
- kfree(vimc);
}
static int vimc_probe(struct platform_device *pdev)
@@ -285,7 +312,12 @@ static int vimc_probe(struct platform_device *pdev)
kfree(vimc);
return ret;
}
+ /*
+ * the release cb is set only after successful registration.
+ * if the registration fails, we release directly from probe
+ */
+ vimc->v4l2_dev.release = vimc_v4l2_dev_release;
platform_set_drvdata(pdev, vimc);
return 0;
}
@@ -296,8 +328,8 @@ static int vimc_remove(struct platform_device *pdev)
dev_dbg(&pdev->dev, "remove");
- vimc_rm_subdevs(vimc);
vimc_unregister(vimc);
+ v4l2_device_put(&vimc->v4l2_dev);
return 0;
}
diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
index 34b98b235880..3beec7f95b47 100644
--- a/drivers/media/platform/vimc/vimc-debayer.c
+++ b/drivers/media/platform/vimc/vimc-debayer.c
@@ -494,25 +494,21 @@ static const struct v4l2_ctrl_ops vimc_deb_ctrl_ops = {
.s_ctrl = vimc_deb_s_ctrl,
};
-static void vimc_deb_release(struct v4l2_subdev *sd)
+void vimc_deb_release(struct vimc_ent_device *ved)
{
struct vimc_deb_device *vdeb =
- container_of(sd, struct vimc_deb_device, sd);
+ container_of(ved, struct vimc_deb_device, ved);
v4l2_ctrl_handler_free(&vdeb->hdl);
media_entity_cleanup(vdeb->ved.ent);
kfree(vdeb);
}
-static const struct v4l2_subdev_internal_ops vimc_deb_int_ops = {
- .release = vimc_deb_release,
-};
-
-void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
+void vimc_deb_unregister(struct vimc_ent_device *ved)
{
- struct vimc_deb_device *vdeb;
+ struct vimc_deb_device *vdeb =
+ container_of(ved, struct vimc_deb_device, ved);
- vdeb = container_of(ved, struct vimc_deb_device, ved);
v4l2_device_unregister_subdev(&vdeb->sd);
}
@@ -563,8 +559,7 @@ struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
ret = vimc_ent_sd_register(&vdeb->ved, &vdeb->sd, v4l2_dev,
vcfg_name,
MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV, 2,
- vdeb->pads,
- &vimc_deb_int_ops, &vimc_deb_ops);
+ vdeb->pads, &vimc_deb_ops);
if (ret)
goto err_free_hdl;
diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
index cb2dc24c3e59..de9bee504b59 100644
--- a/drivers/media/platform/vimc/vimc-scaler.c
+++ b/drivers/media/platform/vimc/vimc-scaler.c
@@ -464,24 +464,20 @@ static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
return vsca->src_frame;
};
-static void vimc_sca_release(struct v4l2_subdev *sd)
+void vimc_sca_release(struct vimc_ent_device *ved)
{
struct vimc_sca_device *vsca =
- container_of(sd, struct vimc_sca_device, sd);
+ container_of(ved, struct vimc_sca_device, ved);
media_entity_cleanup(vsca->ved.ent);
kfree(vsca);
}
-static const struct v4l2_subdev_internal_ops vimc_sca_int_ops = {
- .release = vimc_sca_release,
-};
-
-void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
+void vimc_sca_unregister(struct vimc_ent_device *ved)
{
- struct vimc_sca_device *vsca;
+ struct vimc_sca_device *vsca =
+ container_of(ved, struct vimc_sca_device, ved);
- vsca = container_of(ved, struct vimc_sca_device, ved);
v4l2_device_unregister_subdev(&vsca->sd);
}
@@ -504,8 +500,7 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
ret = vimc_ent_sd_register(&vsca->ved, &vsca->sd, v4l2_dev,
vcfg_name,
MEDIA_ENT_F_PROC_VIDEO_SCALER, 2,
- vsca->pads,
- &vimc_sca_int_ops, &vimc_sca_ops);
+ vsca->pads, &vimc_sca_ops);
if (ret) {
kfree(vsca);
return NULL;
diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
index 9788fe291193..14eeaf461e93 100644
--- a/drivers/media/platform/vimc/vimc-sensor.c
+++ b/drivers/media/platform/vimc/vimc-sensor.c
@@ -279,10 +279,10 @@ static const struct v4l2_ctrl_ops vimc_sen_ctrl_ops = {
.s_ctrl = vimc_sen_s_ctrl,
};
-static void vimc_sen_release(struct v4l2_subdev *sd)
+void vimc_sen_release(struct vimc_ent_device *ved)
{
struct vimc_sen_device *vsen =
- container_of(sd, struct vimc_sen_device, sd);
+ container_of(ved, struct vimc_sen_device, ved);
v4l2_ctrl_handler_free(&vsen->hdl);
tpg_free(&vsen->tpg);
@@ -290,15 +290,11 @@ static void vimc_sen_release(struct v4l2_subdev *sd)
kfree(vsen);
}
-static const struct v4l2_subdev_internal_ops vimc_sen_int_ops = {
- .release = vimc_sen_release,
-};
-
-void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
+void vimc_sen_unregister(struct vimc_ent_device *ved)
{
- struct vimc_sen_device *vsen;
+ struct vimc_sen_device *vsen =
+ container_of(ved, struct vimc_sen_device, ved);
- vsen = container_of(ved, struct vimc_sen_device, ved);
v4l2_device_unregister_subdev(&vsen->sd);
}
@@ -365,7 +361,7 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
ret = vimc_ent_sd_register(&vsen->ved, &vsen->sd, v4l2_dev,
vcfg_name,
MEDIA_ENT_F_CAM_SENSOR, 1, &vsen->pad,
- &vimc_sen_int_ops, &vimc_sen_ops);
+ &vimc_sen_ops);
if (ret)
goto err_free_tpg;
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v4 4/6] media: vimc: capture: crash fix - synchronize call to vb2_queue_release when unregistering
2020-01-13 21:55 [PATCH v4 0/6] media: vimc: race condition fixes Dafna Hirschfeld
` (2 preceding siblings ...)
2020-01-13 21:55 ` [PATCH v4 3/6] media: vimc: use-after-free fix - release vimc in the v4l_device release Dafna Hirschfeld
@ 2020-01-13 21:55 ` Dafna Hirschfeld
2020-01-14 1:31 ` Helen Koike
2020-01-14 10:25 ` Hans Verkuil
2020-01-13 21:55 ` [PATCH v4 5/6] media: mc-devnode.c: set devnode->media_dev to NULL upon release instead of unregister Dafna Hirschfeld
2020-01-13 21:55 ` [PATCH v4 6/6] media: vimc: Track the media device by calling v4l2_device_get/put Dafna Hirschfeld
5 siblings, 2 replies; 20+ messages in thread
From: Dafna Hirschfeld @ 2020-01-13 21:55 UTC (permalink / raw)
To: linux-media
Cc: dafna.hirschfeld, helen.koike, ezequiel, skhan, hverkuil, kernel, dafna3
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@collabora.com>
---
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);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v4 5/6] media: mc-devnode.c: set devnode->media_dev to NULL upon release instead of unregister
2020-01-13 21:55 [PATCH v4 0/6] media: vimc: race condition fixes Dafna Hirschfeld
` (3 preceding siblings ...)
2020-01-13 21:55 ` [PATCH v4 4/6] media: vimc: capture: crash fix - synchronize call to vb2_queue_release when unregistering Dafna Hirschfeld
@ 2020-01-13 21:55 ` Dafna Hirschfeld
2020-01-14 10:26 ` Hans Verkuil
2020-01-13 21:55 ` [PATCH v4 6/6] media: vimc: Track the media device by calling v4l2_device_get/put Dafna Hirschfeld
5 siblings, 1 reply; 20+ messages in thread
From: Dafna Hirschfeld @ 2020-01-13 21:55 UTC (permalink / raw)
To: linux-media
Cc: dafna.hirschfeld, helen.koike, ezequiel, skhan, hverkuil, kernel, dafna3
media_devnode_release calls a release callback. Currently none of
the drivers implement that callback but in the future the vimc
driver and maybe others might add implementation for it.
The release callback will want to access the driver's private data
by using 'container_of(devnode->media_dev' therefore media_dev
should be set to NULL only when the release callback returns.
Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
drivers/media/mc/mc-devnode.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index f11382afe23b..388c9051152a 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -58,7 +58,7 @@ static void media_devnode_release(struct device *cd)
/* Release media_devnode and perform other cleanups as needed. */
if (devnode->release)
devnode->release(devnode);
-
+ devnode->media_dev = NULL;
kfree(devnode);
pr_debug("%s: Media Devnode Deallocated\n", __func__);
}
@@ -283,7 +283,6 @@ void media_devnode_unregister(struct media_devnode *devnode)
mutex_lock(&media_devnode_lock);
/* Delete the cdev on this minor as well */
cdev_device_del(&devnode->cdev, &devnode->dev);
- devnode->media_dev = NULL;
mutex_unlock(&media_devnode_lock);
put_device(&devnode->dev);
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v4 6/6] media: vimc: Track the media device by calling v4l2_device_get/put
2020-01-13 21:55 [PATCH v4 0/6] media: vimc: race condition fixes Dafna Hirschfeld
` (4 preceding siblings ...)
2020-01-13 21:55 ` [PATCH v4 5/6] media: mc-devnode.c: set devnode->media_dev to NULL upon release instead of unregister Dafna Hirschfeld
@ 2020-01-13 21:55 ` Dafna Hirschfeld
2020-01-14 2:27 ` Helen Koike
2020-01-14 10:47 ` Hans Verkuil
5 siblings, 2 replies; 20+ messages in thread
From: Dafna Hirschfeld @ 2020-01-13 21:55 UTC (permalink / raw)
To: linux-media
Cc: dafna.hirschfeld, helen.koike, ezequiel, skhan, hverkuil, kernel, dafna3
After a successful media_device_register call, call v4l2_device_get().
and set the media_devnode release callback to a function that
calls v4l2_device_put().
That should ensure that the v4l2_device's release callback is called
when the very last user of any of the registered device nodes has
closed its fh.
Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
drivers/media/platform/vimc/vimc-core.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
index 9d4e8bc89620..0f03e9cec075 100644
--- a/drivers/media/platform/vimc/vimc-core.c
+++ b/drivers/media/platform/vimc/vimc-core.c
@@ -214,6 +214,14 @@ static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev)
kfree(vimc);
}
+static void vimc_media_device_release(struct media_devnode *devnode)
+{
+ struct media_device *mdev = devnode->media_dev;
+ struct vimc_device *vimc = container_of(mdev, struct vimc_device, mdev);
+
+ v4l2_device_put(&vimc->v4l2_dev);
+}
+
static int vimc_register_devices(struct vimc_device *vimc)
{
int ret;
@@ -252,6 +260,8 @@ static int vimc_register_devices(struct vimc_device *vimc)
goto err_rm_subdevs;
}
+ v4l2_device_get(&vimc->v4l2_dev);
+ vimc->mdev.devnode->release = vimc_media_device_release;
/* Expose all subdev's nodes*/
ret = v4l2_device_register_subdev_nodes(&vimc->v4l2_dev);
if (ret) {
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v4 3/6] media: vimc: use-after-free fix - release vimc in the v4l_device release
2020-01-13 21:55 ` [PATCH v4 3/6] media: vimc: use-after-free fix - release vimc in the v4l_device release Dafna Hirschfeld
@ 2020-01-14 0:22 ` Helen Koike
2020-01-14 10:52 ` Dafna Hirschfeld
0 siblings, 1 reply; 20+ messages in thread
From: Helen Koike @ 2020-01-14 0:22 UTC (permalink / raw)
To: Dafna Hirschfeld, linux-media; +Cc: ezequiel, skhan, hverkuil, kernel, dafna3
Hi Dafna,
Thanks for this patch.
On 1/13/20 7:55 PM, Dafna Hirschfeld wrote:
> A use-after-free bug occures when unbinding the device while it streams.
> The 'struct vimc_ent_device' allocated for the 'Sensor A' is freed
> when calling the sensor's 'rm' callback but the freed pointer is
> later accessed in the function 'vimc_streamer_pipeline_terminate'.
> To fix this bug, move the release callback of the vimc entities
> and vimc_device to the release callback of v4l2_device.
> The .rm callback of vimc_ent_config is replaced by two callbacks:
> .unregister - this is called upon removing the device and
> it unregisters the entity.
> .release - this is called from the release callback of v4l2_device
> and it frees the entity.
> This ensures that the entities will be released when the last fh
> of any of the devices is closed.
I have mixed feelings about this patch, because another solution would
be to make sure to stop all streams before releasing the subdevices,
which could be done by calling 'rm' callback from the capture nodes first.
Also I noticed that calling v4l2_device_unregister_subdev() from the subdevices
is a bit useless, since they are already called from v4l2_device_unregister(), who
already iterates through all subdevices.
So what I suggest is to remove all the 'rm' callbacks from subdevices, and leave
only the 'rm' from the capture entities.
Rename vimc_rm_subdevs() to vimc_rm_video_devices()
And you can loop like this:
-static void vimc_rm_subdevs(struct vimc_device *vimc)
+static void vimc_rm_video_devices(struct vimc_device *vimc)
{
unsigned int i;
for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
- if (vimc->ent_devs[i])
+ if (vimc->ent_devs[i] && vimc->pipe_cfg->ents[i].rm)
vimc->pipe_cfg->ents[i].rm(vimc, vimc->ent_devs[i]);
}
I didn't test, but I think it should work.
Your patch would be cleaner (since you would be removing a bunch of lines),
and we also would be re-using all the release functions from the media framework,
which is nice for testing the core.
What do you think?
Thanks
Helen
>
> The commands that cause the crash and the KASAN report:
>
> media-ctl -d platform:vimc -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
> media-ctl -d platform:vimc -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
> v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=1920,height=1440
> v4l2-ctl -z platform:vimc -d "Raw Capture 0" -v pixelformat=BA81
> v4l2-ctl --stream-mmap --stream-count=1000 -d /dev/video2 &
> sleep 1
> echo -n vimc.0 >/sys/bus/platform/drivers/vimc/unbind
>
> [ 188.417934] BUG: KASAN: use-after-free in vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
> [ 188.420182] Read of size 8 at addr ffff8881e9c26008 by task bash/185
> [ 188.421800]
> [ 188.422223] CPU: 0 PID: 185 Comm: bash Not tainted 5.5.0-rc1+ #1
> [ 188.423681] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
> [ 188.425938] Call Trace:
> [ 188.426610] dump_stack+0x75/0xa0
> [ 188.427519] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
> [ 188.429057] print_address_description.constprop.6+0x16/0x220
> [ 188.430462] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
> [ 188.431979] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
> [ 188.433455] __kasan_report.cold.9+0x1a/0x40
> [ 188.434518] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
> [ 188.436010] kasan_report+0xe/0x20
> [ 188.436859] vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
> [ 188.438339] vimc_streamer_s_stream+0x8b/0x3c0 [vimc]
> [ 188.439576] vimc_cap_stop_streaming+0x22/0x40 [vimc]
> [ 188.440863] __vb2_queue_cancel+0x65/0x560 [videobuf2_common]
> [ 188.442391] vb2_core_queue_release+0x19/0x50 [videobuf2_common]
> [ 188.443974] vimc_cap_rm+0x10/0x20 [vimc]
> [ 188.444986] vimc_rm_subdevs+0x9e/0xe0 [vimc]
> [ 188.446179] vimc_remove+0x19/0x70 [vimc]
> [ 188.447301] platform_drv_remove+0x2f/0x50
> [ 188.448468] device_release_driver_internal+0x133/0x260
> [ 188.449814] unbind_store+0x121/0x150
> [ 188.450726] kernfs_fop_write+0x142/0x230
> [ 188.451724] ? sysfs_kf_bin_read+0x100/0x100
> [ 188.452826] vfs_write+0xdc/0x230
> [ 188.453760] ksys_write+0xaf/0x140
> [ 188.454702] ? __ia32_sys_read+0x40/0x40
> [ 188.455773] ? __do_page_fault+0x473/0x620
> [ 188.456780] do_syscall_64+0x5e/0x1a0
> [ 188.457711] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 188.459079] RIP: 0033:0x7f80f1f13504
> [ 188.459969] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 48 8d 05 f9 61 0d 00 8b 00 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 41 54 49 89 d4 55 48 89 f5 53
> [ 188.464445] RSP: 002b:00007ffd7e843b58 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> [ 188.466276] RAX: ffffffffffffffda RBX: 0000000000000006 RCX: 00007f80f1f13504
> [ 188.467999] RDX: 0000000000000006 RSI: 000055ef2eb21b10 RDI: 0000000000000001
> [ 188.469708] RBP: 000055ef2eb21b10 R08: 00007f80f1fe68c0 R09: 00007f80f1e26740
> [ 188.471407] R10: 000055ef2eade010 R11: 0000000000000246 R12: 00007f80f1fe5760
> [ 188.473381] R13: 0000000000000006 R14: 00007f80f1fe0760 R15: 0000000000000006
> [ 188.475107]
> [ 188.475500] Allocated by task 473:
> [ 188.476351] save_stack+0x19/0x80
> [ 188.477201] __kasan_kmalloc.constprop.6+0xc1/0xd0
> [ 188.478507] vimc_sen_add+0x36/0x309 [vimc]
> [ 188.479649] vimc_probe+0x1e2/0x530 [vimc]
> [ 188.480776] platform_drv_probe+0x46/0xa0
> [ 188.481829] really_probe+0x16c/0x520
> [ 188.482732] driver_probe_device+0x114/0x170
> [ 188.483783] device_driver_attach+0x85/0x90
> [ 188.484800] __driver_attach+0xa8/0x190
> [ 188.485734] bus_for_each_dev+0xe4/0x140
> [ 188.486702] bus_add_driver+0x223/0x2d0
> [ 188.487715] driver_register+0xca/0x140
> [ 188.488767] 0xffffffffc037003d
> [ 188.489635] do_one_initcall+0x86/0x28f
> [ 188.490702] do_init_module+0xf8/0x340
> [ 188.491773] load_module+0x3766/0x3a10
> [ 188.492811] __do_sys_finit_module+0x11a/0x1b0
> [ 188.494059] do_syscall_64+0x5e/0x1a0
> [ 188.495079] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 188.496481]
> [ 188.496893] Freed by task 185:
> [ 188.497670] save_stack+0x19/0x80
> [ 188.498493] __kasan_slab_free+0x125/0x170
> [ 188.499486] kfree+0x8c/0x230
> [ 188.500254] v4l2_subdev_release+0x64/0x70 [videodev]
> [ 188.501498] v4l2_device_release_subdev_node+0x1c/0x30 [videodev]
> [ 188.502976] device_release+0x3c/0xd0
> [ 188.503867] kobject_put+0xf4/0x240
> [ 188.507802] vimc_rm_subdevs+0x9e/0xe0 [vimc]
> [ 188.508846] vimc_remove+0x19/0x70 [vimc]
> [ 188.509792] platform_drv_remove+0x2f/0x50
> [ 188.510752] device_release_driver_internal+0x133/0x260
> [ 188.512006] unbind_store+0x121/0x150
> [ 188.512899] kernfs_fop_write+0x142/0x230
> [ 188.513874] vfs_write+0xdc/0x230
> [ 188.514698] ksys_write+0xaf/0x140
> [ 188.515523] do_syscall_64+0x5e/0x1a0
> [ 188.516543] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 188.517710]
> [ 188.518034] The buggy address belongs to the object at ffff8881e9c26000
> [ 188.518034] which belongs to the cache kmalloc-4k of size 4096
> [ 188.520528] The buggy address is located 8 bytes inside of
> [ 188.520528] 4096-byte region [ffff8881e9c26000, ffff8881e9c27000)
> [ 188.523015] The buggy address belongs to the page:
> [ 188.524357] page:ffffea0007a70800 refcount:1 mapcount:0 mapping:ffff8881f6402140 index:0x0 compound_mapcount: 0
> [ 188.527058] raw: 0200000000010200 dead000000000100 dead000000000122 ffff8881f6402140
> [ 188.528983] raw: 0000000000000000 0000000000040004 00000001ffffffff 0000000000000000
> [ 188.530883] page dumped because: kasan: bad access detected
> [ 188.532336]
> [ 188.532720] Memory state around the buggy address:
> [ 188.533871] ffff8881e9c25f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [ 188.535631] ffff8881e9c25f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [ 188.537370] >ffff8881e9c26000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [ 188.538996] ^
> [ 188.539812] ffff8881e9c26080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [ 188.541549] ffff8881e9c26100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
> drivers/media/platform/vimc/vimc-capture.c | 12 ++--
> drivers/media/platform/vimc/vimc-common.c | 2 -
> drivers/media/platform/vimc/vimc-common.h | 28 +++++----
> drivers/media/platform/vimc/vimc-core.c | 68 ++++++++++++++++------
> drivers/media/platform/vimc/vimc-debayer.c | 17 ++----
> drivers/media/platform/vimc/vimc-scaler.c | 17 ++----
> drivers/media/platform/vimc/vimc-sensor.c | 16 ++---
> 7 files changed, 90 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index 9a78bb7826a8..c5a645f98c66 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -325,20 +325,20 @@ static const struct media_entity_operations vimc_cap_mops = {
> .link_validate = vimc_vdev_link_validate,
> };
>
> -static void vimc_cap_release(struct video_device *vdev)
> +void vimc_cap_release(struct vimc_ent_device *ved)
> {
> struct vimc_cap_device *vcap =
> - container_of(vdev, struct vimc_cap_device, vdev);
> + container_of(ved, struct vimc_cap_device, ved);
>
> media_entity_cleanup(vcap->ved.ent);
> kfree(vcap);
> }
>
> -void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
> +void vimc_cap_unregister(struct vimc_ent_device *ved)
> {
> - struct vimc_cap_device *vcap;
> + struct vimc_cap_device *vcap =
> + container_of(ved, struct vimc_cap_device, ved);
>
> - vcap = container_of(ved, struct vimc_cap_device, ved);
> vb2_queue_release(&vcap->queue);
> video_unregister_device(&vcap->vdev);
> }
> @@ -449,7 +449,7 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
> vdev = &vcap->vdev;
> vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> vdev->entity.ops = &vimc_cap_mops;
> - vdev->release = vimc_cap_release;
> + vdev->release = video_device_release_empty;
> vdev->fops = &vimc_cap_fops;
> vdev->ioctl_ops = &vimc_cap_ioctl_ops;
> vdev->lock = &vcap->lock;
> diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
> index 16ce9f3b7c75..c95c17c048f2 100644
> --- a/drivers/media/platform/vimc/vimc-common.c
> +++ b/drivers/media/platform/vimc/vimc-common.c
> @@ -327,7 +327,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
> u32 function,
> u16 num_pads,
> struct media_pad *pads,
> - const struct v4l2_subdev_internal_ops *sd_int_ops,
> const struct v4l2_subdev_ops *sd_ops)
> {
> int ret;
> @@ -337,7 +336,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
>
> /* Initialize the subdev */
> v4l2_subdev_init(sd, sd_ops);
> - sd->internal_ops = sd_int_ops;
> sd->entity.function = function;
> sd->entity.ops = &vimc_ent_sd_mops;
> sd->owner = THIS_MODULE;
> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
> index 1b6ef7196f3c..99beb2134d40 100644
> --- a/drivers/media/platform/vimc/vimc-common.h
> +++ b/drivers/media/platform/vimc/vimc-common.h
> @@ -125,16 +125,18 @@ struct vimc_device {
> * @name entity name
> * @ved pointer to vimc_ent_device (a node in the
> * topology)
> - * @add subdev add hook - initializes and registers
> - * subdev called from vimc-core
> - * @rm subdev rm hook - unregisters and frees
> - * subdev called from vimc-core
> + * @add initializes and registers
> + * vim entity - called from vimc-core
> + * @unregister unregisters vimc entity - called from vimc-core
> + * @release releases vimc entity - called from the v4l2_dev
> + * release callback
> */
> struct vimc_ent_config {
> const char *name;
> struct vimc_ent_device *(*add)(struct vimc_device *vimc,
> const char *vcfg_name);
> - void (*rm)(struct vimc_device *vimc, struct vimc_ent_device *ved);
> + void (*unregister)(struct vimc_ent_device *ved);
> + void (*release)(struct vimc_ent_device *ved);
> };
>
> /**
> @@ -145,22 +147,26 @@ struct vimc_ent_config {
> */
> bool vimc_is_source(struct media_entity *ent);
>
> -/* prototypes for vimc_ent_config add and rm hooks */
> +/* prototypes for vimc_ent_config hooks */
> struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
> const char *vcfg_name);
> -void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
> +void vimc_cap_unregister(struct vimc_ent_device *ved);
> +void vimc_cap_release(struct vimc_ent_device *ved);
>
> struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
> const char *vcfg_name);
> -void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
> +void vimc_deb_unregister(struct vimc_ent_device *ved);
> +void vimc_deb_release(struct vimc_ent_device *ved);
>
> struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
> const char *vcfg_name);
> -void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
> +void vimc_sca_unregister(struct vimc_ent_device *ved);
> +void vimc_sca_release(struct vimc_ent_device *ved);
>
> struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
> const char *vcfg_name);
> -void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
> +void vimc_sen_unregister(struct vimc_ent_device *ved);
> +void vimc_sen_release(struct vimc_ent_device *ved);
>
> /**
> * vimc_pix_map_by_index - get vimc_pix_map struct by its index
> @@ -195,7 +201,6 @@ const struct vimc_pix_map *vimc_pix_map_by_pixelformat(u32 pixelformat);
> * @num_pads: number of pads to initialize
> * @pads: the array of pads of the entity, the caller should set the
> flags of the pads
> - * @sd_int_ops: pointer to &struct v4l2_subdev_internal_ops
> * @sd_ops: pointer to &struct v4l2_subdev_ops.
> *
> * Helper function initialize and register the struct vimc_ent_device and struct
> @@ -208,7 +213,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
> u32 function,
> u16 num_pads,
> struct media_pad *pads,
> - const struct v4l2_subdev_internal_ops *sd_int_ops,
> const struct v4l2_subdev_ops *sd_ops);
>
> /**
> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
> index 1c55e0382f09..9d4e8bc89620 100644
> --- a/drivers/media/platform/vimc/vimc-core.c
> +++ b/drivers/media/platform/vimc/vimc-core.c
> @@ -48,48 +48,57 @@ static struct vimc_ent_config ent_config[] = {
> {
> .name = "Sensor A",
> .add = vimc_sen_add,
> - .rm = vimc_sen_rm,
> + .unregister = vimc_sen_unregister,
> + .release = vimc_sen_release,
> },
> {
> .name = "Sensor B",
> .add = vimc_sen_add,
> - .rm = vimc_sen_rm,
> + .unregister = vimc_sen_unregister,
> + .release = vimc_sen_release,
> },
> {
> .name = "Debayer A",
> .add = vimc_deb_add,
> - .rm = vimc_deb_rm,
> + .unregister = vimc_deb_unregister,
> + .release = vimc_deb_release,
> },
> {
> .name = "Debayer B",
> .add = vimc_deb_add,
> - .rm = vimc_deb_rm,
> + .unregister = vimc_deb_unregister,
> + .release = vimc_deb_release,
> },
> {
> .name = "Raw Capture 0",
> .add = vimc_cap_add,
> - .rm = vimc_cap_rm,
> + .unregister = vimc_cap_unregister,
> + .release = vimc_cap_release,
> },
> {
> .name = "Raw Capture 1",
> .add = vimc_cap_add,
> - .rm = vimc_cap_rm,
> + .unregister = vimc_cap_unregister,
> + .release = vimc_cap_release,
> },
> {
> /* TODO: change this to vimc-input when it is implemented */
> .name = "RGB/YUV Input",
> .add = vimc_sen_add,
> - .rm = vimc_sen_rm,
> + .unregister = vimc_sen_unregister,
> + .release = vimc_sen_release,
> },
> {
> .name = "Scaler",
> .add = vimc_sca_add,
> - .rm = vimc_sca_rm,
> + .unregister = vimc_sca_unregister,
> + .release = vimc_sca_release,
> },
> {
> .name = "RGB/YUV Capture",
> .add = vimc_cap_add,
> - .rm = vimc_cap_rm,
> + .unregister = vimc_cap_unregister,
> + .release = vimc_cap_release,
> },
> };
>
> @@ -175,13 +184,34 @@ static int vimc_add_subdevs(struct vimc_device *vimc)
> return 0;
> }
>
> -static void vimc_rm_subdevs(struct vimc_device *vimc)
> +static void vimc_release_subdevs(struct vimc_device *vimc)
> {
> unsigned int i;
>
> for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
> if (vimc->ent_devs[i])
> - vimc->pipe_cfg->ents[i].rm(vimc, vimc->ent_devs[i]);
> + vimc->pipe_cfg->ents[i].release(vimc->ent_devs[i]);
> +}
> +
> +
> +static void vimc_unregister_subdevs(struct vimc_device *vimc)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
> + if (vimc->ent_devs[i])
> + vimc->pipe_cfg->ents[i].unregister(vimc->ent_devs[i]);
> +}
> +
> +static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev)
> +{
> + struct vimc_device *vimc =
> + container_of(v4l2_dev, struct vimc_device, v4l2_dev);
> +
> + vimc_release_subdevs(vimc);
> + media_device_cleanup(&vimc->mdev);
> + kfree(vimc->ent_devs);
> + kfree(vimc);
> }
>
> static int vimc_register_devices(struct vimc_device *vimc)
> @@ -195,7 +225,6 @@ static int vimc_register_devices(struct vimc_device *vimc)
> "v4l2 device register failed (err=%d)\n", ret);
> return ret;
> }
> -
> /* allocate ent_devs */
> vimc->ent_devs = kcalloc(vimc->pipe_cfg->num_ents,
> sizeof(*vimc->ent_devs), GFP_KERNEL);
> @@ -236,9 +265,9 @@ static int vimc_register_devices(struct vimc_device *vimc)
>
> err_mdev_unregister:
> media_device_unregister(&vimc->mdev);
> - media_device_cleanup(&vimc->mdev);
> err_rm_subdevs:
> - vimc_rm_subdevs(vimc);
> + vimc_unregister_subdevs(vimc);
> + vimc_release_subdevs(vimc);
> kfree(vimc->ent_devs);
> err_v4l2_unregister:
> v4l2_device_unregister(&vimc->v4l2_dev);
> @@ -248,11 +277,9 @@ static int vimc_register_devices(struct vimc_device *vimc)
>
> static void vimc_unregister(struct vimc_device *vimc)
> {
> + vimc_unregister_subdevs(vimc);
> media_device_unregister(&vimc->mdev);
> - media_device_cleanup(&vimc->mdev);
> v4l2_device_unregister(&vimc->v4l2_dev);
> - kfree(vimc->ent_devs);
> - kfree(vimc);
> }
>
> static int vimc_probe(struct platform_device *pdev)
> @@ -285,7 +312,12 @@ static int vimc_probe(struct platform_device *pdev)
> kfree(vimc);
> return ret;
> }
> + /*
> + * the release cb is set only after successful registration.
> + * if the registration fails, we release directly from probe
> + */
>
> + vimc->v4l2_dev.release = vimc_v4l2_dev_release;
> platform_set_drvdata(pdev, vimc);
> return 0;
> }
> @@ -296,8 +328,8 @@ static int vimc_remove(struct platform_device *pdev)
>
> dev_dbg(&pdev->dev, "remove");
>
> - vimc_rm_subdevs(vimc);
> vimc_unregister(vimc);
> + v4l2_device_put(&vimc->v4l2_dev);
>
> return 0;
> }
> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
> index 34b98b235880..3beec7f95b47 100644
> --- a/drivers/media/platform/vimc/vimc-debayer.c
> +++ b/drivers/media/platform/vimc/vimc-debayer.c
> @@ -494,25 +494,21 @@ static const struct v4l2_ctrl_ops vimc_deb_ctrl_ops = {
> .s_ctrl = vimc_deb_s_ctrl,
> };
>
> -static void vimc_deb_release(struct v4l2_subdev *sd)
> +void vimc_deb_release(struct vimc_ent_device *ved)
> {
> struct vimc_deb_device *vdeb =
> - container_of(sd, struct vimc_deb_device, sd);
> + container_of(ved, struct vimc_deb_device, ved);
>
> v4l2_ctrl_handler_free(&vdeb->hdl);
> media_entity_cleanup(vdeb->ved.ent);
> kfree(vdeb);
> }
>
> -static const struct v4l2_subdev_internal_ops vimc_deb_int_ops = {
> - .release = vimc_deb_release,
> -};
> -
> -void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
> +void vimc_deb_unregister(struct vimc_ent_device *ved)
> {
> - struct vimc_deb_device *vdeb;
> + struct vimc_deb_device *vdeb =
> + container_of(ved, struct vimc_deb_device, ved);
>
> - vdeb = container_of(ved, struct vimc_deb_device, ved);
> v4l2_device_unregister_subdev(&vdeb->sd);
> }
>
> @@ -563,8 +559,7 @@ struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
> ret = vimc_ent_sd_register(&vdeb->ved, &vdeb->sd, v4l2_dev,
> vcfg_name,
> MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV, 2,
> - vdeb->pads,
> - &vimc_deb_int_ops, &vimc_deb_ops);
> + vdeb->pads, &vimc_deb_ops);
> if (ret)
> goto err_free_hdl;
>
> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> index cb2dc24c3e59..de9bee504b59 100644
> --- a/drivers/media/platform/vimc/vimc-scaler.c
> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> @@ -464,24 +464,20 @@ static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
> return vsca->src_frame;
> };
>
> -static void vimc_sca_release(struct v4l2_subdev *sd)
> +void vimc_sca_release(struct vimc_ent_device *ved)
> {
> struct vimc_sca_device *vsca =
> - container_of(sd, struct vimc_sca_device, sd);
> + container_of(ved, struct vimc_sca_device, ved);
>
> media_entity_cleanup(vsca->ved.ent);
> kfree(vsca);
> }
>
> -static const struct v4l2_subdev_internal_ops vimc_sca_int_ops = {
> - .release = vimc_sca_release,
> -};
> -
> -void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
> +void vimc_sca_unregister(struct vimc_ent_device *ved)
> {
> - struct vimc_sca_device *vsca;
> + struct vimc_sca_device *vsca =
> + container_of(ved, struct vimc_sca_device, ved);
>
> - vsca = container_of(ved, struct vimc_sca_device, ved);
> v4l2_device_unregister_subdev(&vsca->sd);
> }
>
> @@ -504,8 +500,7 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
> ret = vimc_ent_sd_register(&vsca->ved, &vsca->sd, v4l2_dev,
> vcfg_name,
> MEDIA_ENT_F_PROC_VIDEO_SCALER, 2,
> - vsca->pads,
> - &vimc_sca_int_ops, &vimc_sca_ops);
> + vsca->pads, &vimc_sca_ops);
> if (ret) {
> kfree(vsca);
> return NULL;
> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
> index 9788fe291193..14eeaf461e93 100644
> --- a/drivers/media/platform/vimc/vimc-sensor.c
> +++ b/drivers/media/platform/vimc/vimc-sensor.c
> @@ -279,10 +279,10 @@ static const struct v4l2_ctrl_ops vimc_sen_ctrl_ops = {
> .s_ctrl = vimc_sen_s_ctrl,
> };
>
> -static void vimc_sen_release(struct v4l2_subdev *sd)
> +void vimc_sen_release(struct vimc_ent_device *ved)
> {
> struct vimc_sen_device *vsen =
> - container_of(sd, struct vimc_sen_device, sd);
> + container_of(ved, struct vimc_sen_device, ved);
>
> v4l2_ctrl_handler_free(&vsen->hdl);
> tpg_free(&vsen->tpg);
> @@ -290,15 +290,11 @@ static void vimc_sen_release(struct v4l2_subdev *sd)
> kfree(vsen);
> }
>
> -static const struct v4l2_subdev_internal_ops vimc_sen_int_ops = {
> - .release = vimc_sen_release,
> -};
> -
> -void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
> +void vimc_sen_unregister(struct vimc_ent_device *ved)
> {
> - struct vimc_sen_device *vsen;
> + struct vimc_sen_device *vsen =
> + container_of(ved, struct vimc_sen_device, ved);
>
> - vsen = container_of(ved, struct vimc_sen_device, ved);
> v4l2_device_unregister_subdev(&vsen->sd);
> }
>
> @@ -365,7 +361,7 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
> ret = vimc_ent_sd_register(&vsen->ved, &vsen->sd, v4l2_dev,
> vcfg_name,
> MEDIA_ENT_F_CAM_SENSOR, 1, &vsen->pad,
> - &vimc_sen_int_ops, &vimc_sen_ops);
> + &vimc_sen_ops);
> if (ret)
> goto err_free_tpg;
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 4/6] media: vimc: capture: crash fix - synchronize call to vb2_queue_release when unregistering
2020-01-13 21:55 ` [PATCH v4 4/6] media: vimc: capture: crash fix - synchronize call to vb2_queue_release when unregistering Dafna Hirschfeld
@ 2020-01-14 1:31 ` Helen Koike
2020-01-14 11:37 ` Dafna Hirschfeld
2020-01-14 10:25 ` Hans Verkuil
1 sibling, 1 reply; 20+ messages in thread
From: Helen Koike @ 2020-01-14 1:31 UTC (permalink / raw)
To: Dafna Hirschfeld, linux-media; +Cc: ezequiel, skhan, hverkuil, kernel, dafna3
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@collabora.com>
> ---
> 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.
Also, I see that vivid doesn't even call vb2_queue_release() (is this a bug?).
Another note is that video_unregister_device() should be called before vb2_queue_release()
to avoid any other ioctls from userspace.
Helen
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 6/6] media: vimc: Track the media device by calling v4l2_device_get/put
2020-01-13 21:55 ` [PATCH v4 6/6] media: vimc: Track the media device by calling v4l2_device_get/put Dafna Hirschfeld
@ 2020-01-14 2:27 ` Helen Koike
2020-01-14 10:47 ` Hans Verkuil
1 sibling, 0 replies; 20+ messages in thread
From: Helen Koike @ 2020-01-14 2:27 UTC (permalink / raw)
To: Dafna Hirschfeld, linux-media; +Cc: ezequiel, skhan, hverkuil, kernel, dafna3
Hi Dafna,
On 1/13/20 7:55 PM, Dafna Hirschfeld wrote:
> After a successful media_device_register call, call v4l2_device_get().
> and set the media_devnode release callback to a function that
> calls v4l2_device_put().
> That should ensure that the v4l2_device's release callback is called
> when the very last user of any of the registered device nodes has
> closed its fh.
This patch shouldn't be required if you accept my suggestion in patch 3/6,
since the release function in v4l2_dev wouldn't be used.
Let me know what you think.
Regards,
Helen
>
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
> drivers/media/platform/vimc/vimc-core.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
> index 9d4e8bc89620..0f03e9cec075 100644
> --- a/drivers/media/platform/vimc/vimc-core.c
> +++ b/drivers/media/platform/vimc/vimc-core.c
> @@ -214,6 +214,14 @@ static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev)
> kfree(vimc);
> }
>
> +static void vimc_media_device_release(struct media_devnode *devnode)
> +{
> + struct media_device *mdev = devnode->media_dev;
> + struct vimc_device *vimc = container_of(mdev, struct vimc_device, mdev);
> +
> + v4l2_device_put(&vimc->v4l2_dev);
> +}
> +
> static int vimc_register_devices(struct vimc_device *vimc)
> {
> int ret;
> @@ -252,6 +260,8 @@ static int vimc_register_devices(struct vimc_device *vimc)
> goto err_rm_subdevs;
> }
>
> + v4l2_device_get(&vimc->v4l2_dev);
> + vimc->mdev.devnode->release = vimc_media_device_release;
> /* Expose all subdev's nodes*/
> ret = v4l2_device_register_subdev_nodes(&vimc->v4l2_dev);
> if (ret) {
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 4/6] media: vimc: capture: crash fix - synchronize call to vb2_queue_release when unregistering
2020-01-13 21:55 ` [PATCH v4 4/6] media: vimc: capture: crash fix - synchronize call to vb2_queue_release when unregistering Dafna Hirschfeld
2020-01-14 1:31 ` Helen Koike
@ 2020-01-14 10:25 ` Hans Verkuil
2020-01-14 11:17 ` Dafna Hirschfeld
1 sibling, 1 reply; 20+ messages in thread
From: Hans Verkuil @ 2020-01-14 10:25 UTC (permalink / raw)
To: Dafna Hirschfeld, linux-media
Cc: helen.koike, ezequiel, skhan, kernel, dafna3
On 1/13/20 10: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@collabora.com>
> ---
> 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);
I wonder if the vb2_queue_release call is needed at all.
What if you just delete it? When the filehandle is closed eventually, it will
call vb2_queue_release as well.
If you DO need to call this function here, then you indeed need to take the mutex.
But I think it is a good idea to add a comment here as well to explain why you
need to call vb2_queue_release().
Regards,
Hans
> video_unregister_device(&vcap->vdev);
> }
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 5/6] media: mc-devnode.c: set devnode->media_dev to NULL upon release instead of unregister
2020-01-13 21:55 ` [PATCH v4 5/6] media: mc-devnode.c: set devnode->media_dev to NULL upon release instead of unregister Dafna Hirschfeld
@ 2020-01-14 10:26 ` Hans Verkuil
0 siblings, 0 replies; 20+ messages in thread
From: Hans Verkuil @ 2020-01-14 10:26 UTC (permalink / raw)
To: Dafna Hirschfeld, linux-media
Cc: helen.koike, ezequiel, skhan, kernel, dafna3
On 1/13/20 10:55 PM, Dafna Hirschfeld wrote:
> media_devnode_release calls a release callback. Currently none of
> the drivers implement that callback but in the future the vimc
> driver and maybe others might add implementation for it.
> The release callback will want to access the driver's private data
> by using 'container_of(devnode->media_dev' therefore media_dev
> should be set to NULL only when the release callback returns.
>
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
> drivers/media/mc/mc-devnode.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
> index f11382afe23b..388c9051152a 100644
> --- a/drivers/media/mc/mc-devnode.c
> +++ b/drivers/media/mc/mc-devnode.c
> @@ -58,7 +58,7 @@ static void media_devnode_release(struct device *cd)
> /* Release media_devnode and perform other cleanups as needed. */
> if (devnode->release)
> devnode->release(devnode);
> -
> + devnode->media_dev = NULL;
This makes no sense since you free the whole devnode in the next line.
Regards,
Hans
> kfree(devnode);
> pr_debug("%s: Media Devnode Deallocated\n", __func__);
> }
> @@ -283,7 +283,6 @@ void media_devnode_unregister(struct media_devnode *devnode)
> mutex_lock(&media_devnode_lock);
> /* Delete the cdev on this minor as well */
> cdev_device_del(&devnode->cdev, &devnode->dev);
> - devnode->media_dev = NULL;
> mutex_unlock(&media_devnode_lock);
>
> put_device(&devnode->dev);
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 6/6] media: vimc: Track the media device by calling v4l2_device_get/put
2020-01-13 21:55 ` [PATCH v4 6/6] media: vimc: Track the media device by calling v4l2_device_get/put Dafna Hirschfeld
2020-01-14 2:27 ` Helen Koike
@ 2020-01-14 10:47 ` Hans Verkuil
1 sibling, 0 replies; 20+ messages in thread
From: Hans Verkuil @ 2020-01-14 10:47 UTC (permalink / raw)
To: Dafna Hirschfeld, linux-media
Cc: helen.koike, ezequiel, skhan, kernel, dafna3
Hi Dafna,
On 1/13/20 10:55 PM, Dafna Hirschfeld wrote:
> After a successful media_device_register call, call v4l2_device_get().
> and set the media_devnode release callback to a function that
> calls v4l2_device_put().
> That should ensure that the v4l2_device's release callback is called
> when the very last user of any of the registered device nodes has
> closed its fh.
>
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
> drivers/media/platform/vimc/vimc-core.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
> index 9d4e8bc89620..0f03e9cec075 100644
> --- a/drivers/media/platform/vimc/vimc-core.c
> +++ b/drivers/media/platform/vimc/vimc-core.c
> @@ -214,6 +214,14 @@ static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev)
> kfree(vimc);
> }
>
> +static void vimc_media_device_release(struct media_devnode *devnode)
> +{
> + struct media_device *mdev = devnode->media_dev;
> + struct vimc_device *vimc = container_of(mdev, struct vimc_device, mdev);
> +
> + v4l2_device_put(&vimc->v4l2_dev);
> +}
> +
> static int vimc_register_devices(struct vimc_device *vimc)
> {
> int ret;
> @@ -252,6 +260,8 @@ static int vimc_register_devices(struct vimc_device *vimc)
> goto err_rm_subdevs;
> }
>
> + v4l2_device_get(&vimc->v4l2_dev);
> + vimc->mdev.devnode->release = vimc_media_device_release;
> /* Expose all subdev's nodes*/
> ret = v4l2_device_register_subdev_nodes(&vimc->v4l2_dev);
> if (ret) {
>
I like the idea, but I think the roles of v4l2_device and media_device should
be swapped. Logically the media device is the top-level device, and the v4l2_device
sits below it. So rather than cleaning everything up in the v4l2_device release
callback, that should be done in the mdev.devnode->release callback.
So during the probe you need a call to get_device(&mdev.devnode->dev) and in the
v4l2_device release callback you call put_device(&mdev.devnode->dev). And the
mdev.devnode->release() callback is then used to clean everything up.
I think it is a good idea to add helper functions media_device_get/put that take
a media_device pointer as argument, but I'd do that as a new last patch, replacing
any get/put_device() calls for mdev.devnode in one go. There may be some
discussion about that, so having this as the last patch makes it easier to postpone
merging it if needed.
Regards,
Hans
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 3/6] media: vimc: use-after-free fix - release vimc in the v4l_device release
2020-01-14 0:22 ` Helen Koike
@ 2020-01-14 10:52 ` Dafna Hirschfeld
2020-01-14 11:34 ` Helen Koike
0 siblings, 1 reply; 20+ messages in thread
From: Dafna Hirschfeld @ 2020-01-14 10:52 UTC (permalink / raw)
To: Helen Koike, linux-media; +Cc: ezequiel, skhan, hverkuil, kernel, dafna3
Hi
On 14.01.20 02:22, Helen Koike wrote:
> Hi Dafna,
>
> Thanks for this patch.
>
> On 1/13/20 7:55 PM, Dafna Hirschfeld wrote:
>> A use-after-free bug occures when unbinding the device while it streams.
>> The 'struct vimc_ent_device' allocated for the 'Sensor A' is freed
>> when calling the sensor's 'rm' callback but the freed pointer is
>> later accessed in the function 'vimc_streamer_pipeline_terminate'.
>> To fix this bug, move the release callback of the vimc entities
>> and vimc_device to the release callback of v4l2_device.
>> The .rm callback of vimc_ent_config is replaced by two callbacks:
>> .unregister - this is called upon removing the device and
>> it unregisters the entity.
>> .release - this is called from the release callback of v4l2_device
>> and it frees the entity.
>> This ensures that the entities will be released when the last fh
>> of any of the devices is closed.
>
> I have mixed feelings about this patch, because another solution would
> be to make sure to stop all streams before releasing the subdevices,
> which could be done by calling 'rm' callback from the capture nodes first.
>
> Also I noticed that calling v4l2_device_unregister_subdev() from the subdevices
> is a bit useless, since they are already called from v4l2_device_unregister(), who
> already iterates through all subdevices.
>
Yes, this is true, I can remove those lines.
> So what I suggest is to remove all the 'rm' callbacks from subdevices, and leave
> only the 'rm' from the capture entities.
>
> Rename vimc_rm_subdevs() to vimc_rm_video_devices()
>
> And you can loop like this:
>
> -static void vimc_rm_subdevs(struct vimc_device *vimc)
> +static void vimc_rm_video_devices(struct vimc_device *vimc)
> {
> unsigned int i;
>
> for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
> - if (vimc->ent_devs[i])
> + if (vimc->ent_devs[i] && vimc->pipe_cfg->ents[i].rm)
> vimc->pipe_cfg->ents[i].rm(vimc, vimc->ent_devs[i]);
> }
>
> I didn't test, but I think it should work.
> Your patch would be cleaner (since you would be removing a bunch of lines),
> and we also would be re-using all the release functions from the media framework,
> which is nice for testing the core.
>
> What do you think?
The problem with releasing the subdevices in the releasing callback of
the subdevice struct (the release of v4l2_subdev_internal_ops)
is that the release cb is called when the last fh of the specific
subdevice is closed. This is a problem since the subdevice might still
be referenced from other places in the code (which is what happen now in
vimc) so the way to solve it is to release everything in the release
callback of v4l2_device. This was also suggested by Hans.
(See
https://lore.kernel.org/linux-media/a713212b-c760-f0c5-da5e-965d4fd87789@xs4all.nl/
)
I actaully don't see the point of having the release field in
v4l2_subdev_internal_ops if no driver implements it and it is actually
probably a bug if a dirver does implement it.
Dafna
>
> Thanks
> Helen
>
>>
>> The commands that cause the crash and the KASAN report:
>>
>> media-ctl -d platform:vimc -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
>> media-ctl -d platform:vimc -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
>> v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=1920,height=1440
>> v4l2-ctl -z platform:vimc -d "Raw Capture 0" -v pixelformat=BA81
>> v4l2-ctl --stream-mmap --stream-count=1000 -d /dev/video2 &
>> sleep 1
>> echo -n vimc.0 >/sys/bus/platform/drivers/vimc/unbind
>>
>> [ 188.417934] BUG: KASAN: use-after-free in vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
>> [ 188.420182] Read of size 8 at addr ffff8881e9c26008 by task bash/185
>> [ 188.421800]
>> [ 188.422223] CPU: 0 PID: 185 Comm: bash Not tainted 5.5.0-rc1+ #1
>> [ 188.423681] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
>> [ 188.425938] Call Trace:
>> [ 188.426610] dump_stack+0x75/0xa0
>> [ 188.427519] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
>> [ 188.429057] print_address_description.constprop.6+0x16/0x220
>> [ 188.430462] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
>> [ 188.431979] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
>> [ 188.433455] __kasan_report.cold.9+0x1a/0x40
>> [ 188.434518] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
>> [ 188.436010] kasan_report+0xe/0x20
>> [ 188.436859] vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
>> [ 188.438339] vimc_streamer_s_stream+0x8b/0x3c0 [vimc]
>> [ 188.439576] vimc_cap_stop_streaming+0x22/0x40 [vimc]
>> [ 188.440863] __vb2_queue_cancel+0x65/0x560 [videobuf2_common]
>> [ 188.442391] vb2_core_queue_release+0x19/0x50 [videobuf2_common]
>> [ 188.443974] vimc_cap_rm+0x10/0x20 [vimc]
>> [ 188.444986] vimc_rm_subdevs+0x9e/0xe0 [vimc]
>> [ 188.446179] vimc_remove+0x19/0x70 [vimc]
>> [ 188.447301] platform_drv_remove+0x2f/0x50
>> [ 188.448468] device_release_driver_internal+0x133/0x260
>> [ 188.449814] unbind_store+0x121/0x150
>> [ 188.450726] kernfs_fop_write+0x142/0x230
>> [ 188.451724] ? sysfs_kf_bin_read+0x100/0x100
>> [ 188.452826] vfs_write+0xdc/0x230
>> [ 188.453760] ksys_write+0xaf/0x140
>> [ 188.454702] ? __ia32_sys_read+0x40/0x40
>> [ 188.455773] ? __do_page_fault+0x473/0x620
>> [ 188.456780] do_syscall_64+0x5e/0x1a0
>> [ 188.457711] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> [ 188.459079] RIP: 0033:0x7f80f1f13504
>> [ 188.459969] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 48 8d 05 f9 61 0d 00 8b 00 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 41 54 49 89 d4 55 48 89 f5 53
>> [ 188.464445] RSP: 002b:00007ffd7e843b58 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
>> [ 188.466276] RAX: ffffffffffffffda RBX: 0000000000000006 RCX: 00007f80f1f13504
>> [ 188.467999] RDX: 0000000000000006 RSI: 000055ef2eb21b10 RDI: 0000000000000001
>> [ 188.469708] RBP: 000055ef2eb21b10 R08: 00007f80f1fe68c0 R09: 00007f80f1e26740
>> [ 188.471407] R10: 000055ef2eade010 R11: 0000000000000246 R12: 00007f80f1fe5760
>> [ 188.473381] R13: 0000000000000006 R14: 00007f80f1fe0760 R15: 0000000000000006
>> [ 188.475107]
>> [ 188.475500] Allocated by task 473:
>> [ 188.476351] save_stack+0x19/0x80
>> [ 188.477201] __kasan_kmalloc.constprop.6+0xc1/0xd0
>> [ 188.478507] vimc_sen_add+0x36/0x309 [vimc]
>> [ 188.479649] vimc_probe+0x1e2/0x530 [vimc]
>> [ 188.480776] platform_drv_probe+0x46/0xa0
>> [ 188.481829] really_probe+0x16c/0x520
>> [ 188.482732] driver_probe_device+0x114/0x170
>> [ 188.483783] device_driver_attach+0x85/0x90
>> [ 188.484800] __driver_attach+0xa8/0x190
>> [ 188.485734] bus_for_each_dev+0xe4/0x140
>> [ 188.486702] bus_add_driver+0x223/0x2d0
>> [ 188.487715] driver_register+0xca/0x140
>> [ 188.488767] 0xffffffffc037003d
>> [ 188.489635] do_one_initcall+0x86/0x28f
>> [ 188.490702] do_init_module+0xf8/0x340
>> [ 188.491773] load_module+0x3766/0x3a10
>> [ 188.492811] __do_sys_finit_module+0x11a/0x1b0
>> [ 188.494059] do_syscall_64+0x5e/0x1a0
>> [ 188.495079] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> [ 188.496481]
>> [ 188.496893] Freed by task 185:
>> [ 188.497670] save_stack+0x19/0x80
>> [ 188.498493] __kasan_slab_free+0x125/0x170
>> [ 188.499486] kfree+0x8c/0x230
>> [ 188.500254] v4l2_subdev_release+0x64/0x70 [videodev]
>> [ 188.501498] v4l2_device_release_subdev_node+0x1c/0x30 [videodev]
>> [ 188.502976] device_release+0x3c/0xd0
>> [ 188.503867] kobject_put+0xf4/0x240
>> [ 188.507802] vimc_rm_subdevs+0x9e/0xe0 [vimc]
>> [ 188.508846] vimc_remove+0x19/0x70 [vimc]
>> [ 188.509792] platform_drv_remove+0x2f/0x50
>> [ 188.510752] device_release_driver_internal+0x133/0x260
>> [ 188.512006] unbind_store+0x121/0x150
>> [ 188.512899] kernfs_fop_write+0x142/0x230
>> [ 188.513874] vfs_write+0xdc/0x230
>> [ 188.514698] ksys_write+0xaf/0x140
>> [ 188.515523] do_syscall_64+0x5e/0x1a0
>> [ 188.516543] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> [ 188.517710]
>> [ 188.518034] The buggy address belongs to the object at ffff8881e9c26000
>> [ 188.518034] which belongs to the cache kmalloc-4k of size 4096
>> [ 188.520528] The buggy address is located 8 bytes inside of
>> [ 188.520528] 4096-byte region [ffff8881e9c26000, ffff8881e9c27000)
>> [ 188.523015] The buggy address belongs to the page:
>> [ 188.524357] page:ffffea0007a70800 refcount:1 mapcount:0 mapping:ffff8881f6402140 index:0x0 compound_mapcount: 0
>> [ 188.527058] raw: 0200000000010200 dead000000000100 dead000000000122 ffff8881f6402140
>> [ 188.528983] raw: 0000000000000000 0000000000040004 00000001ffffffff 0000000000000000
>> [ 188.530883] page dumped because: kasan: bad access detected
>> [ 188.532336]
>> [ 188.532720] Memory state around the buggy address:
>> [ 188.533871] ffff8881e9c25f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>> [ 188.535631] ffff8881e9c25f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>> [ 188.537370] >ffff8881e9c26000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>> [ 188.538996] ^
>> [ 188.539812] ffff8881e9c26080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>> [ 188.541549] ffff8881e9c26100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>> drivers/media/platform/vimc/vimc-capture.c | 12 ++--
>> drivers/media/platform/vimc/vimc-common.c | 2 -
>> drivers/media/platform/vimc/vimc-common.h | 28 +++++----
>> drivers/media/platform/vimc/vimc-core.c | 68 ++++++++++++++++------
>> drivers/media/platform/vimc/vimc-debayer.c | 17 ++----
>> drivers/media/platform/vimc/vimc-scaler.c | 17 ++----
>> drivers/media/platform/vimc/vimc-sensor.c | 16 ++---
>> 7 files changed, 90 insertions(+), 70 deletions(-)
>>
>> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
>> index 9a78bb7826a8..c5a645f98c66 100644
>> --- a/drivers/media/platform/vimc/vimc-capture.c
>> +++ b/drivers/media/platform/vimc/vimc-capture.c
>> @@ -325,20 +325,20 @@ static const struct media_entity_operations vimc_cap_mops = {
>> .link_validate = vimc_vdev_link_validate,
>> };
>>
>> -static void vimc_cap_release(struct video_device *vdev)
>> +void vimc_cap_release(struct vimc_ent_device *ved)
>> {
>> struct vimc_cap_device *vcap =
>> - container_of(vdev, struct vimc_cap_device, vdev);
>> + container_of(ved, struct vimc_cap_device, ved);
>>
>> media_entity_cleanup(vcap->ved.ent);
>> kfree(vcap);
>> }
>>
>> -void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
>> +void vimc_cap_unregister(struct vimc_ent_device *ved)
>> {
>> - struct vimc_cap_device *vcap;
>> + struct vimc_cap_device *vcap =
>> + container_of(ved, struct vimc_cap_device, ved);
>>
>> - vcap = container_of(ved, struct vimc_cap_device, ved);
>> vb2_queue_release(&vcap->queue);
>> video_unregister_device(&vcap->vdev);
>> }
>> @@ -449,7 +449,7 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
>> vdev = &vcap->vdev;
>> vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
>> vdev->entity.ops = &vimc_cap_mops;
>> - vdev->release = vimc_cap_release;
>> + vdev->release = video_device_release_empty;
>> vdev->fops = &vimc_cap_fops;
>> vdev->ioctl_ops = &vimc_cap_ioctl_ops;
>> vdev->lock = &vcap->lock;
>> diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
>> index 16ce9f3b7c75..c95c17c048f2 100644
>> --- a/drivers/media/platform/vimc/vimc-common.c
>> +++ b/drivers/media/platform/vimc/vimc-common.c
>> @@ -327,7 +327,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
>> u32 function,
>> u16 num_pads,
>> struct media_pad *pads,
>> - const struct v4l2_subdev_internal_ops *sd_int_ops,
>> const struct v4l2_subdev_ops *sd_ops)
>> {
>> int ret;
>> @@ -337,7 +336,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
>>
>> /* Initialize the subdev */
>> v4l2_subdev_init(sd, sd_ops);
>> - sd->internal_ops = sd_int_ops;
>> sd->entity.function = function;
>> sd->entity.ops = &vimc_ent_sd_mops;
>> sd->owner = THIS_MODULE;
>> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
>> index 1b6ef7196f3c..99beb2134d40 100644
>> --- a/drivers/media/platform/vimc/vimc-common.h
>> +++ b/drivers/media/platform/vimc/vimc-common.h
>> @@ -125,16 +125,18 @@ struct vimc_device {
>> * @name entity name
>> * @ved pointer to vimc_ent_device (a node in the
>> * topology)
>> - * @add subdev add hook - initializes and registers
>> - * subdev called from vimc-core
>> - * @rm subdev rm hook - unregisters and frees
>> - * subdev called from vimc-core
>> + * @add initializes and registers
>> + * vim entity - called from vimc-core
>> + * @unregister unregisters vimc entity - called from vimc-core
>> + * @release releases vimc entity - called from the v4l2_dev
>> + * release callback
>> */
>> struct vimc_ent_config {
>> const char *name;
>> struct vimc_ent_device *(*add)(struct vimc_device *vimc,
>> const char *vcfg_name);
>> - void (*rm)(struct vimc_device *vimc, struct vimc_ent_device *ved);
>> + void (*unregister)(struct vimc_ent_device *ved);
>> + void (*release)(struct vimc_ent_device *ved);
>> };
>>
>> /**
>> @@ -145,22 +147,26 @@ struct vimc_ent_config {
>> */
>> bool vimc_is_source(struct media_entity *ent);
>>
>> -/* prototypes for vimc_ent_config add and rm hooks */
>> +/* prototypes for vimc_ent_config hooks */
>> struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
>> const char *vcfg_name);
>> -void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
>> +void vimc_cap_unregister(struct vimc_ent_device *ved);
>> +void vimc_cap_release(struct vimc_ent_device *ved);
>>
>> struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
>> const char *vcfg_name);
>> -void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
>> +void vimc_deb_unregister(struct vimc_ent_device *ved);
>> +void vimc_deb_release(struct vimc_ent_device *ved);
>>
>> struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
>> const char *vcfg_name);
>> -void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
>> +void vimc_sca_unregister(struct vimc_ent_device *ved);
>> +void vimc_sca_release(struct vimc_ent_device *ved);
>>
>> struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
>> const char *vcfg_name);
>> -void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
>> +void vimc_sen_unregister(struct vimc_ent_device *ved);
>> +void vimc_sen_release(struct vimc_ent_device *ved);
>>
>> /**
>> * vimc_pix_map_by_index - get vimc_pix_map struct by its index
>> @@ -195,7 +201,6 @@ const struct vimc_pix_map *vimc_pix_map_by_pixelformat(u32 pixelformat);
>> * @num_pads: number of pads to initialize
>> * @pads: the array of pads of the entity, the caller should set the
>> flags of the pads
>> - * @sd_int_ops: pointer to &struct v4l2_subdev_internal_ops
>> * @sd_ops: pointer to &struct v4l2_subdev_ops.
>> *
>> * Helper function initialize and register the struct vimc_ent_device and struct
>> @@ -208,7 +213,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
>> u32 function,
>> u16 num_pads,
>> struct media_pad *pads,
>> - const struct v4l2_subdev_internal_ops *sd_int_ops,
>> const struct v4l2_subdev_ops *sd_ops);
>>
>> /**
>> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
>> index 1c55e0382f09..9d4e8bc89620 100644
>> --- a/drivers/media/platform/vimc/vimc-core.c
>> +++ b/drivers/media/platform/vimc/vimc-core.c
>> @@ -48,48 +48,57 @@ static struct vimc_ent_config ent_config[] = {
>> {
>> .name = "Sensor A",
>> .add = vimc_sen_add,
>> - .rm = vimc_sen_rm,
>> + .unregister = vimc_sen_unregister,
>> + .release = vimc_sen_release,
>> },
>> {
>> .name = "Sensor B",
>> .add = vimc_sen_add,
>> - .rm = vimc_sen_rm,
>> + .unregister = vimc_sen_unregister,
>> + .release = vimc_sen_release,
>> },
>> {
>> .name = "Debayer A",
>> .add = vimc_deb_add,
>> - .rm = vimc_deb_rm,
>> + .unregister = vimc_deb_unregister,
>> + .release = vimc_deb_release,
>> },
>> {
>> .name = "Debayer B",
>> .add = vimc_deb_add,
>> - .rm = vimc_deb_rm,
>> + .unregister = vimc_deb_unregister,
>> + .release = vimc_deb_release,
>> },
>> {
>> .name = "Raw Capture 0",
>> .add = vimc_cap_add,
>> - .rm = vimc_cap_rm,
>> + .unregister = vimc_cap_unregister,
>> + .release = vimc_cap_release,
>> },
>> {
>> .name = "Raw Capture 1",
>> .add = vimc_cap_add,
>> - .rm = vimc_cap_rm,
>> + .unregister = vimc_cap_unregister,
>> + .release = vimc_cap_release,
>> },
>> {
>> /* TODO: change this to vimc-input when it is implemented */
>> .name = "RGB/YUV Input",
>> .add = vimc_sen_add,
>> - .rm = vimc_sen_rm,
>> + .unregister = vimc_sen_unregister,
>> + .release = vimc_sen_release,
>> },
>> {
>> .name = "Scaler",
>> .add = vimc_sca_add,
>> - .rm = vimc_sca_rm,
>> + .unregister = vimc_sca_unregister,
>> + .release = vimc_sca_release,
>> },
>> {
>> .name = "RGB/YUV Capture",
>> .add = vimc_cap_add,
>> - .rm = vimc_cap_rm,
>> + .unregister = vimc_cap_unregister,
>> + .release = vimc_cap_release,
>> },
>> };
>>
>> @@ -175,13 +184,34 @@ static int vimc_add_subdevs(struct vimc_device *vimc)
>> return 0;
>> }
>>
>> -static void vimc_rm_subdevs(struct vimc_device *vimc)
>> +static void vimc_release_subdevs(struct vimc_device *vimc)
>> {
>> unsigned int i;
>>
>> for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
>> if (vimc->ent_devs[i])
>> - vimc->pipe_cfg->ents[i].rm(vimc, vimc->ent_devs[i]);
>> + vimc->pipe_cfg->ents[i].release(vimc->ent_devs[i]);
>> +}
>> +
>> +
>> +static void vimc_unregister_subdevs(struct vimc_device *vimc)
>> +{
>> + unsigned int i;
>> +
>> + for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
>> + if (vimc->ent_devs[i])
>> + vimc->pipe_cfg->ents[i].unregister(vimc->ent_devs[i]);
>> +}
>> +
>> +static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev)
>> +{
>> + struct vimc_device *vimc =
>> + container_of(v4l2_dev, struct vimc_device, v4l2_dev);
>> +
>> + vimc_release_subdevs(vimc);
>> + media_device_cleanup(&vimc->mdev);
>> + kfree(vimc->ent_devs);
>> + kfree(vimc);
>> }
>>
>> static int vimc_register_devices(struct vimc_device *vimc)
>> @@ -195,7 +225,6 @@ static int vimc_register_devices(struct vimc_device *vimc)
>> "v4l2 device register failed (err=%d)\n", ret);
>> return ret;
>> }
>> -
>> /* allocate ent_devs */
>> vimc->ent_devs = kcalloc(vimc->pipe_cfg->num_ents,
>> sizeof(*vimc->ent_devs), GFP_KERNEL);
>> @@ -236,9 +265,9 @@ static int vimc_register_devices(struct vimc_device *vimc)
>>
>> err_mdev_unregister:
>> media_device_unregister(&vimc->mdev);
>> - media_device_cleanup(&vimc->mdev);
>> err_rm_subdevs:
>> - vimc_rm_subdevs(vimc);
>> + vimc_unregister_subdevs(vimc);
>> + vimc_release_subdevs(vimc);
>> kfree(vimc->ent_devs);
>> err_v4l2_unregister:
>> v4l2_device_unregister(&vimc->v4l2_dev);
>> @@ -248,11 +277,9 @@ static int vimc_register_devices(struct vimc_device *vimc)
>>
>> static void vimc_unregister(struct vimc_device *vimc)
>> {
>> + vimc_unregister_subdevs(vimc);
>> media_device_unregister(&vimc->mdev);
>> - media_device_cleanup(&vimc->mdev);
>> v4l2_device_unregister(&vimc->v4l2_dev);
>> - kfree(vimc->ent_devs);
>> - kfree(vimc);
>> }
>>
>> static int vimc_probe(struct platform_device *pdev)
>> @@ -285,7 +312,12 @@ static int vimc_probe(struct platform_device *pdev)
>> kfree(vimc);
>> return ret;
>> }
>> + /*
>> + * the release cb is set only after successful registration.
>> + * if the registration fails, we release directly from probe
>> + */
>>
>> + vimc->v4l2_dev.release = vimc_v4l2_dev_release;
>> platform_set_drvdata(pdev, vimc);
>> return 0;
>> }
>> @@ -296,8 +328,8 @@ static int vimc_remove(struct platform_device *pdev)
>>
>> dev_dbg(&pdev->dev, "remove");
>>
>> - vimc_rm_subdevs(vimc);
>> vimc_unregister(vimc);
>> + v4l2_device_put(&vimc->v4l2_dev);
>>
>> return 0;
>> }
>> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
>> index 34b98b235880..3beec7f95b47 100644
>> --- a/drivers/media/platform/vimc/vimc-debayer.c
>> +++ b/drivers/media/platform/vimc/vimc-debayer.c
>> @@ -494,25 +494,21 @@ static const struct v4l2_ctrl_ops vimc_deb_ctrl_ops = {
>> .s_ctrl = vimc_deb_s_ctrl,
>> };
>>
>> -static void vimc_deb_release(struct v4l2_subdev *sd)
>> +void vimc_deb_release(struct vimc_ent_device *ved)
>> {
>> struct vimc_deb_device *vdeb =
>> - container_of(sd, struct vimc_deb_device, sd);
>> + container_of(ved, struct vimc_deb_device, ved);
>>
>> v4l2_ctrl_handler_free(&vdeb->hdl);
>> media_entity_cleanup(vdeb->ved.ent);
>> kfree(vdeb);
>> }
>>
>> -static const struct v4l2_subdev_internal_ops vimc_deb_int_ops = {
>> - .release = vimc_deb_release,
>> -};
>> -
>> -void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
>> +void vimc_deb_unregister(struct vimc_ent_device *ved)
>> {
>> - struct vimc_deb_device *vdeb;
>> + struct vimc_deb_device *vdeb =
>> + container_of(ved, struct vimc_deb_device, ved);
>>
>> - vdeb = container_of(ved, struct vimc_deb_device, ved);
>> v4l2_device_unregister_subdev(&vdeb->sd);
>> }
>>
>> @@ -563,8 +559,7 @@ struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
>> ret = vimc_ent_sd_register(&vdeb->ved, &vdeb->sd, v4l2_dev,
>> vcfg_name,
>> MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV, 2,
>> - vdeb->pads,
>> - &vimc_deb_int_ops, &vimc_deb_ops);
>> + vdeb->pads, &vimc_deb_ops);
>> if (ret)
>> goto err_free_hdl;
>>
>> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
>> index cb2dc24c3e59..de9bee504b59 100644
>> --- a/drivers/media/platform/vimc/vimc-scaler.c
>> +++ b/drivers/media/platform/vimc/vimc-scaler.c
>> @@ -464,24 +464,20 @@ static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
>> return vsca->src_frame;
>> };
>>
>> -static void vimc_sca_release(struct v4l2_subdev *sd)
>> +void vimc_sca_release(struct vimc_ent_device *ved)
>> {
>> struct vimc_sca_device *vsca =
>> - container_of(sd, struct vimc_sca_device, sd);
>> + container_of(ved, struct vimc_sca_device, ved);
>>
>> media_entity_cleanup(vsca->ved.ent);
>> kfree(vsca);
>> }
>>
>> -static const struct v4l2_subdev_internal_ops vimc_sca_int_ops = {
>> - .release = vimc_sca_release,
>> -};
>> -
>> -void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
>> +void vimc_sca_unregister(struct vimc_ent_device *ved)
>> {
>> - struct vimc_sca_device *vsca;
>> + struct vimc_sca_device *vsca =
>> + container_of(ved, struct vimc_sca_device, ved);
>>
>> - vsca = container_of(ved, struct vimc_sca_device, ved);
>> v4l2_device_unregister_subdev(&vsca->sd);
>> }
>>
>> @@ -504,8 +500,7 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
>> ret = vimc_ent_sd_register(&vsca->ved, &vsca->sd, v4l2_dev,
>> vcfg_name,
>> MEDIA_ENT_F_PROC_VIDEO_SCALER, 2,
>> - vsca->pads,
>> - &vimc_sca_int_ops, &vimc_sca_ops);
>> + vsca->pads, &vimc_sca_ops);
>> if (ret) {
>> kfree(vsca);
>> return NULL;
>> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
>> index 9788fe291193..14eeaf461e93 100644
>> --- a/drivers/media/platform/vimc/vimc-sensor.c
>> +++ b/drivers/media/platform/vimc/vimc-sensor.c
>> @@ -279,10 +279,10 @@ static const struct v4l2_ctrl_ops vimc_sen_ctrl_ops = {
>> .s_ctrl = vimc_sen_s_ctrl,
>> };
>>
>> -static void vimc_sen_release(struct v4l2_subdev *sd)
>> +void vimc_sen_release(struct vimc_ent_device *ved)
>> {
>> struct vimc_sen_device *vsen =
>> - container_of(sd, struct vimc_sen_device, sd);
>> + container_of(ved, struct vimc_sen_device, ved);
>>
>> v4l2_ctrl_handler_free(&vsen->hdl);
>> tpg_free(&vsen->tpg);
>> @@ -290,15 +290,11 @@ static void vimc_sen_release(struct v4l2_subdev *sd)
>> kfree(vsen);
>> }
>>
>> -static const struct v4l2_subdev_internal_ops vimc_sen_int_ops = {
>> - .release = vimc_sen_release,
>> -};
>> -
>> -void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
>> +void vimc_sen_unregister(struct vimc_ent_device *ved)
>> {
>> - struct vimc_sen_device *vsen;
>> + struct vimc_sen_device *vsen =
>> + container_of(ved, struct vimc_sen_device, ved);
>>
>> - vsen = container_of(ved, struct vimc_sen_device, ved);
>> v4l2_device_unregister_subdev(&vsen->sd);
>> }
>>
>> @@ -365,7 +361,7 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
>> ret = vimc_ent_sd_register(&vsen->ved, &vsen->sd, v4l2_dev,
>> vcfg_name,
>> MEDIA_ENT_F_CAM_SENSOR, 1, &vsen->pad,
>> - &vimc_sen_int_ops, &vimc_sen_ops);
>> + &vimc_sen_ops);
>> if (ret)
>> goto err_free_tpg;
>>
>>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 4/6] media: vimc: capture: crash fix - synchronize call to vb2_queue_release when unregistering
2020-01-14 10:25 ` Hans Verkuil
@ 2020-01-14 11:17 ` Dafna Hirschfeld
2020-01-14 11:50 ` Hans Verkuil
0 siblings, 1 reply; 20+ messages in thread
From: Dafna Hirschfeld @ 2020-01-14 11:17 UTC (permalink / raw)
To: Hans Verkuil, linux-media; +Cc: helen.koike, ezequiel, skhan, kernel, dafna3
Hi
On 14.01.20 12:25, Hans Verkuil wrote:
> On 1/13/20 10: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@collabora.com>
>> ---
>> 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);
>
> I wonder if the vb2_queue_release call is needed at all.
>
> What if you just delete it? When the filehandle is closed eventually, it will
> call vb2_queue_release as well.
Hi, I actually tried that, the problem is that then the streaming is
stopped in the release of the video fh. The function
vimc_cap_stop_streaming calls media_pipeline_stop(&vcap->vdev.entity);
after the media entity is already unregistered and therefore
`entity->graph_obj.mdev` is NULL
but the function `media_pipeline_stop` tries to reference this mdev
which crashes.
The code in v4l2-dev.c actually solve this by calling
media_device_unregister_entity in the release cb and not immediately
when unregistered. The problem is that it is unregistered in
`media_device_unregister`
Dafna
>
> If you DO need to call this function here, then you indeed need to take the mutex.
> But I think it is a good idea to add a comment here as well to explain why you
> need to call vb2_queue_release().
>
> Regards,
>
> Hans
>
>> video_unregister_device(&vcap->vdev);
>> }
>>
>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 3/6] media: vimc: use-after-free fix - release vimc in the v4l_device release
2020-01-14 10:52 ` Dafna Hirschfeld
@ 2020-01-14 11:34 ` Helen Koike
0 siblings, 0 replies; 20+ messages in thread
From: Helen Koike @ 2020-01-14 11:34 UTC (permalink / raw)
To: Dafna Hirschfeld, linux-media; +Cc: ezequiel, skhan, hverkuil, kernel, dafna3
On 1/14/20 8:52 AM, Dafna Hirschfeld wrote:
> Hi
>
> On 14.01.20 02:22, Helen Koike wrote:
>> Hi Dafna,
>>
>> Thanks for this patch.
>>
>> On 1/13/20 7:55 PM, Dafna Hirschfeld wrote:
>>> A use-after-free bug occures when unbinding the device while it streams.
>>> The 'struct vimc_ent_device' allocated for the 'Sensor A' is freed
>>> when calling the sensor's 'rm' callback but the freed pointer is
>>> later accessed in the function 'vimc_streamer_pipeline_terminate'.
>>> To fix this bug, move the release callback of the vimc entities
>>> and vimc_device to the release callback of v4l2_device.
>>> The .rm callback of vimc_ent_config is replaced by two callbacks:
>>> .unregister - this is called upon removing the device and
>>> it unregisters the entity.
>>> .release - this is called from the release callback of v4l2_device
>>> and it frees the entity.
>>> This ensures that the entities will be released when the last fh
>>> of any of the devices is closed.
>>
>> I have mixed feelings about this patch, because another solution would
>> be to make sure to stop all streams before releasing the subdevices,
>> which could be done by calling 'rm' callback from the capture nodes first.
>>
>
>> Also I noticed that calling v4l2_device_unregister_subdev() from the subdevices
>> is a bit useless, since they are already called from v4l2_device_unregister(), who
>> already iterates through all subdevices.
>>
> Yes, this is true, I can remove those lines.
>> So what I suggest is to remove all the 'rm' callbacks from subdevices, and leave
>> only the 'rm' from the capture entities.
>>
>> Rename vimc_rm_subdevs() to vimc_rm_video_devices()
>>
>> And you can loop like this:
>>
>> -static void vimc_rm_subdevs(struct vimc_device *vimc)
>> +static void vimc_rm_video_devices(struct vimc_device *vimc)
>> {
>> unsigned int i;
>>
>> for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
>> - if (vimc->ent_devs[i])
>> + if (vimc->ent_devs[i] && vimc->pipe_cfg->ents[i].rm)
>> vimc->pipe_cfg->ents[i].rm(vimc, vimc->ent_devs[i]);
>> }
>>
>> I didn't test, but I think it should work.
>> Your patch would be cleaner (since you would be removing a bunch of lines),
>> and we also would be re-using all the release functions from the media framework,
>> which is nice for testing the core.
>>
>> What do you think?
>
> The problem with releasing the subdevices in the releasing callback of the subdevice struct (the release of v4l2_subdev_internal_ops)
> is that the release cb is called when the last fh of the specific subdevice is closed. This is a problem since the subdevice might still be referenced from other places in the code (which is what happen now in vimc) so the way to solve it is to release everything in the release callback of v4l2_device. This was also suggested by Hans.
> (See https://lore.kernel.org/linux-media/a713212b-c760-f0c5-da5e-965d4fd87789@xs4all.nl/ )
The only moment I see it having references in other places is when the stream is on. If you grant to stop the stream before
releasing the subdevices, then it should be fine to release the scaler while the user has a fh to the debayer for instance.
Unless you forsee any other references besides when the stream is on, what do you think?
>
> I actaully don't see the point of having the release field in
> v4l2_subdev_internal_ops if no driver implements it and it is actually
> probably a bug if a dirver does implement it.
This is a good point, this cb doesn't seem to be used by any drivers, then maybe using the release cb from v4l2_dev is the
way to go, and it is also safer in overall (and this is why I have mixed feelings).
Thanks
Helen
>
> Dafna
>
>
>
>
>
>>
>> Thanks
>> Helen
>>
>>>
>>> The commands that cause the crash and the KASAN report:
>>>
>>> media-ctl -d platform:vimc -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
>>> media-ctl -d platform:vimc -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
>>> v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=1920,height=1440
>>> v4l2-ctl -z platform:vimc -d "Raw Capture 0" -v pixelformat=BA81
>>> v4l2-ctl --stream-mmap --stream-count=1000 -d /dev/video2 &
>>> sleep 1
>>> echo -n vimc.0 >/sys/bus/platform/drivers/vimc/unbind
>>>
>>> [ 188.417934] BUG: KASAN: use-after-free in vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
>>> [ 188.420182] Read of size 8 at addr ffff8881e9c26008 by task bash/185
>>> [ 188.421800]
>>> [ 188.422223] CPU: 0 PID: 185 Comm: bash Not tainted 5.5.0-rc1+ #1
>>> [ 188.423681] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
>>> [ 188.425938] Call Trace:
>>> [ 188.426610] dump_stack+0x75/0xa0
>>> [ 188.427519] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
>>> [ 188.429057] print_address_description.constprop.6+0x16/0x220
>>> [ 188.430462] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
>>> [ 188.431979] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
>>> [ 188.433455] __kasan_report.cold.9+0x1a/0x40
>>> [ 188.434518] ? vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
>>> [ 188.436010] kasan_report+0xe/0x20
>>> [ 188.436859] vimc_streamer_pipeline_terminate+0x75/0x140 [vimc]
>>> [ 188.438339] vimc_streamer_s_stream+0x8b/0x3c0 [vimc]
>>> [ 188.439576] vimc_cap_stop_streaming+0x22/0x40 [vimc]
>>> [ 188.440863] __vb2_queue_cancel+0x65/0x560 [videobuf2_common]
>>> [ 188.442391] vb2_core_queue_release+0x19/0x50 [videobuf2_common]
>>> [ 188.443974] vimc_cap_rm+0x10/0x20 [vimc]
>>> [ 188.444986] vimc_rm_subdevs+0x9e/0xe0 [vimc]
>>> [ 188.446179] vimc_remove+0x19/0x70 [vimc]
>>> [ 188.447301] platform_drv_remove+0x2f/0x50
>>> [ 188.448468] device_release_driver_internal+0x133/0x260
>>> [ 188.449814] unbind_store+0x121/0x150
>>> [ 188.450726] kernfs_fop_write+0x142/0x230
>>> [ 188.451724] ? sysfs_kf_bin_read+0x100/0x100
>>> [ 188.452826] vfs_write+0xdc/0x230
>>> [ 188.453760] ksys_write+0xaf/0x140
>>> [ 188.454702] ? __ia32_sys_read+0x40/0x40
>>> [ 188.455773] ? __do_page_fault+0x473/0x620
>>> [ 188.456780] do_syscall_64+0x5e/0x1a0
>>> [ 188.457711] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>> [ 188.459079] RIP: 0033:0x7f80f1f13504
>>> [ 188.459969] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 48 8d 05 f9 61 0d 00 8b 00 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 41 54 49 89 d4 55 48 89 f5 53
>>> [ 188.464445] RSP: 002b:00007ffd7e843b58 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
>>> [ 188.466276] RAX: ffffffffffffffda RBX: 0000000000000006 RCX: 00007f80f1f13504
>>> [ 188.467999] RDX: 0000000000000006 RSI: 000055ef2eb21b10 RDI: 0000000000000001
>>> [ 188.469708] RBP: 000055ef2eb21b10 R08: 00007f80f1fe68c0 R09: 00007f80f1e26740
>>> [ 188.471407] R10: 000055ef2eade010 R11: 0000000000000246 R12: 00007f80f1fe5760
>>> [ 188.473381] R13: 0000000000000006 R14: 00007f80f1fe0760 R15: 0000000000000006
>>> [ 188.475107]
>>> [ 188.475500] Allocated by task 473:
>>> [ 188.476351] save_stack+0x19/0x80
>>> [ 188.477201] __kasan_kmalloc.constprop.6+0xc1/0xd0
>>> [ 188.478507] vimc_sen_add+0x36/0x309 [vimc]
>>> [ 188.479649] vimc_probe+0x1e2/0x530 [vimc]
>>> [ 188.480776] platform_drv_probe+0x46/0xa0
>>> [ 188.481829] really_probe+0x16c/0x520
>>> [ 188.482732] driver_probe_device+0x114/0x170
>>> [ 188.483783] device_driver_attach+0x85/0x90
>>> [ 188.484800] __driver_attach+0xa8/0x190
>>> [ 188.485734] bus_for_each_dev+0xe4/0x140
>>> [ 188.486702] bus_add_driver+0x223/0x2d0
>>> [ 188.487715] driver_register+0xca/0x140
>>> [ 188.488767] 0xffffffffc037003d
>>> [ 188.489635] do_one_initcall+0x86/0x28f
>>> [ 188.490702] do_init_module+0xf8/0x340
>>> [ 188.491773] load_module+0x3766/0x3a10
>>> [ 188.492811] __do_sys_finit_module+0x11a/0x1b0
>>> [ 188.494059] do_syscall_64+0x5e/0x1a0
>>> [ 188.495079] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>> [ 188.496481]
>>> [ 188.496893] Freed by task 185:
>>> [ 188.497670] save_stack+0x19/0x80
>>> [ 188.498493] __kasan_slab_free+0x125/0x170
>>> [ 188.499486] kfree+0x8c/0x230
>>> [ 188.500254] v4l2_subdev_release+0x64/0x70 [videodev]
>>> [ 188.501498] v4l2_device_release_subdev_node+0x1c/0x30 [videodev]
>>> [ 188.502976] device_release+0x3c/0xd0
>>> [ 188.503867] kobject_put+0xf4/0x240
>>> [ 188.507802] vimc_rm_subdevs+0x9e/0xe0 [vimc]
>>> [ 188.508846] vimc_remove+0x19/0x70 [vimc]
>>> [ 188.509792] platform_drv_remove+0x2f/0x50
>>> [ 188.510752] device_release_driver_internal+0x133/0x260
>>> [ 188.512006] unbind_store+0x121/0x150
>>> [ 188.512899] kernfs_fop_write+0x142/0x230
>>> [ 188.513874] vfs_write+0xdc/0x230
>>> [ 188.514698] ksys_write+0xaf/0x140
>>> [ 188.515523] do_syscall_64+0x5e/0x1a0
>>> [ 188.516543] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>> [ 188.517710]
>>> [ 188.518034] The buggy address belongs to the object at ffff8881e9c26000
>>> [ 188.518034] which belongs to the cache kmalloc-4k of size 4096
>>> [ 188.520528] The buggy address is located 8 bytes inside of
>>> [ 188.520528] 4096-byte region [ffff8881e9c26000, ffff8881e9c27000)
>>> [ 188.523015] The buggy address belongs to the page:
>>> [ 188.524357] page:ffffea0007a70800 refcount:1 mapcount:0 mapping:ffff8881f6402140 index:0x0 compound_mapcount: 0
>>> [ 188.527058] raw: 0200000000010200 dead000000000100 dead000000000122 ffff8881f6402140
>>> [ 188.528983] raw: 0000000000000000 0000000000040004 00000001ffffffff 0000000000000000
>>> [ 188.530883] page dumped because: kasan: bad access detected
>>> [ 188.532336]
>>> [ 188.532720] Memory state around the buggy address:
>>> [ 188.533871] ffff8881e9c25f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>>> [ 188.535631] ffff8881e9c25f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>>> [ 188.537370] >ffff8881e9c26000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>>> [ 188.538996] ^
>>> [ 188.539812] ffff8881e9c26080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>>> [ 188.541549] ffff8881e9c26100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>>>
>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>> ---
>>> drivers/media/platform/vimc/vimc-capture.c | 12 ++--
>>> drivers/media/platform/vimc/vimc-common.c | 2 -
>>> drivers/media/platform/vimc/vimc-common.h | 28 +++++----
>>> drivers/media/platform/vimc/vimc-core.c | 68 ++++++++++++++++------
>>> drivers/media/platform/vimc/vimc-debayer.c | 17 ++----
>>> drivers/media/platform/vimc/vimc-scaler.c | 17 ++----
>>> drivers/media/platform/vimc/vimc-sensor.c | 16 ++---
>>> 7 files changed, 90 insertions(+), 70 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
>>> index 9a78bb7826a8..c5a645f98c66 100644
>>> --- a/drivers/media/platform/vimc/vimc-capture.c
>>> +++ b/drivers/media/platform/vimc/vimc-capture.c
>>> @@ -325,20 +325,20 @@ static const struct media_entity_operations vimc_cap_mops = {
>>> .link_validate = vimc_vdev_link_validate,
>>> };
>>> -static void vimc_cap_release(struct video_device *vdev)
>>> +void vimc_cap_release(struct vimc_ent_device *ved)
>>> {
>>> struct vimc_cap_device *vcap =
>>> - container_of(vdev, struct vimc_cap_device, vdev);
>>> + container_of(ved, struct vimc_cap_device, ved);
>>> media_entity_cleanup(vcap->ved.ent);
>>> kfree(vcap);
>>> }
>>> -void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
>>> +void vimc_cap_unregister(struct vimc_ent_device *ved)
>>> {
>>> - struct vimc_cap_device *vcap;
>>> + struct vimc_cap_device *vcap =
>>> + container_of(ved, struct vimc_cap_device, ved);
>>> - vcap = container_of(ved, struct vimc_cap_device, ved);
>>> vb2_queue_release(&vcap->queue);
>>> video_unregister_device(&vcap->vdev);
>>> }
>>> @@ -449,7 +449,7 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
>>> vdev = &vcap->vdev;
>>> vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
>>> vdev->entity.ops = &vimc_cap_mops;
>>> - vdev->release = vimc_cap_release;
>>> + vdev->release = video_device_release_empty;
>>> vdev->fops = &vimc_cap_fops;
>>> vdev->ioctl_ops = &vimc_cap_ioctl_ops;
>>> vdev->lock = &vcap->lock;
>>> diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
>>> index 16ce9f3b7c75..c95c17c048f2 100644
>>> --- a/drivers/media/platform/vimc/vimc-common.c
>>> +++ b/drivers/media/platform/vimc/vimc-common.c
>>> @@ -327,7 +327,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
>>> u32 function,
>>> u16 num_pads,
>>> struct media_pad *pads,
>>> - const struct v4l2_subdev_internal_ops *sd_int_ops,
>>> const struct v4l2_subdev_ops *sd_ops)
>>> {
>>> int ret;
>>> @@ -337,7 +336,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
>>> /* Initialize the subdev */
>>> v4l2_subdev_init(sd, sd_ops);
>>> - sd->internal_ops = sd_int_ops;
>>> sd->entity.function = function;
>>> sd->entity.ops = &vimc_ent_sd_mops;
>>> sd->owner = THIS_MODULE;
>>> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
>>> index 1b6ef7196f3c..99beb2134d40 100644
>>> --- a/drivers/media/platform/vimc/vimc-common.h
>>> +++ b/drivers/media/platform/vimc/vimc-common.h
>>> @@ -125,16 +125,18 @@ struct vimc_device {
>>> * @name entity name
>>> * @ved pointer to vimc_ent_device (a node in the
>>> * topology)
>>> - * @add subdev add hook - initializes and registers
>>> - * subdev called from vimc-core
>>> - * @rm subdev rm hook - unregisters and frees
>>> - * subdev called from vimc-core
>>> + * @add initializes and registers
>>> + * vim entity - called from vimc-core
>>> + * @unregister unregisters vimc entity - called from vimc-core
>>> + * @release releases vimc entity - called from the v4l2_dev
>>> + * release callback
>>> */
>>> struct vimc_ent_config {
>>> const char *name;
>>> struct vimc_ent_device *(*add)(struct vimc_device *vimc,
>>> const char *vcfg_name);
>>> - void (*rm)(struct vimc_device *vimc, struct vimc_ent_device *ved);
>>> + void (*unregister)(struct vimc_ent_device *ved);
>>> + void (*release)(struct vimc_ent_device *ved);
>>> };
>>> /**
>>> @@ -145,22 +147,26 @@ struct vimc_ent_config {
>>> */
>>> bool vimc_is_source(struct media_entity *ent);
>>> -/* prototypes for vimc_ent_config add and rm hooks */
>>> +/* prototypes for vimc_ent_config hooks */
>>> struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
>>> const char *vcfg_name);
>>> -void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
>>> +void vimc_cap_unregister(struct vimc_ent_device *ved);
>>> +void vimc_cap_release(struct vimc_ent_device *ved);
>>> struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
>>> const char *vcfg_name);
>>> -void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
>>> +void vimc_deb_unregister(struct vimc_ent_device *ved);
>>> +void vimc_deb_release(struct vimc_ent_device *ved);
>>> struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
>>> const char *vcfg_name);
>>> -void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
>>> +void vimc_sca_unregister(struct vimc_ent_device *ved);
>>> +void vimc_sca_release(struct vimc_ent_device *ved);
>>> struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
>>> const char *vcfg_name);
>>> -void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
>>> +void vimc_sen_unregister(struct vimc_ent_device *ved);
>>> +void vimc_sen_release(struct vimc_ent_device *ved);
>>> /**
>>> * vimc_pix_map_by_index - get vimc_pix_map struct by its index
>>> @@ -195,7 +201,6 @@ const struct vimc_pix_map *vimc_pix_map_by_pixelformat(u32 pixelformat);
>>> * @num_pads: number of pads to initialize
>>> * @pads: the array of pads of the entity, the caller should set the
>>> flags of the pads
>>> - * @sd_int_ops: pointer to &struct v4l2_subdev_internal_ops
>>> * @sd_ops: pointer to &struct v4l2_subdev_ops.
>>> *
>>> * Helper function initialize and register the struct vimc_ent_device and struct
>>> @@ -208,7 +213,6 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
>>> u32 function,
>>> u16 num_pads,
>>> struct media_pad *pads,
>>> - const struct v4l2_subdev_internal_ops *sd_int_ops,
>>> const struct v4l2_subdev_ops *sd_ops);
>>> /**
>>> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
>>> index 1c55e0382f09..9d4e8bc89620 100644
>>> --- a/drivers/media/platform/vimc/vimc-core.c
>>> +++ b/drivers/media/platform/vimc/vimc-core.c
>>> @@ -48,48 +48,57 @@ static struct vimc_ent_config ent_config[] = {
>>> {
>>> .name = "Sensor A",
>>> .add = vimc_sen_add,
>>> - .rm = vimc_sen_rm,
>>> + .unregister = vimc_sen_unregister,
>>> + .release = vimc_sen_release,
>>> },
>>> {
>>> .name = "Sensor B",
>>> .add = vimc_sen_add,
>>> - .rm = vimc_sen_rm,
>>> + .unregister = vimc_sen_unregister,
>>> + .release = vimc_sen_release,
>>> },
>>> {
>>> .name = "Debayer A",
>>> .add = vimc_deb_add,
>>> - .rm = vimc_deb_rm,
>>> + .unregister = vimc_deb_unregister,
>>> + .release = vimc_deb_release,
>>> },
>>> {
>>> .name = "Debayer B",
>>> .add = vimc_deb_add,
>>> - .rm = vimc_deb_rm,
>>> + .unregister = vimc_deb_unregister,
>>> + .release = vimc_deb_release,
>>> },
>>> {
>>> .name = "Raw Capture 0",
>>> .add = vimc_cap_add,
>>> - .rm = vimc_cap_rm,
>>> + .unregister = vimc_cap_unregister,
>>> + .release = vimc_cap_release,
>>> },
>>> {
>>> .name = "Raw Capture 1",
>>> .add = vimc_cap_add,
>>> - .rm = vimc_cap_rm,
>>> + .unregister = vimc_cap_unregister,
>>> + .release = vimc_cap_release,
>>> },
>>> {
>>> /* TODO: change this to vimc-input when it is implemented */
>>> .name = "RGB/YUV Input",
>>> .add = vimc_sen_add,
>>> - .rm = vimc_sen_rm,
>>> + .unregister = vimc_sen_unregister,
>>> + .release = vimc_sen_release,
>>> },
>>> {
>>> .name = "Scaler",
>>> .add = vimc_sca_add,
>>> - .rm = vimc_sca_rm,
>>> + .unregister = vimc_sca_unregister,
>>> + .release = vimc_sca_release,
>>> },
>>> {
>>> .name = "RGB/YUV Capture",
>>> .add = vimc_cap_add,
>>> - .rm = vimc_cap_rm,
>>> + .unregister = vimc_cap_unregister,
>>> + .release = vimc_cap_release,
>>> },
>>> };
>>> @@ -175,13 +184,34 @@ static int vimc_add_subdevs(struct vimc_device *vimc)
>>> return 0;
>>> }
>>> -static void vimc_rm_subdevs(struct vimc_device *vimc)
>>> +static void vimc_release_subdevs(struct vimc_device *vimc)
>>> {
>>> unsigned int i;
>>> for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
>>> if (vimc->ent_devs[i])
>>> - vimc->pipe_cfg->ents[i].rm(vimc, vimc->ent_devs[i]);
>>> + vimc->pipe_cfg->ents[i].release(vimc->ent_devs[i]);
>>> +}
>>> +
>>> +
>>> +static void vimc_unregister_subdevs(struct vimc_device *vimc)
>>> +{
>>> + unsigned int i;
>>> +
>>> + for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
>>> + if (vimc->ent_devs[i])
>>> + vimc->pipe_cfg->ents[i].unregister(vimc->ent_devs[i]);
>>> +}
>>> +
>>> +static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev)
>>> +{
>>> + struct vimc_device *vimc =
>>> + container_of(v4l2_dev, struct vimc_device, v4l2_dev);
>>> +
>>> + vimc_release_subdevs(vimc);
>>> + media_device_cleanup(&vimc->mdev);
>>> + kfree(vimc->ent_devs);
>>> + kfree(vimc);
>>> }
>>> static int vimc_register_devices(struct vimc_device *vimc)
>>> @@ -195,7 +225,6 @@ static int vimc_register_devices(struct vimc_device *vimc)
>>> "v4l2 device register failed (err=%d)\n", ret);
>>> return ret;
>>> }
>>> -
>>> /* allocate ent_devs */
>>> vimc->ent_devs = kcalloc(vimc->pipe_cfg->num_ents,
>>> sizeof(*vimc->ent_devs), GFP_KERNEL);
>>> @@ -236,9 +265,9 @@ static int vimc_register_devices(struct vimc_device *vimc)
>>> err_mdev_unregister:
>>> media_device_unregister(&vimc->mdev);
>>> - media_device_cleanup(&vimc->mdev);
>>> err_rm_subdevs:
>>> - vimc_rm_subdevs(vimc);
>>> + vimc_unregister_subdevs(vimc);
>>> + vimc_release_subdevs(vimc);
>>> kfree(vimc->ent_devs);
>>> err_v4l2_unregister:
>>> v4l2_device_unregister(&vimc->v4l2_dev);
>>> @@ -248,11 +277,9 @@ static int vimc_register_devices(struct vimc_device *vimc)
>>> static void vimc_unregister(struct vimc_device *vimc)
>>> {
>>> + vimc_unregister_subdevs(vimc);
>>> media_device_unregister(&vimc->mdev);
>>> - media_device_cleanup(&vimc->mdev);
>>> v4l2_device_unregister(&vimc->v4l2_dev);
>>> - kfree(vimc->ent_devs);
>>> - kfree(vimc);
>>> }
>>> static int vimc_probe(struct platform_device *pdev)
>>> @@ -285,7 +312,12 @@ static int vimc_probe(struct platform_device *pdev)
>>> kfree(vimc);
>>> return ret;
>>> }
>>> + /*
>>> + * the release cb is set only after successful registration.
>>> + * if the registration fails, we release directly from probe
>>> + */
>>> + vimc->v4l2_dev.release = vimc_v4l2_dev_release;
>>> platform_set_drvdata(pdev, vimc);
>>> return 0;
>>> }
>>> @@ -296,8 +328,8 @@ static int vimc_remove(struct platform_device *pdev)
>>> dev_dbg(&pdev->dev, "remove");
>>> - vimc_rm_subdevs(vimc);
>>> vimc_unregister(vimc);
>>> + v4l2_device_put(&vimc->v4l2_dev);
>>> return 0;
>>> }
>>> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
>>> index 34b98b235880..3beec7f95b47 100644
>>> --- a/drivers/media/platform/vimc/vimc-debayer.c
>>> +++ b/drivers/media/platform/vimc/vimc-debayer.c
>>> @@ -494,25 +494,21 @@ static const struct v4l2_ctrl_ops vimc_deb_ctrl_ops = {
>>> .s_ctrl = vimc_deb_s_ctrl,
>>> };
>>> -static void vimc_deb_release(struct v4l2_subdev *sd)
>>> +void vimc_deb_release(struct vimc_ent_device *ved)
>>> {
>>> struct vimc_deb_device *vdeb =
>>> - container_of(sd, struct vimc_deb_device, sd);
>>> + container_of(ved, struct vimc_deb_device, ved);
>>> v4l2_ctrl_handler_free(&vdeb->hdl);
>>> media_entity_cleanup(vdeb->ved.ent);
>>> kfree(vdeb);
>>> }
>>> -static const struct v4l2_subdev_internal_ops vimc_deb_int_ops = {
>>> - .release = vimc_deb_release,
>>> -};
>>> -
>>> -void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
>>> +void vimc_deb_unregister(struct vimc_ent_device *ved)
>>> {
>>> - struct vimc_deb_device *vdeb;
>>> + struct vimc_deb_device *vdeb =
>>> + container_of(ved, struct vimc_deb_device, ved);
>>> - vdeb = container_of(ved, struct vimc_deb_device, ved);
>>> v4l2_device_unregister_subdev(&vdeb->sd);
>>> }
>>> @@ -563,8 +559,7 @@ struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
>>> ret = vimc_ent_sd_register(&vdeb->ved, &vdeb->sd, v4l2_dev,
>>> vcfg_name,
>>> MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV, 2,
>>> - vdeb->pads,
>>> - &vimc_deb_int_ops, &vimc_deb_ops);
>>> + vdeb->pads, &vimc_deb_ops);
>>> if (ret)
>>> goto err_free_hdl;
>>> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
>>> index cb2dc24c3e59..de9bee504b59 100644
>>> --- a/drivers/media/platform/vimc/vimc-scaler.c
>>> +++ b/drivers/media/platform/vimc/vimc-scaler.c
>>> @@ -464,24 +464,20 @@ static void *vimc_sca_process_frame(struct vimc_ent_device *ved,
>>> return vsca->src_frame;
>>> };
>>> -static void vimc_sca_release(struct v4l2_subdev *sd)
>>> +void vimc_sca_release(struct vimc_ent_device *ved)
>>> {
>>> struct vimc_sca_device *vsca =
>>> - container_of(sd, struct vimc_sca_device, sd);
>>> + container_of(ved, struct vimc_sca_device, ved);
>>> media_entity_cleanup(vsca->ved.ent);
>>> kfree(vsca);
>>> }
>>> -static const struct v4l2_subdev_internal_ops vimc_sca_int_ops = {
>>> - .release = vimc_sca_release,
>>> -};
>>> -
>>> -void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
>>> +void vimc_sca_unregister(struct vimc_ent_device *ved)
>>> {
>>> - struct vimc_sca_device *vsca;
>>> + struct vimc_sca_device *vsca =
>>> + container_of(ved, struct vimc_sca_device, ved);
>>> - vsca = container_of(ved, struct vimc_sca_device, ved);
>>> v4l2_device_unregister_subdev(&vsca->sd);
>>> }
>>> @@ -504,8 +500,7 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
>>> ret = vimc_ent_sd_register(&vsca->ved, &vsca->sd, v4l2_dev,
>>> vcfg_name,
>>> MEDIA_ENT_F_PROC_VIDEO_SCALER, 2,
>>> - vsca->pads,
>>> - &vimc_sca_int_ops, &vimc_sca_ops);
>>> + vsca->pads, &vimc_sca_ops);
>>> if (ret) {
>>> kfree(vsca);
>>> return NULL;
>>> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
>>> index 9788fe291193..14eeaf461e93 100644
>>> --- a/drivers/media/platform/vimc/vimc-sensor.c
>>> +++ b/drivers/media/platform/vimc/vimc-sensor.c
>>> @@ -279,10 +279,10 @@ static const struct v4l2_ctrl_ops vimc_sen_ctrl_ops = {
>>> .s_ctrl = vimc_sen_s_ctrl,
>>> };
>>> -static void vimc_sen_release(struct v4l2_subdev *sd)
>>> +void vimc_sen_release(struct vimc_ent_device *ved)
>>> {
>>> struct vimc_sen_device *vsen =
>>> - container_of(sd, struct vimc_sen_device, sd);
>>> + container_of(ved, struct vimc_sen_device, ved);
>>> v4l2_ctrl_handler_free(&vsen->hdl);
>>> tpg_free(&vsen->tpg);
>>> @@ -290,15 +290,11 @@ static void vimc_sen_release(struct v4l2_subdev *sd)
>>> kfree(vsen);
>>> }
>>> -static const struct v4l2_subdev_internal_ops vimc_sen_int_ops = {
>>> - .release = vimc_sen_release,
>>> -};
>>> -
>>> -void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
>>> +void vimc_sen_unregister(struct vimc_ent_device *ved)
>>> {
>>> - struct vimc_sen_device *vsen;
>>> + struct vimc_sen_device *vsen =
>>> + container_of(ved, struct vimc_sen_device, ved);
>>> - vsen = container_of(ved, struct vimc_sen_device, ved);
>>> v4l2_device_unregister_subdev(&vsen->sd);
>>> }
>>> @@ -365,7 +361,7 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
>>> ret = vimc_ent_sd_register(&vsen->ved, &vsen->sd, v4l2_dev,
>>> vcfg_name,
>>> MEDIA_ENT_F_CAM_SENSOR, 1, &vsen->pad,
>>> - &vimc_sen_int_ops, &vimc_sen_ops);
>>> + &vimc_sen_ops);
>>> if (ret)
>>> goto err_free_tpg;
>>>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 4/6] media: vimc: capture: crash fix - synchronize call to vb2_queue_release when unregistering
2020-01-14 1:31 ` Helen Koike
@ 2020-01-14 11:37 ` Dafna Hirschfeld
0 siblings, 0 replies; 20+ messages in thread
From: Dafna Hirschfeld @ 2020-01-14 11:37 UTC (permalink / raw)
To: Helen Koike, linux-media; +Cc: ezequiel, skhan, hverkuil, kernel, dafna3
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@collabora.com>
>> ---
>> 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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 4/6] media: vimc: capture: crash fix - synchronize call to vb2_queue_release when unregistering
2020-01-14 11:17 ` Dafna Hirschfeld
@ 2020-01-14 11:50 ` Hans Verkuil
2020-01-23 13:55 ` Dafna Hirschfeld
0 siblings, 1 reply; 20+ messages in thread
From: Hans Verkuil @ 2020-01-14 11:50 UTC (permalink / raw)
To: Dafna Hirschfeld, linux-media
Cc: helen.koike, ezequiel, skhan, kernel, dafna3
On 1/14/20 12:17 PM, Dafna Hirschfeld wrote:
> Hi
>
> On 14.01.20 12:25, Hans Verkuil wrote:
>> On 1/13/20 10: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@collabora.com>
>>> ---
>>> 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);
>>
>> I wonder if the vb2_queue_release call is needed at all.
>>
>> What if you just delete it? When the filehandle is closed eventually, it will
>> call vb2_queue_release as well.
>
> Hi, I actually tried that, the problem is that then the streaming is
> stopped in the release of the video fh. The function
> vimc_cap_stop_streaming calls media_pipeline_stop(&vcap->vdev.entity);
> after the media entity is already unregistered and therefore
> `entity->graph_obj.mdev` is NULL
> but the function `media_pipeline_stop` tries to reference this mdev
> which crashes.
>
> The code in v4l2-dev.c actually solve this by calling
> media_device_unregister_entity in the release cb and not immediately
> when unregistered. The problem is that it is unregistered in
> `media_device_unregister`
I really don't like all the cleanup that happens in media_device_unregister.
That is really stuff that should happen in the media_devnode release callback.
This whole problem goes away if it is done like that, I believe.
Basically when you unregister a device it should just unregister the device node,
mark it as unregistered and cause all file operations except release() to fail.
The actual cleanup should be postponed until the last user closes the fh.
It would be interesting if you can experiment with this.
Regards,
Hans
>
> Dafna
>
>
>
>>
>> If you DO need to call this function here, then you indeed need to take the mutex.
>> But I think it is a good idea to add a comment here as well to explain why you
>> need to call vb2_queue_release().
>>
>> Regards,
>>
>> Hans
>>
>>> video_unregister_device(&vcap->vdev);
>>> }
>>>
>>>
>>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 4/6] media: vimc: capture: crash fix - synchronize call to vb2_queue_release when unregistering
2020-01-14 11:50 ` Hans Verkuil
@ 2020-01-23 13:55 ` Dafna Hirschfeld
2020-01-23 14:22 ` Hans Verkuil
0 siblings, 1 reply; 20+ messages in thread
From: Dafna Hirschfeld @ 2020-01-23 13:55 UTC (permalink / raw)
To: Hans Verkuil, linux-media; +Cc: helen.koike, ezequiel, skhan, kernel, dafna3
On 14.01.20 12:50, Hans Verkuil wrote:
> On 1/14/20 12:17 PM, Dafna Hirschfeld wrote:
>> Hi
>>
>> On 14.01.20 12:25, Hans Verkuil wrote:
>>> On 1/13/20 10: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@collabora.com>
>>>> ---
>>>> 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);
>>>
>>> I wonder if the vb2_queue_release call is needed at all.
>>>
>>> What if you just delete it? When the filehandle is closed eventually, it will
>>> call vb2_queue_release as well.
>>
>> Hi, I actually tried that, the problem is that then the streaming is
>> stopped in the release of the video fh. The function
>> vimc_cap_stop_streaming calls media_pipeline_stop(&vcap->vdev.entity);
>> after the media entity is already unregistered and therefore
>> `entity->graph_obj.mdev` is NULL
>> but the function `media_pipeline_stop` tries to reference this mdev
>> which crashes.
>>
>> The code in v4l2-dev.c actually solve this by calling
>> media_device_unregister_entity in the release cb and not immediately
>> when unregistered. The problem is that it is unregistered in
>> `media_device_unregister`
>
> I really don't like all the cleanup that happens in media_device_unregister.
> That is really stuff that should happen in the media_devnode release callback.
>
> This whole problem goes away if it is done like that, I believe.
>
> Basically when you unregister a device it should just unregister the device node,
> mark it as unregistered and cause all file operations except release() to fail.
>
> The actual cleanup should be postponed until the last user closes the fh.
>
> It would be interesting if you can experiment with this.
This also means removing the call to `media_device_unregister_entity` in
v4l2_device_release in v4l2-dev.c ?
Also, according to the documentation the drivers should call `media_devnode_remove`
but this should probably also postpone to media_devnode release right?
Dafna
>
> Regards,
>
> Hans
>
>>
>> Dafna
>>
>>
>>
>>>
>>> If you DO need to call this function here, then you indeed need to take the mutex.
>>> But I think it is a good idea to add a comment here as well to explain why you
>>> need to call vb2_queue_release().
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>> video_unregister_device(&vcap->vdev);
>>>> }
>>>>
>>>>
>>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 4/6] media: vimc: capture: crash fix - synchronize call to vb2_queue_release when unregistering
2020-01-23 13:55 ` Dafna Hirschfeld
@ 2020-01-23 14:22 ` Hans Verkuil
0 siblings, 0 replies; 20+ messages in thread
From: Hans Verkuil @ 2020-01-23 14:22 UTC (permalink / raw)
To: Dafna Hirschfeld, linux-media
Cc: helen.koike, ezequiel, skhan, kernel, dafna3
On 1/23/20 2:55 PM, Dafna Hirschfeld wrote:
>
>
> On 14.01.20 12:50, Hans Verkuil wrote:
>> On 1/14/20 12:17 PM, Dafna Hirschfeld wrote:
>>> Hi
>>>
>>> On 14.01.20 12:25, Hans Verkuil wrote:
>>>> On 1/13/20 10: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@collabora.com>
>>>>> ---
>>>>> 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);
>>>>
>>>> I wonder if the vb2_queue_release call is needed at all.
>>>>
>>>> What if you just delete it? When the filehandle is closed eventually, it will
>>>> call vb2_queue_release as well.
>>>
>>> Hi, I actually tried that, the problem is that then the streaming is
>>> stopped in the release of the video fh. The function
>>> vimc_cap_stop_streaming calls media_pipeline_stop(&vcap->vdev.entity);
>>> after the media entity is already unregistered and therefore
>>> `entity->graph_obj.mdev` is NULL
>>> but the function `media_pipeline_stop` tries to reference this mdev
>>> which crashes.
>>>
>>> The code in v4l2-dev.c actually solve this by calling
>>> media_device_unregister_entity in the release cb and not immediately
>>> when unregistered. The problem is that it is unregistered in
>>> `media_device_unregister`
>>
>> I really don't like all the cleanup that happens in media_device_unregister.
>> That is really stuff that should happen in the media_devnode release callback.
>>
>> This whole problem goes away if it is done like that, I believe.
>>
>> Basically when you unregister a device it should just unregister the device node,
>> mark it as unregistered and cause all file operations except release() to fail.
>>
>> The actual cleanup should be postponed until the last user closes the fh.
>>
>> It would be interesting if you can experiment with this.
>
> This also means removing the call to `media_device_unregister_entity` in
> v4l2_device_release in v4l2-dev.c ?
Actually, I think the media topology should be updated when the device node is
unregistered: that's the moment the video node is removed in /dev and the topology
should reflect that.
> Also, according to the documentation the drivers should call `media_devnode_remove`
> but this should probably also postpone to media_devnode release right?
Same here: this should be done at unregistration time.
You don't want to update the topology only at release time since that will cause
problems in scenarios where entities are dynamically added/removed. Say that a
DMA capture entity is removed (i.e. the corresponding video device is unregistered),
then when you query the MC topology after that's done it should no longer show that
entity, even if some application still has a filehandle open on the unregistered
video device.
That's actually a nice test to check with a configfs enabled vimc.
Regards,
Hans
>
> Dafna
>
>>
>> Regards,
>>
>> Hans
>>
>>>
>>> Dafna
>>>
>>>
>>>
>>>>
>>>> If you DO need to call this function here, then you indeed need to take the mutex.
>>>> But I think it is a good idea to add a comment here as well to explain why you
>>>> need to call vb2_queue_release().
>>>>
>>>> Regards,
>>>>
>>>> Hans
>>>>
>>>>> video_unregister_device(&vcap->vdev);
>>>>> }
>>>>>
>>>>
>>
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-01-23 14:22 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-13 21:55 [PATCH v4 0/6] media: vimc: race condition fixes Dafna Hirschfeld
2020-01-13 21:55 ` [PATCH v4 1/6] media: vimc: replace vimc->pdev.dev with vimc->mdev.dev Dafna Hirschfeld
2020-01-13 21:55 ` [PATCH v4 2/6] media: vimc: allocate vimc_device dynamically Dafna Hirschfeld
2020-01-13 21:55 ` [PATCH v4 3/6] media: vimc: use-after-free fix - release vimc in the v4l_device release Dafna Hirschfeld
2020-01-14 0:22 ` Helen Koike
2020-01-14 10:52 ` Dafna Hirschfeld
2020-01-14 11:34 ` Helen Koike
2020-01-13 21:55 ` [PATCH v4 4/6] media: vimc: capture: crash fix - synchronize call to vb2_queue_release when unregistering Dafna Hirschfeld
2020-01-14 1:31 ` Helen Koike
2020-01-14 11:37 ` Dafna Hirschfeld
2020-01-14 10:25 ` Hans Verkuil
2020-01-14 11:17 ` Dafna Hirschfeld
2020-01-14 11:50 ` Hans Verkuil
2020-01-23 13:55 ` Dafna Hirschfeld
2020-01-23 14:22 ` Hans Verkuil
2020-01-13 21:55 ` [PATCH v4 5/6] media: mc-devnode.c: set devnode->media_dev to NULL upon release instead of unregister Dafna Hirschfeld
2020-01-14 10:26 ` Hans Verkuil
2020-01-13 21:55 ` [PATCH v4 6/6] media: vimc: Track the media device by calling v4l2_device_get/put Dafna Hirschfeld
2020-01-14 2:27 ` Helen Koike
2020-01-14 10:47 ` Hans Verkuil
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).