[PATCH] mailbox/pcc.c move shmem unmap to teardown

[PATCH] mailbox/pcc.c move shmem unmap to teardown

[Adam Young]

If the mailbox IRQ and shmems are not cleaned up atomically there is a
race condition. If the shmem is torn down while the IRQ is active, a late
interrupt can trigger a write to un-mapped memory.
If the shmem is torn down after the IRQ, and another thread requests the
channel again, we can end up with a channel that has had its shmem
unmapped.

By moving the unmap to the teardown process, we can let the mailbox
mechanism prevent re-entrance into the startup/teardown functions.

Assisted-by: Codex:gpt-5.4
Fixes: fa362ffafa51 ("mailbox: pcc: Always map the shared memory communication address")
Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
---
 drivers/mailbox/pcc.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 636879ae1db7..7fa2f6b5c492 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -401,18 +401,8 @@ EXPORT_SYMBOL_GPL(pcc_mbox_request_channel);
 void pcc_mbox_free_channel(struct pcc_mbox_chan *pchan)
 {
 	struct mbox_chan *chan = pchan->mchan;
-	struct pcc_chan_info *pchan_info;
-	struct pcc_mbox_chan *pcc_mbox_chan;
-
 	if (!chan || !chan->cl)
 		return;
-	pchan_info = chan->con_priv;
-	pcc_mbox_chan = &pchan_info->chan;
-	if (pcc_mbox_chan->shmem) {
-		iounmap(pcc_mbox_chan->shmem);
-		pcc_mbox_chan->shmem = NULL;
-	}
-
 	mbox_free_channel(chan);
 }
 EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
