mailbox: pcc: Free the channel before unmapping the shared memory

[PATCH] mailbox: pcc: Free the channel before unmapping the shared memory

Posted by Breno Leitao 1 month, 2 weeks ago

I am seeing a crash on PCC that is related to a an shared memory being
unmapped before the IRQ is disabled, and the IRQ kicks in and hits the
unmapped (NULL) address. This is a summary of what I see on my box:

	scmi_protocol scmi_dev.1: Message for 1 type 0 is not expected!
	Unable to handle kernel NULL pointer dereference at virtual address 0000000000000004
	 __handle_irq_event_percpu+0x1c4/0x9e0
	 handle_irq_event+0x98/0x218
	 handle_fasteoi_irq+0x230/0x750
	 generic_handle_domain_irq+0xac/0x138
	 gic_handle_irq+0x344/0x740
	 call_on_irq_stack+0x30/0x48

The trapping store is iowrite32(SCMI_SHMEM_FLAG_INTR_ENABLED,
&shmem->header.flags), a write of 1 at offset 4 of a NULL base.

But, back to the problem, pcc_mbox_free_channel() unmaps the shared
memory and clears pchan->chan.shmem *before* freeing the IRQ (aka
calling mbox_free_channel()).

The interrupt is still live when the mapping goes away.

Free the channel first, before the memory unmap. mbox_free_channel()
calls pcc_shutdown(), which frees the platform interrupt, and then unmap
shared memory.

Fixes: 7f9e19f207be ("mailbox: pcc: Check before sending MCTP PCC response ACK")
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/mailbox/pcc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 636879ae1db76..d32f170141de7 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -408,12 +408,13 @@ void pcc_mbox_free_channel(struct pcc_mbox_chan *pchan)
 		return;
 	pchan_info = chan->con_priv;
 	pcc_mbox_chan = &pchan_info->chan;
+
+	mbox_free_channel(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);
 

---
base-commit: 5e6de6a2b522f659defacb1551d0465ba6ce13cf
change-id: 20260812-pcc-3a7413982a5e

Best regards,
--  
Breno Leitao <leitao@debian.org>

Re: [PATCH] mailbox: pcc: Free the channel before unmapping the shared memory

Posted by Adam Young 1 month, 2 weeks ago

On 8/12/26 08:36, Breno Leitao wrote:
> I am seeing a crash on PCC that is related to a an shared memory being
> unmapped before the IRQ is disabled, and the IRQ kicks in and hits the
> unmapped (NULL) address. This is a summary of what I see on my box:
>
> 	scmi_protocol scmi_dev.1: Message for 1 type 0 is not expected!
> 	Unable to handle kernel NULL pointer dereference at virtual address 0000000000000004
> 	 __handle_irq_event_percpu+0x1c4/0x9e0
> 	 handle_irq_event+0x98/0x218
> 	 handle_fasteoi_irq+0x230/0x750
> 	 generic_handle_domain_irq+0xac/0x138
> 	 gic_handle_irq+0x344/0x740
> 	 call_on_irq_stack+0x30/0x48
>
> The trapping store is iowrite32(SCMI_SHMEM_FLAG_INTR_ENABLED,
> &shmem->header.flags), a write of 1 at offset 4 of a NULL base.
>
> But, back to the problem, pcc_mbox_free_channel() unmaps the shared
> memory and clears pchan->chan.shmem *before* freeing the IRQ (aka
> calling mbox_free_channel()).
>
> The interrupt is still live when the mapping goes away.
>
> Free the channel first, before the memory unmap. mbox_free_channel()
> calls pcc_shutdown(), which frees the platform interrupt, and then unmap
> shared memory.

I posted a related fix undere the MCTP PCC Driver changes.

This fix is necessary but not sufficient to deal with the race 
conditions.  Take a look at the series of patches under here:

https://lore.kernel.org/all/20260721175258.87600-1-admiyo@os.amperecomputing.com/




