mailbox: Use per-thread completion to fix wrong completion order

PATCH RESEND Superseded Joonwon Kang 2 participants 3 messages Dec 16, 2025 – Feb 6

Subsystems (3)

Stable branch Tegra architecture support Mailbox API

lore

Share & tools

lore

Joonwon Kang

author

2025-12-16 08:43 UTC #0 lore raw

PATCH RESEND 1/2 mailbox: Use per-thread completion to fix wrong completion order

Previously, a sender thread in mbox_send_message() could be woken up at a wrong time in blocking mode. It is because there was only a single completion for a channel whereas messages from multiple threads could be sent on the same channel in any order; since the shared completion could be signalled in any order, it could wake up a wrong sender thread.

This commit resolves the false wake-up issue with the following changes:
- Completions are created just as many as the number of concurrent sender threads
- A completion is created on a sender thread's stack
- Each slot of the message queue, i.e. msg_data, contains a pointer to its target completion
- tx_tick() signals the completion of the currently active slot of the message queue

Cc [email protected]

Link https://lore.kernel.org/all/[email protected] open here

Signed-off-by Joonwon Kang <[email protected]>

---
Link -> v1: The previous solution in the Link tries to have per-message
completion: tx_cmpl[MBOX_TX_QUEUE_LEN]; each completion belongs to each slot of the message queue: msg_data[i]. Those completions take up additional memory even when they are not used. Instead, this patch tries to have per-"thread" completion; each completion belongs to each sender thread and each slot of the message queue has a pointer to that completion; struct mbox_message has the "pointer" field struct completion *tx_complete which points to the completion which is created on the stack of the sender, instead of owning the completion by struct completion tx_complete. This way, we could avoid additional memory use since a completion will be allocated only when necessary. Plus, more importantly, we could avoid the window where the same completion is reused by different sender threads which the previous solution still has.

 drivers/mailbox/mailbox.c          | 43 +++++++++++++++++++-----------
 drivers/mailbox/tegra-hsp.c        |  2 +-
 include/linux/mailbox_controller.h | 20 +++++++++-----
 3 files changed, 43 insertions(+), 22 deletions(-)

drivers/mailbox/mailbox.c +28 ~~-15~~

index 617ba505691d..0afe3ae3bfdc 100644
@@ -23,7 +23,7 @@
 static LIST_HEAD(mbox_cons);
 static DEFINE_MUTEX(con_mutex);
 
-static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
+static int add_to_rbuf(struct mbox_chan *chan, void *mssg, struct completion *tx_complete)
 {
 	int idx;

@@ -34,7 +34,8 @@ static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
 		return -ENOBUFS;
 
 	idx = chan->msg_free;
-	chan->msg_data[idx] = mssg;
+	chan->msg_data[idx].data = mssg;
+	chan->msg_data[idx].tx_complete = tx_complete;
 	chan->msg_count++;
 
 	if (idx == MBOX_TX_QUEUE_LEN - 1)

@@ -52,7 +53,7 @@ static void msg_submit(struct mbox_chan *chan)
 	int err = -EBUSY;
 
 	scoped_guard(spinlock_irqsave, &chan->lock) {
-		if (!chan->msg_count || chan->active_req)
+		if (!chan->msg_count || chan->active_req >= 0)
 			break;
 
 		count = chan->msg_count;

@@ -62,14 +63,14 @@ static void msg_submit(struct mbox_chan *chan)
 		else
 			idx += MBOX_TX_QUEUE_LEN - count;
 
-		data = chan->msg_data[idx];
+		data = chan->msg_data[idx].data;
 
 		if (chan->cl->tx_prepare)
 			chan->cl->tx_prepare(chan->cl, data);
 		/* Try to submit a message to the MBOX controller */
 		err = chan->mbox->ops->send_data(chan, data);
 		if (!err) {
-			chan->active_req = data;
+			chan->active_req = idx;
 			chan->msg_count--;
 		}
 	}

@@ -83,11 +84,17 @@ static void msg_submit(struct mbox_chan *chan)
 
 static void tx_tick(struct mbox_chan *chan, int r)
 {
-	void *mssg;
+	int idx;
+	void *mssg = NULL;
+	struct completion *tx_complete = NULL;
 
 	scoped_guard(spinlock_irqsave, &chan->lock) {
-		mssg = chan->active_req;
-		chan->active_req = NULL;
+		idx = chan->active_req;
+		if (idx >= 0) {
+			mssg = chan->msg_data[idx].data;
+			tx_complete = chan->msg_data[idx].tx_complete;
+			chan->active_req = -1;
+		}
 	}
 
 	/* Submit next message */

@@ -101,7 +108,7 @@ static void tx_tick(struct mbox_chan *chan, int r)
 		chan->cl->tx_done(chan->cl, mssg, r);
 
 	if (r != -ETIME && chan->cl->tx_block)
-		complete(&chan->tx_complete);
+		complete(tx_complete);
 }
 
 static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)

@@ -114,7 +121,7 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
 	for (i = 0; i < mbox->num_chans; i++) {
 		struct mbox_chan *chan = &mbox->chans[i];
 
-		if (chan->active_req && chan->cl) {
+		if (chan->active_req >= 0 && chan->cl) {
 			txdone = chan->mbox->ops->last_tx_done(chan);
 			if (txdone)
 				tx_tick(chan, 0);

@@ -245,11 +252,18 @@ EXPORT_SYMBOL_GPL(mbox_client_peek_data);
 int mbox_send_message(struct mbox_chan *chan, void *mssg)
 {
 	int t;
+	struct completion tx_complete;
 
 	if (!chan || !chan->cl)
 		return -EINVAL;
 
-	t = add_to_rbuf(chan, mssg);
+	if (chan->cl->tx_block) {
+		init_completion(&tx_complete);
+		t = add_to_rbuf(chan, mssg, &tx_complete);
+	} else {
+		t = add_to_rbuf(chan, mssg, NULL);
+	}
+
 	if (t < 0) {
 		dev_err(chan->mbox->dev, "Try increasing MBOX_TX_QUEUE_LEN\n");
 		return t;

@@ -266,7 +280,7 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)
 		else
 			wait = msecs_to_jiffies(chan->cl->tx_tout);
 
-		ret = wait_for_completion_timeout(&chan->tx_complete, wait);
+		ret = wait_for_completion_timeout(&tx_complete, wait);
 		if (ret == 0) {
 			t = -ETIME;
 			tx_tick(chan, t);

@@ -319,9 +333,8 @@ static int __mbox_bind_client(struct mbox_chan *chan, struct mbox_client *cl)
 	scoped_guard(spinlock_irqsave, &chan->lock) {
 		chan->msg_free = 0;
 		chan->msg_count = 0;
-		chan->active_req = NULL;
+		chan->active_req = -1;
 		chan->cl = cl;
-		init_completion(&chan->tx_complete);
 
 		if (chan->txdone_method	== TXDONE_BY_POLL && cl->knows_txdone)
 			chan->txdone_method = TXDONE_BY_ACK;

@@ -477,7 +490,7 @@ void mbox_free_channel(struct mbox_chan *chan)
 	/* The queued TX requests are simply aborted, no callbacks are made */
 	scoped_guard(spinlock_irqsave, &chan->lock) {
 		chan->cl = NULL;
-		chan->active_req = NULL;
+		chan->active_req = -1;
 		if (chan->txdone_method == TXDONE_BY_ACK)
 			chan->txdone_method = TXDONE_BY_POLL;
 	}

drivers/mailbox/tegra-hsp.c +1 -1

index ed9a0bb2bcd8..de7494ce0a9f 100644
@@ -497,7 +497,7 @@ static int tegra_hsp_mailbox_flush(struct mbox_chan *chan,
 			mbox_chan_txdone(chan, 0);
 
 			/* Wait until channel is empty */
-			if (chan->active_req != NULL)
+			if (chan->active_req >= 0)
 				continue;
 
 			return 0;

include/linux/mailbox_controller.h +14 -6

index 80a427c7ca29..67e08a440f5f 100644
@@ -105,16 +105,25 @@ struct mbox_controller {
  */
 #define MBOX_TX_QUEUE_LEN	20
 
+/**
+ * struct mbox_message - Internal representation of a mailbox message
+ * @data:		Data packet
+ * @tx_complete:	Pointer to the transmission completion
+ */
+struct mbox_message {
+	void *data;
+	struct completion *tx_complete;
+};
+
 /**
  * struct mbox_chan - s/w representation of a communication chan
  * @mbox:		Pointer to the parent/provider of this channel
  * @txdone_method:	Way to detect TXDone chosen by the API
  * @cl:			Pointer to the current owner of this channel
- * @tx_complete:	Transmission completion
- * @active_req:		Currently active request hook
+ * @active_req:		Index of the currently active slot in the queue
  * @msg_count:		No. of mssg currently queued
  * @msg_free:		Index of next available mssg slot
- * @msg_data:		Hook for data packet
+ * @msg_data:		Queue of data packets
  * @lock:		Serialise access to the channel
  * @con_priv:		Hook for controller driver to attach private data
  */

@@ -122,10 +131,9 @@ struct mbox_chan {
 	struct mbox_controller *mbox;
 	unsigned txdone_method;
 	struct mbox_client *cl;
-	struct completion tx_complete;
-	void *active_req;
+	int active_req;
 	unsigned msg_count, msg_free;
-	void *msg_data[MBOX_TX_QUEUE_LEN];
+	struct mbox_message msg_data[MBOX_TX_QUEUE_LEN];
 	spinlock_t lock; /* Serialise access to the channel */
 	void *con_priv;
 };

Signature · 1 line

2.52.0.239.gd5f0c6e74e-goog

Jassi Brar

maintainer

2026-01-25 01:42 UTC #1 lore raw

Quoted 7 lines · Joonwon Kang

Previously, a sender thread in mbox_send_message() could be woken up at a wrong time in blocking mode. It is because there was only a single completion for a channel whereas messages from multiple threads could be sent on the same channel in any order; since the shared completion could be signalled in any order, it could wake up a wrong sender thread.

Quoted 9 lines

This commit resolves the false wake-up issue with the following changes:
- Completions are created just as many as the number of concurrent sender threads
- A completion is created on a sender thread's stack
- Each slot of the message queue, i.e. msg_data, contains a pointer to its target completion
- tx_tick() signals the completion of the currently active slot of the message queue

Mailbox API does not support shared channels. Each channel is supposed to be owned by one client. Though a client can serve multiple users of the channel, but then it will have to serialize access to the channel. The implication is mailbox_send_message should not be called before the last call returns (in blocking mode).
Even with this patch, consider when threadA is active and threadB too is waiting next. If the tx_tout races with threadA's transmission, threadB may timeout and call tx_tick() on the channel thereby affecting threadA. Which also eventually proceeds to complete on threadB's tx_complete which was on the stack and hence no more exists thereby causing UAF. So if you have multiple users in blocking mode, have a local queuing mechanism.

Thanks.
Jassi

Joonwon Kang

author

2026-02-06 10:02 UTC #2 lore raw

Re: [PATCH 1/2 RESEND] mailbox: Use per-thread completion to fix

Quoted 15 lines

Quoted 6 lines
Previously, a sender thread in mbox_send_message() could be woken up at a wrong time in blocking mode. It is because there was only a single completion for a channel whereas messages from multiple threads could be sent on the same channel in any order; since the shared completion could be signalled in any order, it could wake up a wrong sender thread.
Quoted 9 lines
This commit resolves the false wake-up issue with the following changes:
- Completions are created just as many as the number of concurrent sender threads
- A completion is created on a sender thread's stack
- Each slot of the message queue, i.e. msg_data, contains a pointer to its target completion
- tx_tick() signals the completion of the currently active slot of the message queue

Quoted 5 lines

Mailbox API does not support shared channels. Each channel is supposed to be owned by one client. Though a client can serve multiple users of the channel, but then it will have to serialize access to the channel. The implication is mailbox_send_message should not be called before the last call returns (in blocking mode).

This sounds like a suddenly big change to the mailbox API after all other docs or discussion, e.g. Link in the commit message, imply that it supports multi- thread use case. I think it would be better to make it support multi-thread not to cause breaking changes to the mailbox client drivers. At least, we may be able to implement mbox_send_message() using mutex or other locks to still support multi-thread.

Quoted 6 lines

Even with this patch, consider when threadA is active and threadB too is waiting next. If the tx_tout races with threadA's transmission, threadB may timeout and call tx_tick() on the channel thereby affecting threadA. Which also eventually proceeds to complete on threadB's tx_complete which was on the stack and hence no more exists thereby causing UAF.

Indeed, thanks for this input. Here we need to consider how tx_tick() is is called in conjunction with other events like timeout. tx_tick() can be called either when timeout occurs or by client or controller. Below is the break down of the cases.

Case 1) Thread A is active and no timeout occurs to Thread B

In this case, tx_tick() will be called once the tx is done for Thread A and then Thread B will go next. So, no problem.

Case 2) Thread A is active but timeout occurs to Thread B

This is the case that you pointed out. In this case, we could cancel the request for Thread B not disrupting the active request of Thread A and it should be okay to cancel it since it is before sending it to the controller, i.e. before the call to mbox->ops->send_data(). By not sending it, tx_tick() should also not be called either by client or controller. So, it should be no problem.

Will create a new version of patch with this change.

Case 3) Thread A is active but timeout occurs to Thread A and tx_tick() is called for Thread A

Case 4) Thread A is done with tx, Thread B is now active but timeout occurs to Thread B and tx_tick() is called for Thread B

These cases could occur, e.g. when there is no way for controller to know a tx done interrupt that it receives is for the currently active request or for the previous one, or when the timeout occurrs just before the controller is about to call mbox_chan_txdone(). If it happens anyway, it could also cause inconsistency of the mailbox internal status, but UAF will not occur with the patch which handles Case 2. However, these cases are out of topic for multi-thread support. It is more about the timeout support. It could occur even in a single thread as follows.

- Thread A is active with a request and timeout occurs.
- Thread A goes with the next request and is active again.
- However, the controller calls mbox_chan_txdone(), thus tx_tick(), intending for the first request.
- The mailbox internal status goes inconsistent!
- The controller may soon call another mbox_chan_txdone() for the second request.

So, these timeout cases are existing issues regardless of whether it is single- thread or multi-thread and should be considered orthogonally.