| Message ID | 20241024061809.400260-3-senozhatsky@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: venus: close() fixes | expand |
On 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/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
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); -}
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);
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(-)