>
> Fixes: 7f9e19f207be ("mailbox: pcc: Check before sending MCTP PCC response ACK")
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>   drivers/mailbox/pcc.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 636879ae1db76..d32f170141de7 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -408,12 +408,13 @@ void pcc_mbox_free_channel(struct pcc_mbox_chan *pchan)
>   		return;
>   	pchan_info = chan->con_priv;
>   	pcc_mbox_chan = &pchan_info->chan;
> +
> +	mbox_free_channel(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);
>   
>
> ---
> base-commit: 5e6de6a2b522f659defacb1551d0465ba6ce13cf
> change-id: 20260812-pcc-3a7413982a5e
>
> Best regards,
> --
> Breno Leitao <leitao@debian.org>
>

Re: [PATCH] mailbox: pcc: Free the channel before unmapping the shared memory

Posted by Breno Leitao 1 month, 1 week ago

Hello Adam,

On Sat, Aug 15, 2026 at 11:58:16PM -0400, Adam Young wrote:
> On 8/12/26 08:36, Breno Leitao wrote:
> > Free the channel first, before the memory unmap. mbox_free_channel()
> > calls pcc_shutdown(), which frees the platform interrupt, and then unmap
> > shared memory.
>
> I posted a related fix undere the MCTP PCC Driver changes.
>
> This fix is necessary but not sufficient to deal with the race conditions.
> Take a look at the series of patches under here:
>
> https://lore.kernel.org/all/20260721175258.87600-1-admiyo@os.amperecomputing.com/

That looks like a much bigger series, with new features, open discussions,
Nacks, and already 45 revisions.

This curent fix is only meant to address a real problem (user after
free) I am hitting in production systems.

My suggestion is that we get this real issue unblocked, even if not
complete, while you keep working on your larger series.

Thanks,
--breno

Re: [PATCH] mailbox: pcc: Free the channel before unmapping the shared memory

Posted by Adam Young 1 month, 1 week ago

On 8/17/26 05:42, Breno Leitao wrote:
> Hello Adam,
>
> On Sat, Aug 15, 2026 at 11:58:16PM -0400, Adam Young wrote:
>> On 8/12/26 08:36, Breno Leitao wrote:
>>> Free the channel first, before the memory unmap. mbox_free_channel()
>>> calls pcc_shutdown(), which frees the platform interrupt, and then unmap
>>> shared memory.
>> I posted a related fix undere the MCTP PCC Driver changes.
>>
>> This fix is necessary but not sufficient to deal with the race conditions.
>> Take a look at the series of patches under here:
>>
>> https://lore.kernel.org/all/20260721175258.87600-1-admiyo@os.amperecomputing.com/
> That looks like a much bigger series, with new features, open discussions,
> Nacks, and already 45 revisions.
>
> This curent fix is only meant to address a real problem (user after
> free) I am hitting in production systems.
>
> My suggestion is that we get this real issue unblocked, even if not
> complete, while you keep working on your larger series.
>
> Thanks,
> --breno

There are two patches that address this issue from that series:

This one is, I believe, a superset of the fix you put in.

https://lore.kernel.org/all/20260721175258.87600-2-admiyo@os.amperecomputing.com/

Note that the unmap is moved to the mailbox teardown function, which 
happens immediately after the IRQ unregister.  Those two operatations 
need to happen atomically to avoid the issue you saw. Thus, I think your 
patch is insufficient to actually deal with the race condition:  it 
needs to happen in pcc_shutdown.

Re: [PATCH] mailbox: pcc: Free the channel before unmapping the shared memory

Posted by Adam Young 1 month, 1 week ago

