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
Re-arragne inst destruction. First remove the inst from the
core ->instacnes list, second synchronize IRQ/IRQ-thread to
make sure that nothing else would see the inst while we take
it down.
Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.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-googOn 24/10/2024 07:16, Sergey Senozhatsky wrote: > 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 > > Re-arragne inst destruction. First remove the inst from the > core ->instacnes list, second synchronize IRQ/IRQ-thread to > make sure that nothing else would see the inst while we take > it down. > > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.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); This needs a Fixes: tag too. It also occurs to me that most of the close() operation code is shared between venc_close() and vdec_close() a welcome patch for V3 would be to functionally decompose the common code to a shared location. So that would be your two Fixes: first and then a reduction patch for a total of three. Anyway with the Fixes applied Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- bod
On (24/10/24 09:59), Bryan O'Donoghue wrote: > This needs a Fixes: tag too. Ack. > It also occurs to me that most of the close() operation code is shared > between venc_close() and vdec_close() a welcome patch for V3 would be to > functionally decompose the common code to a shared location. Any preferences where that "shared location" should be?
On 24/10/2024 10:39, Sergey Senozhatsky wrote: > On (24/10/24 09:59), Bryan O'Donoghue wrote: >> This needs a Fixes: tag too. > > Ack. > >> It also occurs to me that most of the close() operation code is shared >> between venc_close() and vdec_close() a welcome patch for V3 would be to >> functionally decompose the common code to a shared location. > > Any preferences where that "shared location" should be? Probably core.c is the only place we can jam stuff to be shared but, if you think of something more appropriate feel free. --- bod
On (24/10/24 10:43), Bryan O'Donoghue wrote:
> > > It also occurs to me that most of the close() operation code is shared
> > > between venc_close() and vdec_close() a welcome patch for V3 would be to
> > > functionally decompose the common code to a shared location.
> >
> > Any preferences where that "shared location" should be?
>
> Probably core.c is the only place we can jam stuff to be shared
Ack.
So, we need to
- export a couple of symbols
- include vdec header in core
Does something like this look OK to you?
---
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 31ea0982f58e..e7c69a292b3c 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -500,6 +500,31 @@ 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);
+ 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);
+}
+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 3748a9a74dce..75e9b29d1b01 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -17,6 +17,7 @@
#include "hfi.h"
#include "hfi_platform.h"
#include "hfi_helper.h"
+#include "vdec.h"
#define VDBGL "VenusLow : "
#define VDBGM "VenusMed : "
@@ -569,4 +570,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 d802ece8948f..b7d4801222cb 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -1746,29 +1746,8 @@ 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);
- 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);
-
+ venus_close_common(inst);
vdec_pm_put(inst, false);
kfree(inst);
diff --git a/drivers/media/platform/qcom/venus/vdec_ctrls.c b/drivers/media/platform/qcom/venus/vdec_ctrls.c
index 7e0f29bf7fae..2b6b2eee619c 100644
--- a/drivers/media/platform/qcom/venus/vdec_ctrls.c
+++ b/drivers/media/platform/qcom/venus/vdec_ctrls.c
@@ -192,3 +192,4 @@ void vdec_ctrl_deinit(struct venus_inst *inst)
{
v4l2_ctrl_handler_free(&inst->ctrl_handler);
}
+EXPORT_SYMBOL_GPL(vdec_ctrl_deinit);
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index a72681de1179..55108117d085 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -1537,28 +1537,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);
On (24/10/24 19:08), Sergey Senozhatsky wrote:
> So, we need to
> - export a couple of symbols
> - include vdec header in core
[..]
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -500,6 +500,31 @@ 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);
> + 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);
> +}
> +EXPORT_SYMBOL_GPL(venus_close_common);
[..]
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -17,6 +17,7 @@
> #include "hfi.h"
> #include "hfi_platform.h"
> #include "hfi_helper.h"
> +#include "vdec.h"
>
> #define VDBGL "VenusLow : "
> #define VDBGM "VenusMed : "
> @@ -569,4 +570,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
[..]
> +++ b/drivers/media/platform/qcom/venus/vdec_ctrls.c
> @@ -192,3 +192,4 @@ void vdec_ctrl_deinit(struct venus_inst *inst)
> {
> v4l2_ctrl_handler_free(&inst->ctrl_handler);
> }
> +EXPORT_SYMBOL_GPL(vdec_ctrl_deinit);
Would have been entirely too simple had I compile-tested
my patches, wouldn't it?
depmod: ERROR: Cycle detected: venus_core -> venus_dec -> venus_core
vdec_ctrl_deinit() is a v4l2_ctrl_handler_free() wrapper, a one-liner,
so I turned into a static inline.
Compiles with the addition of:
---
diff --git a/drivers/media/platform/qcom/venus/vdec.h b/drivers/media/platform/qcom/venus/vdec.h
index 6b262d0bf561..2687255b1616 100644
--- a/drivers/media/platform/qcom/venus/vdec.h
+++ b/drivers/media/platform/qcom/venus/vdec.h
@@ -6,9 +6,14 @@
#ifndef __VENUS_VDEC_H__
#define __VENUS_VDEC_H__
+#include <media/v4l2-ctrls.h>
+
struct venus_inst;
int vdec_ctrl_init(struct venus_inst *inst);
-void vdec_ctrl_deinit(struct venus_inst *inst);
+static inline void vdec_ctrl_deinit(struct venus_inst *inst)
+{
+ v4l2_ctrl_handler_free(&inst->ctrl_handler);
+}
#endif
diff --git a/drivers/media/platform/qcom/venus/vdec_ctrls.c b/drivers/media/platform/qcom/venus/vdec_ctrls.c
index 2b6b2eee619c..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,9 +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);
-}
On 24/10/2024 11:08, Sergey Senozhatsky wrote: > On (24/10/24 10:43), Bryan O'Donoghue wrote: >>>> It also occurs to me that most of the close() operation code is shared >>>> between venc_close() and vdec_close() a welcome patch for V3 would be to >>>> functionally decompose the common code to a shared location. >>> >>> Any preferences where that "shared location" should be? >> >> Probably core.c is the only place we can jam stuff to be shared > > Ack. > > So, we need to > - export a couple of symbols > - include vdec header in core > > Does something like this look OK to you? y lgtm --- bod
© 2016 - 2024 Red Hat, Inc.