[PATCH for-rc v2] RDMA/efa: Fix race between admin command timeout and completion

[PATCH for-rc v2] RDMA/efa: Fix race between admin command timeout and completion

[Yonatan Nachum]

An admin command that times out can race with the interrupt handler
processing its completion, in two ways.

The timeout path releases the completion context without synchronizing
against the handler. A handler that has already seen the command as
submitted then copies the CQE into the caller's response buffer after
the waiter returned and that buffer went out of scope.

The waiter can also observe the command as completed before the CQE was
copied, since the status is published first. It decodes its own
uninitialized response buffer, returns success with garbage contents and
leaks stack data to the caller. Both wait paths read the status without
the CQ lock.

Add a timed-out state and publish it under the CQ lock in both wait
paths before returning, so the handler, which only completes commands in
submitted state, cannot act on an abandoned context. In the interrupt
wait path test the return value of the wait instead of the status: the
handler signals the completion after copying the CQE, so a successful
wait implies a valid response. Read the status in the polling wait path
inside the critical section that drains the CQ, which also lets a
completion arriving on the last iteration be honored instead of reported
as a timeout.

Fixes: 0420e542569b ("RDMA/efa: Implement functions that submit and complete admin commands")
Reviewed-by: Michael Margolin <mrgolin@amazon.com>
Reviewed-by: Tom Sela <tomsela@amazon.com>
Signed-off-by: Yonatan Nachum <ynachum@amazon.com>
---
Changelog:
v2:
 * Read the completion status under the CQ lock in the polling path
   instead of only taking the lock around the timed-out store.
 * Use the wait_for_completion_timeout() return value in the interrupt
   path instead of reading the status unlocked to determine whether the
   command timed out.
v1: https://lore.kernel.org/all/20260901113016.581935-1-ynachum@amazon.com/

 drivers/infiniband/hw/efa/efa_com.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/hw/efa/efa_com.c b/drivers/infiniband/hw/efa/efa_com.c
