Re: [PATCH 1/2] mailbox: qcom-cpucp: fix PREEMPT_RT self-deadlock in IRQ handler

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Dropping `chan->lock` before `mbox_chan_received_data()` introduces a TOCTOU race with `mbox_free_channel()`, leading to a NULL pointer dereference.
- [Medium] Acknowledging the hardware RX interrupt before invoking the RX callback violates the mailbox framework API and can cause client re-entrancy.
--

--- Patch [1]: [PATCH 1/2] mailbox: qcom-cpucp: fix PREEMPT_RT self-deadlock in IRQ handler ---
commit 995d8aa11aed719aa1d45f140bab12a284779e97
Author: Jia Yang <jia.yang@xxxxxxxxxxxxxxxx>

mailbox: qcom-cpucp: fix PREEMPT_RT self-deadlock in IRQ handler

This patch addresses a PREEMPT_RT self-deadlock in the qcom-cpucp mailbox IRQ
handler. It prevents lock re-entrancy by caching the channel client, clearing
the hardware interrupt inside the lock, and invoking the receive callback
outside the lock.

> diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
> index 862e45e8fbd58..0f7fe189e8b1f 100644
> --- a/drivers/mailbox/qcom-cpucp-mbox.c
> +++ b/drivers/mailbox/qcom-cpucp-mbox.c
> @@ -63,14 +63,25 @@ static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
[ ... ]
>  		/*
>  		 * Provide mutual exclusion with changes to chan->cl.
>  		 * Save cl locally and clear the HW interrupt inside the lock,
>  		 * then invoke mbox_chan_received_data() outside the lock to
>  		 * avoid a PREEMPT_RT self-deadlock: mbox_chan_received_data()
>  		 * can call back into mbox_send_message() via scmi_rx_callback()
>  		 * -> mailbox_clear_channel(), which re-acquires chan->lock
>  		 * (converted to an rt_spinlock under PREEMPT_RT).
>  		 */
>  		spin_lock_irqsave(&chan->lock, flags);
> -		if (chan->cl)
> -			mbox_chan_received_data(chan, &val);
> +		cl = chan->cl;
>  		writeq(BIT(i), cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);

[Severity: Medium]
Does clearing the hardware interrupt before invoking the RX callback violate
the mailbox framework API expectations?

If the remote processor immediately sends another message upon seeing this
ACK, could a new interrupt fire and be processed on another CPU while the
lock is dropped, leading to concurrent entries into the client's rx_callback
for the same channel?

>  		spin_unlock_irqrestore(&chan->lock, flags);
> +
> +		if (cl)
> +			mbox_chan_received_data(chan, &val);

[Severity: High]
If mbox_free_channel() runs concurrently and sets chan->cl = NULL immediately
after spin_unlock_irqrestore(), won't mbox_chan_received_data() cause a NULL
pointer dereference?

Even though we check the local cached "cl" variable here, the framework API
unconditionally dereferences chan->cl without locking:

drivers/mailbox/mailbox.c:mbox_chan_received_data() {
	/* No buffering the received data */
	if (chan->cl->rx_callback)
		chan->cl->rx_callback(chan->cl, mssg);
}

Could this race be reliably triggered during channel teardown or client
driver unbind?

>  	}
>  
>  	return IRQ_HANDLED;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806-mailbox-qcom-cpucp-preempt-rt-fix-v1-0-d302a63dfb5e@xxxxxxxxxxxxxxxx?part=1