| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250618080339.969173-1-joonwonkang@google.com>
Date: Wed, 18 Jun 2025 08:03:39 +0000
From: Joonwon Kang <joonwonkang@...gle.com>
To: jassisinghbrar@...il.com, thierry.reding@...il.com
Cc: alexey.klimov@....com, sudeep.holla@....com, jonathanh@...dia.com,
linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org,
Joonwon Kang <joonwonkang@...gle.com>
Subject: [PATCH 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 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 as many as the number of concurrent sender
threads
- A completion is created in 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
Link: https://lore.kernel.org/all/1490809381-28869-1-git-send-email-jaswinder.singh@linaro.org
Signed-off-by: Joonwon Kang <joonwonkang@...gle.com>
---
drivers/mailbox/mailbox.c | 43 +++++++++++++++++++-----------
drivers/mailbox/tegra-hsp.c | 2 +-
include/linux/mailbox_controller.h | 20 +++++++++-----
3 files changed, 43 insertions(+), 22 deletions(-)
diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 5cd8ae222073..80cd310964a8 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -22,7 +22,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;
@@ -33,7 +33,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)
@@ -51,7 +52,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;
@@ -61,14 +62,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--;
}
}
@@ -82,11 +83,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 */
@@ -100,7 +107,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)
@@ -113,7 +120,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);
@@ -244,11 +251,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;
@@ -265,7 +279,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);
@@ -318,9 +332,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;
@@ -461,7 +474,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;
}
diff --git a/drivers/mailbox/tegra-hsp.c b/drivers/mailbox/tegra-hsp.c
index ed9a0bb2bcd8..de7494ce0a9f 100644
--- a/drivers/mailbox/tegra-hsp.c
+++ b/drivers/mailbox/tegra-hsp.c
@@ -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;
diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
index ad01c4082358..ae29fba3bc46 100644
--- a/include/linux/mailbox_controller.h
+++ b/include/linux/mailbox_controller.h
@@ -102,16 +102,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
*/
@@ -119,10 +128,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;
};
--
2.50.0.rc2.696.g1fc2a0284f-goog
Powered by blists - more mailing lists