index 04c6d63c449e..9c59d127e1e0 100644
--- a/drivers/infiniband/hw/efa/efa_com.c
+++ b/drivers/infiniband/hw/efa/efa_com.c
@@ -41,6 +41,7 @@ enum efa_cmd_status {
 	EFA_CMD_ALLOCATED,
 	EFA_CMD_SUBMITTED,
 	EFA_CMD_COMPLETED,
+	EFA_CMD_TIMED_OUT,
 };
 
 struct efa_comp_ctx {
@@ -591,6 +592,7 @@ static int efa_com_comp_status_to_errno(u8 comp_status)
 static int efa_com_wait_and_process_admin_cq_polling(struct efa_comp_ctx *comp_ctx,
 						     struct efa_com_admin_queue *aq)
 {
+	bool comp_timeout = false;
 	unsigned long timeout;
 	unsigned long flags;
 
@@ -599,12 +601,19 @@ static int efa_com_wait_and_process_admin_cq_polling(struct efa_comp_ctx *comp_c
 	while (1) {
 		spin_lock_irqsave(&aq->cq.lock, flags);
 		efa_com_handle_admin_completion(aq);
-		spin_unlock_irqrestore(&aq->cq.lock, flags);
-
-		if (comp_ctx->status != EFA_CMD_SUBMITTED)
+		if (comp_ctx->status != EFA_CMD_SUBMITTED) {
+			spin_unlock_irqrestore(&aq->cq.lock, flags);
 			break;
+		}
 
 		if (time_is_before_jiffies(timeout)) {
+			comp_ctx->status = EFA_CMD_TIMED_OUT;
+			comp_timeout = true;
+		}
+
+		spin_unlock_irqrestore(&aq->cq.lock, flags);
+
+		if (comp_timeout) {
 			ibdev_err_ratelimited(
 				aq->efa_dev,
 				"Wait for completion (polling) timeout\n");
@@ -625,10 +634,11 @@ static int efa_com_wait_and_process_admin_cq_polling(struct efa_comp_ctx *comp_c
 static int efa_com_wait_and_process_admin_cq_interrupts(struct efa_comp_ctx *comp_ctx,
 							struct efa_com_admin_queue *aq)
 {
-	unsigned long flags;
+	unsigned long flags, remaining_jiffies;
+	bool completed;
 
-	wait_for_completion_timeout(&comp_ctx->wait_event,
-				    usecs_to_jiffies(aq->completion_timeout));
+	remaining_jiffies = wait_for_completion_timeout(
+		&comp_ctx->wait_event, usecs_to_jiffies(aq->completion_timeout));
 
 	/*
 	 * In case the command wasn't completed find out the root cause.
@@ -636,14 +646,16 @@ static int efa_com_wait_and_process_admin_cq_interrupts(struct efa_comp_ctx *com
 	 * 1) No completion (timeout reached)
 	 * 2) There is completion but the device didn't get any msi-x interrupt.
 	 */
-	if (comp_ctx->status == EFA_CMD_SUBMITTED) {
+	if (remaining_jiffies == 0) {
 		spin_lock_irqsave(&aq->cq.lock, flags);
 		efa_com_handle_admin_completion(aq);
+		completed = (comp_ctx->status == EFA_CMD_COMPLETED);
+		comp_ctx->status = EFA_CMD_TIMED_OUT;
 		spin_unlock_irqrestore(&aq->cq.lock, flags);
 
 		atomic64_inc(&aq->stats.no_completion);
 
-		if (comp_ctx->status == EFA_CMD_COMPLETED)
+		if (completed)
 			ibdev_err_ratelimited(
 				aq->efa_dev,
 				"The device sent a completion but the driver didn't receive any MSI-X interrupt for admin cmd %s(%d) status %d (id: %d, sq producer: %d, sq consumer: %d, cq consumer: %d)\n",
-- 
2.50.1

Re: [PATCH for-rc v2] RDMA/efa: Fix race between admin command timeout and completion

[sashiko-bot]

> An admin command that times out can race with the interrupt handler
> processing its completion, in two ways.
> 
> The timeout path releases the completion context without synchronizing
> against the handler. A handler that has already seen the command as
> submitted then copies the CQE into the caller's response buffer after
> the waiter returned and that buffer went out of scope.
> 
> The waiter can also observe the command as completed before the CQE was
> copied, since the status is published first. It decodes its own
> uninitialized response buffer, returns success with garbage contents and
> leaks stack data to the caller. Both wait paths read the status without
> the CQ lock.
> 
> Add a timed-out state and publish it under the CQ lock in both wait
> paths before returning, so the handler, which only completes commands in
> submitted state, cannot act on an abandoned context. In the interrupt
> wait path test the return value of the wait instead of the status: the
> handler signals the completion after copying the CQE, so a successful
> wait implies a valid response. Read the status in the polling wait path
> inside the critical section that drains the CQ, which also lets a
> completion arriving on the last iteration be honored instead of reported
> as a timeout.
> 
> Fixes: 0420e542569b ("RDMA/efa: Implement functions that submit and complete admin commands")
> Reviewed-by: Michael Margolin <mrgolin@amazon.com>
> Reviewed-by: Tom Sela <tomsela@amazon.com>
> Signed-off-by: Yonatan Nachum <ynachum@amazon.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012165113.576899-1-ynachum@amazon.com?part=1

Re: [PATCH for-rc v2] RDMA/efa: Fix race between admin command timeout and completion

[Leon Romanovsky]

On Mon, 12 Oct 2026 16:51:13 +0000, Yonatan Nachum wrote:
> An admin command that times out can race with the interrupt handler
> processing its completion, in two ways.
> 
> The timeout path releases the completion context without synchronizing
> against the handler. A handler that has already seen the command as
> submitted then copies the CQE into the caller's response buffer after
> the waiter returned and that buffer went out of scope.
> 
> [...]

Applied, thanks!

[1/1] RDMA/efa: Fix race between admin command timeout and completion
      https://git.kernel.org/rdma/rdma/c/2e0df4424434f2

Best regards,
-- 
Leon Romanovsky <leon@kernel.org>