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

[sashiko-bot@kernel.org]

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