On 8/17/26 05:42, Breno Leitao wrote:
> Hello Adam,
>
> On Sat, Aug 15, 2026 at 11:58:16PM -0400, Adam Young wrote:
>> On 8/12/26 08:36, Breno Leitao wrote:
>>> Free the channel first, before the memory unmap. mbox_free_channel()
>>> calls pcc_shutdown(), which frees the platform interrupt, and then unmap
>>> shared memory.
>> I posted a related fix undere the MCTP PCC Driver changes.
>>
>> This fix is necessary but not sufficient to deal with the race conditions.
>> Take a look at the series of patches under here:
>>
>> https://lore.kernel.org/all/20260721175258.87600-1-admiyo@os.amperecomputing.com/
> That looks like a much bigger series, with new features, open discussions,
> Nacks, and already 45 revisions.
>
> This curent fix is only meant to address a real problem (user after
> free) I am hitting in production systems.
>
> My suggestion is that we get this real issue unblocked, even if not
> complete, while you keep working on your larger series.
>
> Thanks,
> --breno

Yes.  In order to get MCTP over PCC working, and get through both Human 
and now AI code review, the series has grown again.

The Race condition you see is solved by one of the patches in the 
series.  However, it moves the start up out of the PCC specific function 
and into the Mailbox start up and teardown.  You cannot deal with the 
race conditions without doing that.

I have been chasing PCC issues for a couple years now.  The Type 2 
drivers do not typically have the issues that the extended drivers have, 
as they typically deal with a minimal amount of memory, usually just a 
single atomic word that can be coupled with the doorbell.  Extended 
memory is much more likely to trigger hidden rac conditions as the 
amount of work done per doorbell-ring or message-send is non-atomic.  
THe cases include: a doorbell coming during start up or during tear down.

I am happy to resubmit any of the changes from that series in a differnt 
name/order to get them reviewed and merged.  However, previous patches I 
submited have gotten minimal review.  I put them all in one series as I 
need the Network reviewers to be able to sign off on the end-to-end of 
the MCTP over PCC patch.

So, I am very flexible as far as what to do with the patches, but lets 
not put in half-fixes that make that process harder.

Re: [PATCH] mailbox: pcc: Free the channel before unmapping the shared memory

Posted by Sudeep Holla 1 month, 2 weeks ago

On Wed, Aug 12, 2026 at 05:36:03AM -0700, Breno Leitao wrote:
> I am seeing a crash on PCC that is related to a an shared memory being
> unmapped before the IRQ is disabled, and the IRQ kicks in and hits the
> unmapped (NULL) address. This is a summary of what I see on my box:
> 
> 	scmi_protocol scmi_dev.1: Message for 1 type 0 is not expected!
> 	Unable to handle kernel NULL pointer dereference at virtual address 0000000000000004
> 	 __handle_irq_event_percpu+0x1c4/0x9e0
> 	 handle_irq_event+0x98/0x218
> 	 handle_fasteoi_irq+0x230/0x750
> 	 generic_handle_domain_irq+0xac/0x138
> 	 gic_handle_irq+0x344/0x740
> 	 call_on_irq_stack+0x30/0x48
> 
> The trapping store is iowrite32(SCMI_SHMEM_FLAG_INTR_ENABLED,
> &shmem->header.flags), a write of 1 at offset 4 of a NULL base.
> 
> But, back to the problem, pcc_mbox_free_channel() unmaps the shared
> memory and clears pchan->chan.shmem *before* freeing the IRQ (aka
> calling mbox_free_channel()).
> 
> The interrupt is still live when the mapping goes away.
> 
> Free the channel first, before the memory unmap. mbox_free_channel()
> calls pcc_shutdown(), which frees the platform interrupt, and then unmap
> shared memory.
> 
> Fixes: 7f9e19f207be ("mailbox: pcc: Check before sending MCTP PCC response ACK")

Reviewed-by: Sudeep Holla <sudeep.holla@kernel.org>

