[PATCH 0/2] media: venus: close() fixes

[PATCH 0/2] media: venus: close() fixes

[Sergey Senozhatsky]

A couple of venus fixes for close() handling (both enc and dec).

Sergey Senozhatsky (2):
  media: venus: fix enc/dec destruction order
  media: venus: sync with threaded IRQ during inst destruction

 drivers/media/platform/qcom/venus/vdec.c | 20 +++++++++++++++++---
 drivers/media/platform/qcom/venus/venc.c | 19 ++++++++++++++++---
 2 files changed, 33 insertions(+), 6 deletions(-)

-- 
2.47.0.105.g07ac214952-goog

[PATCH 1/2] media: venus: fix enc/dec destruction order

[Sergey Senozhatsky]

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().

Suggested-by: Tomasz Figa <tfiga@google.com>
Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.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.105.g07ac214952-goog

[PATCH 2/2] media: venus: sync with threaded IRQ during inst destruction

[Sergey Senozhatsky]

When destroy 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

Guard inst destruction (both dec and enc) with hard and threaded
IRQ synchronization.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 drivers/media/platform/qcom/venus/vdec.c | 13 +++++++++++++
 drivers/media/platform/qcom/venus/venc.c | 13 +++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 0013c4704f03..ff1823bc967c 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -1747,6 +1747,13 @@ static int vdec_close(struct file *file)
 {
 	struct venus_inst *inst = to_inst(file);
 
+	/*
+	 * Make sure we don't have pending IRQs and no threaded IRQ handler is
+	 * running nor pending (synchronize_irq)), which can race with inst
+	 * destruction.
+	 */
+	disable_irq(inst->core->irq);
+
 	vdec_pm_get(inst);
 
 	cancel_work_sync(&inst->delayed_process_work);
@@ -1763,6 +1770,12 @@ static int vdec_close(struct file *file)
 
 	vdec_pm_put(inst, false);
 
+	/*
+	 * inst is gone from the core->instances list, re-enable IRQ and
+	 * threaded IRQ
+	 */
+	enable_irq(inst->core->irq);
+
 	kfree(inst);
 	return 0;
 }
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 6a26a6592424..6575e84312fe 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -1515,6 +1515,13 @@ static int venc_close(struct file *file)
 {
 	struct venus_inst *inst = to_inst(file);
 
+	/*
+	 * Make sure we don't have pending IRQs and no threaded IRQ handler is
+	 * running nor pending (synchronize_irq)), which can race with inst
+	 * destruction.
+	 */
+	disable_irq(inst->core->irq);
+
 	venc_pm_get(inst);
 
 	v4l2_m2m_ctx_release(inst->m2m_ctx);
@@ -1530,6 +1537,12 @@ static int venc_close(struct file *file)
 
 	venc_pm_put(inst, false);
 
+	/*
+	 * inst is gone from the core->instances list, re-enable IRQ and
+	 * threaded IRQ
+	 */
+	enable_irq(inst->core->irq);
+
 	kfree(inst);
 	return 0;
 }
-- 
2.47.0.105.g07ac214952-goog

Re: [PATCH 2/2] media: venus: sync with threaded IRQ during inst destruction

[Sergey Senozhatsky]

On (24/10/23 14:24), Sergey Senozhatsky wrote:
> Guard inst destruction (both dec and enc) with hard and threaded
> IRQ synchronization.

Folks, please ignore this patch.   Stand by for v2.

Re: [PATCH 2/2] media: venus: sync with threaded IRQ during inst destruction

[Sergey Senozhatsky]

On (24/10/24 13:58), Sergey Senozhatsky wrote:
> Date: Thu, 24 Oct 2024 13:58:36 +0900
> From: Sergey Senozhatsky <senozhatsky@chromium.org>
> To: Sergey Senozhatsky <senozhatsky@chromium.org>
> Cc: Stanimir Varbanov <stanimir.k.varbanov@gmail.com>, Vikash Garodia
>  <quic_vgarodia@quicinc.com>, Bryan O'Donoghue
>  <bryan.odonoghue@linaro.org>, linux-media@vger.kernel.org,
>  linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] media: venus: sync with threaded IRQ during inst
>  destruction
> Message-ID: <20241024045836.GJ1279924@google.com>
> 
> On (24/10/23 14:24), Sergey Senozhatsky wrote:
> > Guard inst destruction (both dec and enc) with hard and threaded
> > IRQ synchronization.
> 
> Folks, please ignore this patch.   Stand by for v2.

I think it probably should be something like this (both for dec and
enc).

---

@@ -1538,9 +1538,25 @@ 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 (disable_irq() calls synchronize_irq()), which
+        * can race with the inst destruction.
+        */
+       disable_irq(inst->core->irq);
+       /*
+        * Lastly, inst is gone from the core->instances list and we don't
+        * have running/pending IRQ/IRQ-thread, proceed with the destruction
+        */
+       enable_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);

Re: [PATCH 2/2] media: venus: sync with threaded IRQ during inst destruction

[Tomasz Figa]

Hi Sergey,

On Thu, Oct 24, 2024 at 2:13 PM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (24/10/24 13:58), Sergey Senozhatsky wrote:
> > Date: Thu, 24 Oct 2024 13:58:36 +0900
> > From: Sergey Senozhatsky <senozhatsky@chromium.org>
> > To: Sergey Senozhatsky <senozhatsky@chromium.org>
> > Cc: Stanimir Varbanov <stanimir.k.varbanov@gmail.com>, Vikash Garodia
> >  <quic_vgarodia@quicinc.com>, Bryan O'Donoghue
> >  <bryan.odonoghue@linaro.org>, linux-media@vger.kernel.org,
> >  linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 2/2] media: venus: sync with threaded IRQ during inst
> >  destruction
> > Message-ID: <20241024045836.GJ1279924@google.com>
> >
> > On (24/10/23 14:24), Sergey Senozhatsky wrote:
> > > Guard inst destruction (both dec and enc) with hard and threaded
> > > IRQ synchronization.
> >
> > Folks, please ignore this patch.   Stand by for v2.
>
> I think it probably should be something like this (both for dec and
> enc).
>
> ---
>
> @@ -1538,9 +1538,25 @@ 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 (disable_irq() calls synchronize_irq()), which
> +        * can race with the inst destruction.
> +        */
> +       disable_irq(inst->core->irq);
> +       /*
> +        * Lastly, inst is gone from the core->instances list and we don't
> +        * have running/pending IRQ/IRQ-thread, proceed with the destruction
> +        */
> +       enable_irq(inst->core->irq);
> +

Thanks a lot for looking into this. Wouldn't it be enough to just call
synchronize_irq() at this point, since the instance was removed from
the list already? I guess the question is if that's the only way the
interrupt handler can get hold of the instance.

Best,
Tomasz

>         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);
>

Re: [PATCH 2/2] media: venus: sync with threaded IRQ during inst destruction

[Sergey Senozhatsky]

On (24/10/24 14:18), Tomasz Figa wrote:
> > @@ -1538,9 +1538,25 @@ 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 (disable_irq() calls synchronize_irq()), which
> > +        * can race with the inst destruction.
> > +        */
> > +       disable_irq(inst->core->irq);
> > +       /*
> > +        * Lastly, inst is gone from the core->instances list and we don't
> > +        * have running/pending IRQ/IRQ-thread, proceed with the destruction
> > +        */
> > +       enable_irq(inst->core->irq);
> > +
> 
> Thanks a lot for looking into this. Wouldn't it be enough to just call
> synchronize_irq() at this point, since the instance was removed from
> the list already? I guess the question is if that's the only way the
> interrupt handler can get hold of the instance.

Good question.

synchronize_irq() waits for IRQ-threads, so if inst is accessed only from
IRQ-thread then we are fine.  If, however, inst is also accessed from hard
IRQ, then synchronize_irq() won't work, I guess, because it doesn't wait
for "in flight hard IRQs".  disable_irq() OTOH "waits for completion", so
we cover in-flight hard IRQs too.

Re: [PATCH 2/2] media: venus: sync with threaded IRQ during inst destruction

[Tomasz Figa]

On Thu, Oct 24, 2024 at 2:46 PM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (24/10/24 14:18), Tomasz Figa wrote:
> > > @@ -1538,9 +1538,25 @@ 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 (disable_irq() calls synchronize_irq()), which
> > > +        * can race with the inst destruction.
> > > +        */
> > > +       disable_irq(inst->core->irq);
> > > +       /*
> > > +        * Lastly, inst is gone from the core->instances list and we don't
> > > +        * have running/pending IRQ/IRQ-thread, proceed with the destruction
> > > +        */
> > > +       enable_irq(inst->core->irq);
> > > +
> >
> > Thanks a lot for looking into this. Wouldn't it be enough to just call
> > synchronize_irq() at this point, since the instance was removed from
> > the list already? I guess the question is if that's the only way the
> > interrupt handler can get hold of the instance.
>
> Good question.
>
> synchronize_irq() waits for IRQ-threads, so if inst is accessed only from
> IRQ-thread then we are fine.  If, however, inst is also accessed from hard
> IRQ, then synchronize_irq() won't work, I guess, because it doesn't wait
> for "in flight hard IRQs".  disable_irq() OTOH "waits for completion", so
> we cover in-flight hard IRQs too.

Looking at the code, synchronize_irq() internally also calls
synchronize_hardirq() and that in turn waits for the
IRQD_IRQ_INPROGESS flag to be cleared before returning [1]. The flag
is set by handle_irq_event() before most of the IRQ handling is run
and cleared at the end of the function [2], which makes me believe
that it would actually ensure all the hardirq and threaded IRQ
handlers would be waited for.

[1] https://elixir.bootlin.com/linux/v6.11.5/source/kernel/irq/manage.c#L38
[2] https://elixir.bootlin.com/linux/v6.11.5/source/kernel/irq/handle.c#L202

Although I guess it would be the best if someone confirmed that,
because with all the IRQ handling complexities of SMP, nothing can be
certain today. :P

Best,
Tomasz