@@ -488,15 +478,22 @@ static int pcc_startup(struct mbox_chan *chan)
 
 /**
  * pcc_shutdown - Called from Mailbox Controller code. Used here
- *		to free the interrupt.
+ *		to free the interrupt and unmap the shared memory.
  * @chan: Pointer to Mailbox channel to shutdown.
  */
 static void pcc_shutdown(struct mbox_chan *chan)
 {
 	struct pcc_chan_info *pchan = chan->con_priv;
+	struct pcc_mbox_chan *pcc_mbox_chan;
 
 	if (pchan->plat_irq > 0)
 		devm_free_irq(chan->mbox->dev, pchan->plat_irq, chan);
+
+	pcc_mbox_chan = &pchan->chan;
+	if (pcc_mbox_chan->shmem) {
+		iounmap(pcc_mbox_chan->shmem);
+		pcc_mbox_chan->shmem = NULL;
+	}
 }
 
 static const struct mbox_chan_ops pcc_chan_ops = {
-- 
2.43.0

Re: [PATCH] mailbox/pcc.c move shmem unmap to teardown

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Double Unmap / iounmap(NULL) in pcc_mbox_request_channel error path.
- [High] TOCTOU Race Condition in pcc_mbox_request_channel leading to panic.
--

commit b19a3a11311850c88f208492b1f19cd0503eefd7
Author: Adam Young <admiyo@os.amperecomputing.com>

mailbox/pcc.c move shmem unmap to teardown

This commit moves the shared memory unmapping from pcc_mbox_free_channel()
to the pcc_shutdown() callback. This is intended to fix a race condition
where late interrupts could write to unmapped memory if the channel was
torn down while the IRQ was active.

> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 636879ae1db76..7fa2f6b5c492a 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
[ ... ]
> @@ -488,15 +478,22 @@ static int pcc_startup(struct mbox_chan *chan)
>  
>  /**
>   * pcc_shutdown - Called from Mailbox Controller code. Used here
> - *		to free the interrupt.
> + *		to free the interrupt and unmap the shared memory.
>   * @chan: Pointer to Mailbox channel to shutdown.
>   */
>  static void pcc_shutdown(struct mbox_chan *chan)
>  {
>  	struct pcc_chan_info *pchan = chan->con_priv;
> +	struct pcc_mbox_chan *pcc_mbox_chan;
>  
>  	if (pchan->plat_irq > 0)
>  		devm_free_irq(chan->mbox->dev, pchan->plat_irq, chan);
> +
> +	pcc_mbox_chan = &pchan->chan;
> +	if (pcc_mbox_chan->shmem) {
> +		iounmap(pcc_mbox_chan->shmem);
> +		pcc_mbox_chan->shmem = NULL;
> +	}
>  }

Will this cause a regression with a double unmap and an iounmap(NULL) call
during the error path of pcc_mbox_request_channel()?

If mbox_bind_client() fails during its startup phase, the mailbox core
automatically unwinds by calling mbox_free_channel(), which invokes this
updated pcc_shutdown() callback. 

When control returns to pcc_mbox_request_channel(), it sees the failure
and executes its own error path:

drivers/mailbox/pcc.c:pcc_mbox_request_channel() {
...
	rc = mbox_bind_client(chan, cl);
	if (rc) {
		iounmap(pcc_mchan->shmem);
		pcc_mchan->shmem = NULL;
		return ERR_PTR(rc);
	}
...
}

Since the pointer was already set to NULL in pcc_shutdown(), won't this
unconditionally execute iounmap(NULL)? This violates kernel API contracts
and can trigger a stack trace on architectures like x86, or potentially
cause a kernel oops. Should an explicit check for pcc_mchan->shmem be added
to the error path?

Additionally, there appears to be a pre-existing regression in
pcc_mbox_request_channel() that wasn't introduced by this patch.

If two threads request the same subspace simultaneously, can both observe
chan->cl == NULL without locks and proceed to map the shared memory?

drivers/mailbox/pcc.c:pcc_mbox_request_channel() {
...
	if (IS_ERR(chan) || chan->cl) {
		pr_err("Channel not found for idx: %d\n", subspace_id);
		return ERR_PTR(-EBUSY);
	}

	pcc_mchan = &pchan->chan;
	pcc_mchan->shmem = acpi_os_ioremap(pcc_mchan->shmem_base_addr,
					   pcc_mchan->shmem_size);
...

Both threads blindly overwrite the shared pcc_mchan->shmem pointer.
When both threads call mbox_bind_client(), one thread will successfully
bind and begin using the channel, while the second thread will fail
with -EBUSY.

The second thread will then enter its error path:

	rc = mbox_bind_client(chan, cl);
	if (rc) {
		iounmap(pcc_mchan->shmem);
		pcc_mchan->shmem = NULL;
		return ERR_PTR(rc);
	}

Doesn't this unmap the active memory mapping for the winning thread?
The next time the winning thread interacts with the hardware, won't it
dereference the NULL pointer or access unmapped memory, resulting in a
kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515161001.699470-1-admiyo@os.amperecomputing.com?part=1

Re: [PATCH] mailbox/pcc.c move shmem unmap to teardown

[Adam Young]

On 5/15/26 12:49, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Double Unmap / iounmap(NULL) in pcc_mbox_request_channel error path.
> - [High] TOCTOU Race Condition in pcc_mbox_request_channel leading to panic.
> --
>
> commit b19a3a11311850c88f208492b1f19cd0503eefd7
> Author: Adam Young <admiyo@os.amperecomputing.com>
>
> mailbox/pcc.c move shmem unmap to teardown
>
> This commit moves the shared memory unmapping from pcc_mbox_free_channel()
> to the pcc_shutdown() callback. This is intended to fix a race condition
> where late interrupts could write to unmapped memory if the channel was
> torn down while the IRQ was active.
>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index 636879ae1db76..7fa2f6b5c492a 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
> [ ... ]
>> @@ -488,15 +478,22 @@ static int pcc_startup(struct mbox_chan *chan)
>>   
>>   /**
>>    * pcc_shutdown - Called from Mailbox Controller code. Used here
>> - *		to free the interrupt.
>> + *		to free the interrupt and unmap the shared memory.
>>    * @chan: Pointer to Mailbox channel to shutdown.
>>    */
>>   static void pcc_shutdown(struct mbox_chan *chan)
>>   {
>>   	struct pcc_chan_info *pchan = chan->con_priv;
>> +	struct pcc_mbox_chan *pcc_mbox_chan;
>>   
>>   	if (pchan->plat_irq > 0)
>>   		devm_free_irq(chan->mbox->dev, pchan->plat_irq, chan);
>> +
>> +	pcc_mbox_chan = &pchan->chan;
>> +	if (pcc_mbox_chan->shmem) {
>> +		iounmap(pcc_mbox_chan->shmem);
>> +		pcc_mbox_chan->shmem = NULL;
>> +	}
>>   }
> Will this cause a regression with a double unmap and an iounmap(NULL) call
> during the error path of pcc_mbox_request_channel()?
>
> If mbox_bind_client() fails during its startup phase, the mailbox core
> automatically unwinds by calling mbox_free_channel(), which invokes this
> updated pcc_shutdown() callback.
>
> When control returns to pcc_mbox_request_channel(), it sees the failure
> and executes its own error path:
>
> drivers/mailbox/pcc.c:pcc_mbox_request_channel() {
> ...
> 	rc = mbox_bind_client(chan, cl);
> 	if (rc) {
> 		iounmap(pcc_mchan->shmem);

Yes this call needs to be removed.


> 		pcc_mchan->shmem = NULL;
> 		return ERR_PTR(rc);
> 	}
> ...
> }
>
> Since the pointer was already set to NULL in pcc_shutdown(), won't this
> unconditionally execute iounmap(NULL)? This violates kernel API contracts
> and can trigger a stack trace on architectures like x86, or potentially
> cause a kernel oops. Should an explicit check for pcc_mchan->shmem be added
> to the error path?
>
> Additionally, there appears to be a pre-existing regression in
> pcc_mbox_request_channel() that wasn't introduced by this patch.
>
> If two threads request the same subspace simultaneously, can both observe
> chan->cl == NULL without locks and proceed to map the shared memory?
No they can't.  Locking needs to be introduced but is beyond the scope 
of this patch.
>
> drivers/mailbox/pcc.c:pcc_mbox_request_channel() {
> ...
> 	if (IS_ERR(chan) || chan->cl) {
> 		pr_err("Channel not found for idx: %d\n", subspace_id);
> 		return ERR_PTR(-EBUSY);
> 	}
>
> 	pcc_mchan = &pchan->chan;
> 	pcc_mchan->shmem = acpi_os_ioremap(pcc_mchan->shmem_base_addr,
> 					   pcc_mchan->shmem_size);
> ...
>
> Both threads blindly overwrite the shared pcc_mchan->shmem pointer.
> When both threads call mbox_bind_client(), one thread will successfully
> bind and begin using the channel, while the second thread will fail
> with -EBUSY.
>
> The second thread will then enter its error path:
>
> 	rc = mbox_bind_client(chan, cl);
> 	if (rc) {
> 		iounmap(pcc_mchan->shmem);
> 		pcc_mchan->shmem = NULL;
> 		return ERR_PTR(rc);
> 	}
>
> Doesn't this unmap the active memory mapping for the winning thread?
> The next time the winning thread interacts with the hardware, won't it
> dereference the NULL pointer or access unmapped memory, resulting in a
> kernel panic?
>