Digging my git repo, I found I had this fix initially as part of
Commit 9f3bbbb72ad0 ("mailbox: pcc: Initialize SHMEM before binding the
channel with the client") when I encountered that issue but then removed
to post it as separate fix which I never did 🙁.

Thanks for catching this, much appreciated!

-- 
Regards,
Sudeep

Re: [PATCH] mailbox: pcc: Free the channel before unmapping the shared memory

Posted by Sudeep Holla 1 month, 2 weeks ago

On Wed, Aug 12, 2026 at 05:36:03AM -0700, Breno Leitao wrote:
> I am seeing a crash on PCC that is related to a an shared memory being
> unmapped before the IRQ is disabled, and the IRQ kicks in and hits the
> unmapped (NULL) address. This is a summary of what I see on my box:
> 
> 	scmi_protocol scmi_dev.1: Message for 1 type 0 is not expected!
> 	Unable to handle kernel NULL pointer dereference at virtual address 0000000000000004
> 	 __handle_irq_event_percpu+0x1c4/0x9e0
> 	 handle_irq_event+0x98/0x218
> 	 handle_fasteoi_irq+0x230/0x750
> 	 generic_handle_domain_irq+0xac/0x138
> 	 gic_handle_irq+0x344/0x740
> 	 call_on_irq_stack+0x30/0x48
> 

Just curious as how are you testing ACPI PCC + SCMI using upstream and
on which platform ?

-- 
Regards,
Sudeep

Re: [PATCH] mailbox: pcc: Free the channel before unmapping the shared memory

Posted by Breno Leitao 1 month, 2 weeks ago

Hello Sudeep,

On Thu, Aug 13, 2026 at 03:57:29AM +0100, Sudeep Holla wrote:
> On Wed, Aug 12, 2026 at 05:36:03AM -0700, Breno Leitao wrote:
> > I am seeing a crash on PCC that is related to a an shared memory being
> > unmapped before the IRQ is disabled, and the IRQ kicks in and hits the
> > unmapped (NULL) address. This is a summary of what I see on my box:
> > 
> > 	scmi_protocol scmi_dev.1: Message for 1 type 0 is not expected!
> > 	Unable to handle kernel NULL pointer dereference at virtual address 0000000000000004
> > 	 __handle_irq_event_percpu+0x1c4/0x9e0
> > 	 handle_irq_event+0x98/0x218
> > 	 handle_fasteoi_irq+0x230/0x750
> > 	 generic_handle_domain_irq+0xac/0x138
> > 	 gic_handle_irq+0x344/0x740
> > 	 call_on_irq_stack+0x30/0x48
> > 
> 
> Just curious as how are you testing ACPI PCC + SCMI using upstream and
> on which platform ?

I am using Arm AGI CPU with the latest SCMI patches that are being under
discussion in the mailing list.

--breno

Re: [PATCH] mailbox: pcc: Free the channel before unmapping the shared memory

Posted by Sudeep Holla 1 month, 2 weeks ago

On Thu, Aug 13, 2026 at 01:18:58AM -0700, Breno Leitao wrote:
> Hello Sudeep,
> 
> On Thu, Aug 13, 2026 at 03:57:29AM +0100, Sudeep Holla wrote:
> > On Wed, Aug 12, 2026 at 05:36:03AM -0700, Breno Leitao wrote:
> > > I am seeing a crash on PCC that is related to a an shared memory being
> > > unmapped before the IRQ is disabled, and the IRQ kicks in and hits the
> > > unmapped (NULL) address. This is a summary of what I see on my box:
> > > 
> > > 	scmi_protocol scmi_dev.1: Message for 1 type 0 is not expected!
> > > 	Unable to handle kernel NULL pointer dereference at virtual address 0000000000000004
> > > 	 __handle_irq_event_percpu+0x1c4/0x9e0
> > > 	 handle_irq_event+0x98/0x218
> > > 	 handle_fasteoi_irq+0x230/0x750
> > > 	 generic_handle_domain_irq+0xac/0x138
> > > 	 gic_handle_irq+0x344/0x740
> > > 	 call_on_irq_stack+0x30/0x48
> > > 
> > 
> > Just curious as how are you testing ACPI PCC + SCMI using upstream and
> > on which platform ?
> 
> I am using Arm AGI CPU with the latest SCMI patches that are being under
> discussion in the mailing list.
> 

Thanks, I just wanted to rule out any hidden issues in the ACPI PCC driver
I wrote which hasn't got much review 😉.

-- 
Regards,
Sudeep