* [PATCHv4 1/3] media: venus: fix enc/dec destruction order
2024-10-25 10:50 [PATCHv4 0/3] media: venus: close() fixes Sergey Senozhatsky
@ 2024-10-25 10:50 ` Sergey Senozhatsky
2024-10-25 10:50 ` [PATCHv4 2/3] media: venus: sync with threaded IRQ during inst destruction Sergey Senozhatsky
2024-10-25 10:50 ` [PATCHv4 3/3] media: venus: factor out inst destruction routine Sergey Senozhatsky
2 siblings, 0 replies; 6+ messages in thread
From: Sergey Senozhatsky @ 2024-10-25 10:50 UTC (permalink / raw)
To: Stanimir Varbanov, Vikash Garodia
Cc: Dikshita Agarwal, Bryan O'Donoghue, linux-media, linux-kernel,
Sergey Senozhatsky, Tomasz Figa
We destroy mutex-es too early as they are still taken in
v4l2_fh_exit()->v4l2_event_unsubscribe()->v4l2_ctrl_find().
We should destroy mutex-es right before kfree(). Also
do not vdec_ctrl_deinit() before v4l2_fh_exit().
Fixes: 7472c1c69138 ("[media] media: venus: vdec: add video decoder files")
Suggested-by: Tomasz Figa <tfiga@google.com>
Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
drivers/media/platform/qcom/venus/vdec.c | 7 ++++---
drivers/media/platform/qcom/venus/venc.c | 6 +++---
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 6252a6b3d4ba..0013c4704f03 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -1752,13 +1752,14 @@ static int vdec_close(struct file *file)
cancel_work_sync(&inst->delayed_process_work);
v4l2_m2m_ctx_release(inst->m2m_ctx);
v4l2_m2m_release(inst->m2m_dev);
- vdec_ctrl_deinit(inst);
ida_destroy(&inst->dpb_ids);
hfi_session_destroy(inst);
- mutex_destroy(&inst->lock);
- mutex_destroy(&inst->ctx_q_lock);
v4l2_fh_del(&inst->fh);
v4l2_fh_exit(&inst->fh);
+ vdec_ctrl_deinit(inst);
+
+ mutex_destroy(&inst->lock);
+ mutex_destroy(&inst->ctx_q_lock);
vdec_pm_put(inst, false);
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 322a7737e2c7..6a26a6592424 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -1519,14 +1519,14 @@ static int venc_close(struct file *file)
v4l2_m2m_ctx_release(inst->m2m_ctx);
v4l2_m2m_release(inst->m2m_dev);
- venc_ctrl_deinit(inst);
hfi_session_destroy(inst);
- mutex_destroy(&inst->lock);
- mutex_destroy(&inst->ctx_q_lock);
v4l2_fh_del(&inst->fh);
v4l2_fh_exit(&inst->fh);
+ venc_ctrl_deinit(inst);
inst->enc_state = VENUS_ENC_STATE_DEINIT;
+ mutex_destroy(&inst->lock);
+ mutex_destroy(&inst->ctx_q_lock);
venc_pm_put(inst, false);
--
2.47.0.163.g1226f6d8fa-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCHv4 2/3] media: venus: sync with threaded IRQ during inst destruction
2024-10-25 10:50 [PATCHv4 0/3] media: venus: close() fixes Sergey Senozhatsky
2024-10-25 10:50 ` [PATCHv4 1/3] media: venus: fix enc/dec destruction order Sergey Senozhatsky
@ 2024-10-25 10:50 ` Sergey Senozhatsky
2024-10-25 10:50 ` [PATCHv4 3/3] media: venus: factor out inst destruction routine Sergey Senozhatsky
2 siblings, 0 replies; 6+ messages in thread
From: Sergey Senozhatsky @ 2024-10-25 10:50 UTC (permalink / raw)
To: Stanimir Varbanov, Vikash Garodia
Cc: Dikshita Agarwal, Bryan O'Donoghue, linux-media, linux-kernel,
Sergey Senozhatsky
When destroying an inst we should make sure that we don't race
against threaded IRQ (or pending IRQ), otherwise we can concurrently
kfree() inst context and inst itself.
BUG: KASAN: slab-use-after-free in vb2_queue_error+0x80/0x90
Call trace:
dump_backtrace+0x1c4/0x1f8
show_stack+0x38/0x60
dump_stack_lvl+0x168/0x1f0
print_report+0x170/0x4c8
kasan_report+0x94/0xd0
__asan_report_load2_noabort+0x20/0x30
vb2_queue_error+0x80/0x90
venus_helper_vb2_queue_error+0x54/0x78
venc_event_notify+0xec/0x158
hfi_event_notify+0x878/0xd20
hfi_process_msg_packet+0x27c/0x4e0
venus_isr_thread+0x258/0x6e8
hfi_isr_thread+0x70/0x90
venus_isr_thread+0x34/0x50
irq_thread_fn+0x88/0x130
irq_thread+0x160/0x2c0
kthread+0x294/0x328
ret_from_fork+0x10/0x20
Allocated by task 20291:
kasan_set_track+0x4c/0x80
kasan_save_alloc_info+0x28/0x38
__kasan_kmalloc+0x84/0xa0
kmalloc_trace+0x7c/0x98
v4l2_m2m_ctx_init+0x74/0x280
venc_open+0x444/0x6d0
v4l2_open+0x19c/0x2a0
chrdev_open+0x374/0x3f0
do_dentry_open+0x710/0x10a8
vfs_open+0x88/0xa8
path_openat+0x1e6c/0x2700
do_filp_open+0x1a4/0x2e0
do_sys_openat2+0xe8/0x508
do_sys_open+0x15c/0x1a0
__arm64_sys_openat+0xa8/0xc8
invoke_syscall+0xdc/0x270
el0_svc_common+0x1ec/0x250
do_el0_svc+0x54/0x70
el0_svc+0x50/0xe8
el0t_64_sync_handler+0x48/0x120
el0t_64_sync+0x1a8/0x1b0
Rearrange inst destruction. First remove the inst from the
core->instances list, second synchronize IRQ/IRQ-thread to
make sure that nothing else would see the inst while we take
it down.
Fixes: 7472c1c69138 ("[media] media: venus: vdec: add video decoder files")
Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
drivers/media/platform/qcom/venus/vdec.c | 12 +++++++++++-
drivers/media/platform/qcom/venus/venc.c | 12 +++++++++++-
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 0013c4704f03..b3192a36f388 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -1750,10 +1750,20 @@ static int vdec_close(struct file *file)
vdec_pm_get(inst);
cancel_work_sync(&inst->delayed_process_work);
+ /*
+ * First, remove the inst from the ->instances list, so that
+ * to_instance() will return NULL.
+ */
+ hfi_session_destroy(inst);
+ /*
+ * Second, make sure we don't have IRQ/IRQ-thread currently running
+ * or pending execution, which would race with the inst destruction.
+ */
+ synchronize_irq(inst->core->irq);
+
v4l2_m2m_ctx_release(inst->m2m_ctx);
v4l2_m2m_release(inst->m2m_dev);
ida_destroy(&inst->dpb_ids);
- hfi_session_destroy(inst);
v4l2_fh_del(&inst->fh);
v4l2_fh_exit(&inst->fh);
vdec_ctrl_deinit(inst);
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 6a26a6592424..36981ce448f5 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -1517,9 +1517,19 @@ static int venc_close(struct file *file)
venc_pm_get(inst);
+ /*
+ * First, remove the inst from the ->instances list, so that
+ * to_instance() will return NULL.
+ */
+ hfi_session_destroy(inst);
+ /*
+ * Second, make sure we don't have IRQ/IRQ-thread currently running
+ * or pending execution, which would race with the inst destruction.
+ */
+ synchronize_irq(inst->core->irq);
+
v4l2_m2m_ctx_release(inst->m2m_ctx);
v4l2_m2m_release(inst->m2m_dev);
- hfi_session_destroy(inst);
v4l2_fh_del(&inst->fh);
v4l2_fh_exit(&inst->fh);
venc_ctrl_deinit(inst);
--
2.47.0.163.g1226f6d8fa-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCHv4 3/3] media: venus: factor out inst destruction routine
2024-10-25 10:50 [PATCHv4 0/3] media: venus: close() fixes Sergey Senozhatsky
2024-10-25 10:50 ` [PATCHv4 1/3] media: venus: fix enc/dec destruction order Sergey Senozhatsky
2024-10-25 10:50 ` [PATCHv4 2/3] media: venus: sync with threaded IRQ during inst destruction Sergey Senozhatsky
@ 2024-10-25 10:50 ` Sergey Senozhatsky
2024-10-25 11:43 ` Bryan O'Donoghue
2 siblings, 1 reply; 6+ messages in thread
From: Sergey Senozhatsky @ 2024-10-25 10:50 UTC (permalink / raw)
To: Stanimir Varbanov, Vikash Garodia
Cc: Dikshita Agarwal, Bryan O'Donoghue, linux-media, linux-kernel,
Sergey Senozhatsky
Factor out common instance destruction code into
a common function.
Suggested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
drivers/media/platform/qcom/venus/core.c | 25 +++++++++++++++++++
drivers/media/platform/qcom/venus/core.h | 2 ++
drivers/media/platform/qcom/venus/vdec.c | 24 ++----------------
drivers/media/platform/qcom/venus/vdec.h | 1 -
.../media/platform/qcom/venus/vdec_ctrls.c | 6 -----
drivers/media/platform/qcom/venus/venc.c | 22 +---------------
6 files changed, 30 insertions(+), 50 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 423deb5e94dc..ee6c2051a0c4 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -19,6 +19,7 @@
#include <linux/pm_domain.h>
#include <linux/pm_runtime.h>
#include <media/videobuf2-v4l2.h>
+#include <media/v4l2-ctrls.h>
#include <media/v4l2-mem2mem.h>
#include <media/v4l2-ioctl.h>
@@ -502,6 +503,30 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev)
return ret;
}
+void venus_close_common(struct venus_inst *inst)
+{
+ /*
+ * First, remove the inst from the ->instances list, so that
+ * to_instance() will return NULL.
+ */
+ hfi_session_destroy(inst);
+ /*
+ * Second, make sure we don't have IRQ/IRQ-thread currently running
+ * or pending execution, which would race with the inst destruction.
+ */
+ synchronize_irq(inst->core->irq);
+
+ v4l2_m2m_ctx_release(inst->m2m_ctx);
+ v4l2_m2m_release(inst->m2m_dev);
+ v4l2_fh_del(&inst->fh);
+ v4l2_fh_exit(&inst->fh);
+ v4l2_ctrl_handler_free(&inst->ctrl_handler);
+
+ mutex_destroy(&inst->lock);
+ mutex_destroy(&inst->ctx_q_lock);
+}
+EXPORT_SYMBOL_GPL(venus_close_common);
+
static __maybe_unused int venus_runtime_resume(struct device *dev)
{
struct venus_core *core = dev_get_drvdata(dev);
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 435325432922..7bb36a270e15 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -560,4 +560,6 @@ is_fw_rev_or_older(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev)
(core)->venus_ver.minor == vminor &&
(core)->venus_ver.rev <= vrev);
}
+
+void venus_close_common(struct venus_inst *inst);
#endif
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index b3192a36f388..cba95dc492f1 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -1737,7 +1737,7 @@ static int vdec_open(struct file *file)
err_session_destroy:
hfi_session_destroy(inst);
err_ctrl_deinit:
- vdec_ctrl_deinit(inst);
+ v4l2_ctrl_handler_free(&inst->ctrl_handler);
err_free:
kfree(inst);
return ret;
@@ -1748,29 +1748,9 @@ static int vdec_close(struct file *file)
struct venus_inst *inst = to_inst(file);
vdec_pm_get(inst);
-
cancel_work_sync(&inst->delayed_process_work);
- /*
- * First, remove the inst from the ->instances list, so that
- * to_instance() will return NULL.
- */
- hfi_session_destroy(inst);
- /*
- * Second, make sure we don't have IRQ/IRQ-thread currently running
- * or pending execution, which would race with the inst destruction.
- */
- synchronize_irq(inst->core->irq);
-
- v4l2_m2m_ctx_release(inst->m2m_ctx);
- v4l2_m2m_release(inst->m2m_dev);
+ venus_close_common(inst);
ida_destroy(&inst->dpb_ids);
- v4l2_fh_del(&inst->fh);
- v4l2_fh_exit(&inst->fh);
- vdec_ctrl_deinit(inst);
-
- mutex_destroy(&inst->lock);
- mutex_destroy(&inst->ctx_q_lock);
-
vdec_pm_put(inst, false);
kfree(inst);
diff --git a/drivers/media/platform/qcom/venus/vdec.h b/drivers/media/platform/qcom/venus/vdec.h
index 6b262d0bf561..0cf981108ff0 100644
--- a/drivers/media/platform/qcom/venus/vdec.h
+++ b/drivers/media/platform/qcom/venus/vdec.h
@@ -9,6 +9,5 @@
struct venus_inst;
int vdec_ctrl_init(struct venus_inst *inst);
-void vdec_ctrl_deinit(struct venus_inst *inst);
#endif
diff --git a/drivers/media/platform/qcom/venus/vdec_ctrls.c b/drivers/media/platform/qcom/venus/vdec_ctrls.c
index 7e0f29bf7fae..fa034a7fdbed 100644
--- a/drivers/media/platform/qcom/venus/vdec_ctrls.c
+++ b/drivers/media/platform/qcom/venus/vdec_ctrls.c
@@ -4,7 +4,6 @@
* Copyright (C) 2017 Linaro Ltd.
*/
#include <linux/types.h>
-#include <media/v4l2-ctrls.h>
#include "core.h"
#include "helpers.h"
@@ -187,8 +186,3 @@ int vdec_ctrl_init(struct venus_inst *inst)
return 0;
}
-
-void vdec_ctrl_deinit(struct venus_inst *inst)
-{
- v4l2_ctrl_handler_free(&inst->ctrl_handler);
-}
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 36981ce448f5..2ae22ba156bb 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -1516,28 +1516,8 @@ static int venc_close(struct file *file)
struct venus_inst *inst = to_inst(file);
venc_pm_get(inst);
-
- /*
- * First, remove the inst from the ->instances list, so that
- * to_instance() will return NULL.
- */
- hfi_session_destroy(inst);
- /*
- * Second, make sure we don't have IRQ/IRQ-thread currently running
- * or pending execution, which would race with the inst destruction.
- */
- synchronize_irq(inst->core->irq);
-
- v4l2_m2m_ctx_release(inst->m2m_ctx);
- v4l2_m2m_release(inst->m2m_dev);
- v4l2_fh_del(&inst->fh);
- v4l2_fh_exit(&inst->fh);
- venc_ctrl_deinit(inst);
-
+ venus_close_common(inst);
inst->enc_state = VENUS_ENC_STATE_DEINIT;
- mutex_destroy(&inst->lock);
- mutex_destroy(&inst->ctx_q_lock);
-
venc_pm_put(inst, false);
kfree(inst);
--
2.47.0.163.g1226f6d8fa-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread