Re: [PATCH v7 05/19] media: meson: vdec: Fix race condition and synchronize esparser IRQ

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] The newly added early return path in `esparser_isr()` incorrectly returns `IRQ_HANDLED` without clearing the hardware interrupt status, leading to an interrupt storm and violating shared IRQ semantics.
- [Medium] The commit message falsely claims to fix a race condition where the ISR 'attempts to handle stream data and allocate internal tracking state structures', which does not exist in the code.

Pre-existing issues:
- [High] The synchronization and early-exit logic introduced by the patch is ineffective and leaves the system vulnerable to synchronous bus faults during teardown.
- [High] Unconditional execution of `vdec_poweroff()` in `vdec_close()` causes clock underflows.
--

--- Patch [5]: [PATCH v7 05/19] media: meson: vdec: Fix race condition and synchronize esparser IRQ ---
commit 2f7e559e32b43d068f1e5390f023fbf7f75f1262
Author: Anand Moon <[email protected]>

media: meson: vdec: Fix race condition and synchronize esparser IRQ

This commit attempts to fix a race condition during session teardown sequences
by synchronizing the esparser IRQ and adding a NULL check for the session
pointer in the ISR. It aims to prevent the interrupt handler from running
while context structures are dismantled.

Closes: https://lore.kernel.org/all/[email protected]/

> During session teardown sequences in vdec_stop_streaming() and
> vdec_close(), the 'esparser' hardware interrupt handler can still be
> actively triggered or executing on another CPU core. This creates a
> transient race condition where the ISR attempts to handle stream data and
> allocate internal tracking state structures after session contexts have
> been modified or freed.

[Severity: Medium]
Does the ISR actually allocate tracking state or handle stream data?

Looking at esparser_isr(), the sess pointer is retrieved and checked for NULL,
but the rest of the ISR does not appear to dereference this pointer or
allocate any internal tracking structures.

> diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c
> index 4632346f04a9e..37749ede308c6 100644
> --- a/drivers/staging/media/meson/vdec/esparser.c
> +++ b/drivers/staging/media/meson/vdec/esparser.c
> @@ -60,6 +60,12 @@ static irqreturn_t esparser_isr(int irq, void *dev)
>  {
>  	int int_status;
>  	struct amvdec_core *core = dev;
> +	struct amvdec_session *sess;
> +
> +	/* Secure an atomic snapshot to protect against concurrent teardown */
> +	sess = smp_load_acquire(&core->cur_sess);
> +	if (!sess)
> +		return IRQ_HANDLED;

[Severity: High]
Is it safe to return IRQ_HANDLED here without interacting with the hardware
to clear the interrupt?

Because esparser_irq is registered with IRQF_SHARED, returning IRQ_HANDLED
when cur_sess is NULL falsely tells the IRQ subsystem that this driver
handled the interrupt.

Will this cause an interrupt storm for level-triggered interrupts, or break
spurious interrupt detection for other devices sharing this IRQ line?

>  
>  	int_status = amvdec_read_parser(core, PARSER_INT_STATUS);
>  	amvdec_write_parser(core, PARSER_INT_STATUS, int_status);

[ ... ]

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> index 7ae3d5a9dd6ab..7689ffdb2e500 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
> @@ -486,6 +486,8 @@ static void vdec_stop_streaming(struct vb2_queue *q)
>  
>  		/* Synchronize and flush pending hardware interrupt service routines */
>  		synchronize_irq(core->vdec_irq);
> +		/* Ensure esparser ISR finishes executing */
> +		synchronize_irq(core->esparser_irq);
>  
>  		vdec_poweroff(sess);
>  		vdec_free_canvas(sess);

[Severity: High]
This is a pre-existing issue, but does this synchronization logic fully
close the race condition?

The synchronize_irq() call waits for in-flight ISRs, but the interrupt line
remains enabled. Later in this teardown path:

vdec_stop_streaming()
    vdec_poweroff(sess);
    ...
    smp_store_release(&core->cur_sess, NULL);

If the interrupt fires after vdec_poweroff() completes, but before cur_sess
is set to NULL, the ISR will pass the !sess check and attempt to read
PARSER_INT_STATUS from unclocked hardware.

Could this lead to a synchronous bus fault and kernel panic?

[ ... ]

> @@ -994,6 +996,8 @@ static int vdec_close(struct file *file)
>  
>  	/* Synchronize and flush pending hardware interrupt service routines */
>  	synchronize_irq(core->vdec_irq);
> +	/* Ensure esparser ISR finishes executing */
> +	synchronize_irq(core->esparser_irq);
>  
>  	if (!IS_ERR_OR_NULL(sess->recycle_thread)) {
>  		kthread_stop(sess->recycle_thread);

[Severity: High]
This is a pre-existing issue, but does vdec_close() unconditionally call
vdec_poweroff() even if the device was just opened without starting streaming?

If vdec_open() is called and immediately followed by vdec_close(), calling
vdec_poweroff() unconditionally would execute clk_disable_unprepare() on
dos_clk and dos_parser_clk which were never enabled.

Can this cause clock framework underflow warnings and potentially disable
clocks for other hardware sharing the same clock tree?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic