[PATCH v2 0/2] RDMA/srp: fix use-after-free of a request in srp_destroy_qp()

[PATCH v2 0/2] RDMA/srp: fix use-after-free of a request in srp_destroy_qp()

[Yehyeong Lee]

Bart asked whether reordering srp_remove_target() also fixes the crash.
It does.  Against the same test target, the report appears in 5 of 5
runs without his patch and in 0 of 5 with it.  The drain still finds
the same two completions; the request pool now outlives it.  A
conforming target was unchanged over 5 runs each way.  Both patches
applied together behave the same way.  I never reached the SCSI error
handler.

Each patch stops the crash on its own, so this series carries both.
Patch 1 takes the shared ib_cqe out of the request; it is tagged for
stable.  Patch 2 is Bart's, carried as posted.  Whether it should go to
stable too is his call.

Two notes on patch 2, from source only.  srp_stop_rport_timers() is
documented as "Must be called after srp_remove_host() and
scsi_remove_host()" (scsi_transport_srp.c:789); patch 2 calls it before
both.  And the host now stays SHOST_RUNNING until the end, so a device
added by a late scan would miss the cache sync.  Its commands fail with
DID_NO_CONNECT, so that is a lost sync, not a crash.

v1: https://lore.kernel.org/linux-rdma/20260812190418.200337-1-yhlee@isslab.korea.ac.kr/

Bart Van Assche (1):
  RDMA/srp: Fix srp_remove_target()

Yehyeong Lee (1):
  RDMA/srp: fix use-after-free of a request in srp_destroy_qp()

 drivers/infiniband/ulp/srp/ib_srp.c | 30 +++++++++++++++++------------
 drivers/infiniband/ulp/srp/ib_srp.h |  8 +++++++-
 2 files changed, 25 insertions(+), 13 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] RDMA/srp: fix use-after-free of a request in srp_destroy_qp()

[Yehyeong Lee]

srp_destroy_qp() drains the send queue before destroying the queue pair.
For the fast registration and invalidation work requests the wr_cqe it
finds there is &req->reg_cqe, which lives in the blk-mq request pool, and
srp_remove_target() has already freed that pool through
scsi_remove_host(): page_owner records the page freed by
blk_mq_free_tags_callback() while that call is running.
__ib_process_cq() then calls wc->wr_cqe->done on it.

A target whose login fails after a command has been mapped leaves such a
work request flushed in the send queue, and the drain reads it after the
pool is gone.

  [   18.969484] BUG: KASAN: use-after-free in __ib_process_cq+0x2ec/0x390
  [   18.970521] Read of size 8 at addr ffff888107cc5a50 by task kworker/0:1/11
  [   18.971544]
  [   18.971811] CPU: 0 UID: 0 PID: 11 Comm: kworker/0:1 Not tainted 7.2.0-rc5-CLEANA-gd788e9657d56-dirty #17 PREEMPT(lazy)
  [   18.971819] Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
  [   18.971831] Workqueue: srp_remove srp_remove_work
  [   18.971868] Call Trace:
  [   18.971879]  <TASK>
  [   18.971883]  dump_stack_lvl+0x53/0x70
  [   18.971937]  print_report+0xd0/0x630
  [   18.971987]  ? __pfx__raw_spin_lock_irqsave+0x10/0x10
  [   18.972010]  ? _raw_spin_lock_irqsave+0x85/0xe0
  [   18.972017]  ? __ib_process_cq+0x2ec/0x390
  [   18.972022]  kasan_report+0xce/0x100
  [   18.972027]  ? __ib_process_cq+0x2ec/0x390
  [   18.972033]  __ib_process_cq+0x2ec/0x390
  [   18.972038]  ib_process_cq_direct+0x8e/0xd0
  [   18.972043]  ? __pfx_ib_process_cq_direct+0x10/0x10
  [   18.972053]  ? _raw_spin_lock_irq+0x80/0xe0
  [   18.972057]  ? __pfx__raw_spin_lock_irq+0x10/0x10
  [   18.972061]  ? kfree+0x121/0x380
  [   18.972083]  srp_free_ch_ib+0x284/0xc00
  [   18.972090]  srp_remove_work+0x30f/0x650
  [   18.972095]  process_one_work+0x633/0x1030
  [   18.972127]  ? assign_work+0x11d/0x370
  [   18.972132]  worker_thread+0x45b/0xd10
  [   18.972138]  ? __pfx_worker_thread+0x10/0x10
  [   18.972144]  ? __pfx_worker_thread+0x10/0x10
  [   18.972149]  kthread+0x2c6/0x3b0
  [   18.972163]  ? recalc_sigpending+0x15c/0x1e0
  [   18.972184]  ? __pfx_kthread+0x10/0x10
  [   18.972188]  ret_from_fork+0x36e/0x5a0
  [   18.972212]  ? __pfx_ret_from_fork+0x10/0x10
  [   18.972217]  ? __switch_to+0x572/0xdd0
  [   18.972231]  ? __pfx_kthread+0x10/0x10
  [   18.972236]  ret_from_fork_asm+0x1a/0x30
  [   18.972247]  </TASK>
  [   18.972250]
  [   18.993356] The buggy address belongs to the physical page:
  [   18.994084] page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x107cc5
  [   18.995090] flags: 0x200000000000000(node=0|zone=2)
  [   18.995739] raw: 0200000000000000 ffffea00041f3148 ffffea00041f3148 0000000000000000
  [   18.996725] raw: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
  [   18.997705] page dumped because: kasan: bad access detected
  [   18.998427]
  [   18.998644] Memory state around the buggy address:
  [   18.999235]  ffff888107cc5900: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
  [   19.000867]  ffff888107cc5980: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
  [   19.002577] >ffff888107cc5a00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
  [   19.003500]                                                  ^
  [   19.004261]  ffff888107cc5a80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
  [   19.005198]  ffff888107cc5b00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff

The completion does not identify the request.  Both handlers hand it to
srp_handle_qp_err(), which finds the channel through cq->cq_context, and
neither work request is signalled on success.  Put the two completion
contexts in the channel, which outlives the queue pair.

Fixes: 9294000d6d89 ("IB/srp: Drain the send queue before destroying a QP")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 16 ++++++++--------
 drivers/infiniband/ulp/srp/ib_srp.h |  8 +++++++-
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 6b429ef63f8f3..f3f19277c2720 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -144,6 +144,8 @@ static void srp_rename_dev(struct ib_device *device, void *client_data);
 static void srp_recv_done(struct ib_cq *cq, struct ib_wc *wc);
 static void srp_handle_qp_err(struct ib_cq *cq, struct ib_wc *wc,
 		const char *opname);
+static void srp_inv_rkey_err_done(struct ib_cq *cq, struct ib_wc *wc);
+static void srp_reg_mr_err_done(struct ib_cq *cq, struct ib_wc *wc);
 static int srp_ib_cm_handler(struct ib_cm_id *cm_id,
 			     const struct ib_cm_event *event);
 static int srp_rdma_cm_handler(struct rdma_cm_id *cm_id,
@@ -604,6 +606,8 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch)
 	ch->qp = qp;
 	ch->recv_cq = recv_cq;
 	ch->send_cq = send_cq;
+	ch->reg_cqe.done = srp_reg_mr_err_done;
+	ch->inv_cqe.done = srp_inv_rkey_err_done;
 
 	if (dev->use_fast_reg) {
 		if (ch->fr_pool)
@@ -1159,8 +1163,7 @@ static void srp_inv_rkey_err_done(struct ib_cq *cq, struct ib_wc *wc)
 	srp_handle_qp_err(cq, wc, "INV RKEY");
 }
 
-static int srp_inv_rkey(struct srp_request *req, struct srp_rdma_ch *ch,
-		u32 rkey)
+static int srp_inv_rkey(struct srp_rdma_ch *ch, u32 rkey)
 {
 	struct ib_send_wr wr = {
 		.opcode		    = IB_WR_LOCAL_INV,
@@ -1170,8 +1173,7 @@ static int srp_inv_rkey(struct srp_request *req, struct srp_rdma_ch *ch,
 		.ex.invalidate_rkey = rkey,
 	};
 
-	wr.wr_cqe = &req->reg_cqe;
-	req->reg_cqe.done = srp_inv_rkey_err_done;
+	wr.wr_cqe = &ch->inv_cqe;
 	return ib_post_send(ch->qp, &wr, NULL);
 }
 
@@ -1193,7 +1195,7 @@ static void srp_unmap_data(struct scsi_cmnd *scmnd,
 		struct srp_fr_desc **pfr;
 
 		for (i = req->nmdesc, pfr = req->fr_list; i > 0; i--, pfr++) {
-			res = srp_inv_rkey(req, ch, (*pfr)->mr->rkey);
+			res = srp_inv_rkey(ch, (*pfr)->mr->rkey);
 			if (res < 0) {
 				shost_printk(KERN_ERR, target->scsi_host, PFX
 				  "Queueing INV WR for rkey %#x failed (%d)\n",
@@ -1470,11 +1472,9 @@ static int srp_map_finish_fr(struct srp_map_state *state,
 
 	WARN_ON_ONCE(desc->mr->length == 0);
 
-	req->reg_cqe.done = srp_reg_mr_err_done;
-
 	wr.wr.next = NULL;
 	wr.wr.opcode = IB_WR_REG_MR;
-	wr.wr.wr_cqe = &req->reg_cqe;
+	wr.wr.wr_cqe = &ch->reg_cqe;
 	wr.wr.num_sge = 0;
 	wr.wr.send_flags = 0;
 	wr.mr = desc->mr;
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 349576ff38450..22720f103d4eb 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -137,7 +137,6 @@ struct srp_request {
 	struct srp_direct_buf  *indirect_desc;
 	dma_addr_t		indirect_dma_addr;
 	short			nmdesc;
-	struct ib_cqe		reg_cqe;
 };
 
 /**
@@ -145,6 +144,10 @@ struct srp_request {
  * @comp_vector: Completion vector used by this RDMA channel.
  * @max_it_iu_len: Maximum initiator-to-target information unit length.
  * @max_ti_iu_len: Maximum target-to-initiator information unit length.
+ * @reg_cqe: Completion context for fast registration work requests.
+ * @inv_cqe: Completion context for invalidation work requests.  These are
+ *	per channel because their completions do not identify the request and
+ *	because the request pool can be freed before the send queue is drained.
  */
 struct srp_rdma_ch {
 	/* These are RW in the hot path, and commonly used together */
@@ -170,6 +173,9 @@ struct srp_rdma_ch {
 	struct completion	done;
 	int			status;
 
+	struct ib_cqe		reg_cqe;
+	struct ib_cqe		inv_cqe;
+
 	union {
 		struct ib_cm {
 			struct sa_path_rec	path;
-- 
2.43.0

Re: [PATCH v2 1/2] RDMA/srp: fix use-after-free of a request in srp_destroy_qp()

[Bart Van Assche]

On 8/17/26 8:52 PM, Yehyeong Lee wrote:
> -static int srp_inv_rkey(struct srp_request *req, struct srp_rdma_ch *ch,
> -		u32 rkey)
> +static int srp_inv_rkey(struct srp_rdma_ch *ch, u32 rkey)
>   {
>   	struct ib_send_wr wr = {
>   		.opcode		    = IB_WR_LOCAL_INV,
> @@ -1170,8 +1173,7 @@ static int srp_inv_rkey(struct srp_request *req, struct srp_rdma_ch *ch,
>   		.ex.invalidate_rkey = rkey,
>   	};
>   
> -	wr.wr_cqe = &req->reg_cqe;
> -	req->reg_cqe.done = srp_inv_rkey_err_done;
> +	wr.wr_cqe = &ch->inv_cqe;
>   	return ib_post_send(ch->qp, &wr, NULL);
>   }

This patch introduces a race condition because it associates a single
CQE data structure with multiple concurrently submitted work requests.
Please drop this patch.

Thanks,

Bart.

Re: [PATCH v2 1/2] RDMA/srp: fix use-after-free of a request in srp_destroy_qp()

[Yehyeong Lee]

My answer to this went out without In-Reply-To and started its own
thread:

https://lore.kernel.org/linux-rdma/20260819014815.642500-1-yhlee@isslab.korea.ac.kr/

Best regards,

Yehyeong Lee

[PATCH v2 2/2] RDMA/srp: Fix srp_remove_target()

[Yehyeong Lee]

From: Bart Van Assche <bvanassche@acm.org>

Remove all logical units before disconnecting the transport because one or
more SCSI commands may be submitted while removing logical units. Remove
the SCSI host after the transport has been disconnected because the code
that disconnects the transport needs resources that are freed by the code
that removes the SCSI host (SCSI host tag set). Remove the srp_rport_get()
and srp_rport_put() calls because the purpose of these calls was to keep
the rport until tl_err_work is cancelled.

Reported-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
Closes: https://lore.kernel.org/linux-rdma/20260812190418.200337-1-yhlee@isslab.korea.ac.kr/
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index f3f19277c2720..ee2bd209b5cde 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1042,15 +1042,20 @@ static void srp_del_scsi_host_attr(struct Scsi_Host *shost)
 
 static void srp_remove_target(struct srp_target_port *target)
 {
+	struct scsi_device *sdev;
 	struct srp_rdma_ch *ch;
 	int i;
 
 	WARN_ON_ONCE(target->state != SRP_TARGET_REMOVED);
 
 	srp_del_scsi_host_attr(target->scsi_host);
-	srp_rport_get(target->rport);
-	srp_remove_host(target->scsi_host);
-	scsi_remove_host(target->scsi_host);
+	/*
+	 * Remove all logical units. This must happen before the
+	 * srp_disconnect_target() call because scsi_remove_device() may trigger
+	 * submission of SCSI commands. See also sd_shutdown().
+	 */
+	shost_for_each_device(sdev, target->scsi_host)
+		scsi_remove_device(sdev);
 	srp_stop_rport_timers(target->rport);
 	srp_disconnect_target(target);
 	kobj_ns_drop(KOBJ_NS_TYPE_NET, to_ns_common(target->net));
@@ -1059,7 +1064,8 @@ static void srp_remove_target(struct srp_target_port *target)
 		srp_free_ch_ib(target, ch);
 	}
 	cancel_work_sync(&target->tl_err_work);
-	srp_rport_put(target->rport);
+	srp_remove_host(target->scsi_host);
+	scsi_remove_host(target->scsi_host);
 	kfree(target->ch);
 	target->ch = NULL;
 
-- 
2.43.0

Re: (subset) [PATCH v2 0/2] RDMA/srp: fix use-after-free of a request in srp_destroy_qp()

[Leon Romanovsky]

On Tue, 18 Aug 2026 12:52:27 +0900, Yehyeong Lee wrote:
> Bart asked whether reordering srp_remove_target() also fixes the crash.
> It does.  Against the same test target, the report appears in 5 of 5
> runs without his patch and in 0 of 5 with it.  The drain still finds
> the same two completions; the request pool now outlives it.  A
> conforming target was unchanged over 5 runs each way.  Both patches
> applied together behave the same way.  I never reached the SCSI error
> handler.
> 
> [...]

Applied, thanks!

[2/2] RDMA/srp: Fix srp_remove_target()
      https://git.kernel.org/rdma/rdma/c/9cdfad5dd5529e

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

Re: (subset) [PATCH v2 0/2] RDMA/srp: fix use-after-free of a request in srp_destroy_qp()

[Bart Van Assche]

On 9/2/26 5:59 AM, Leon Romanovsky wrote:
> Applied, thanks!
> 
> [2/2] RDMA/srp: Fix srp_remove_target()
>        https://git.kernel.org/rdma/rdma/c/9cdfad5dd5529e

Thanks Leon!

Bart.

Re: [PATCH v2 1/2] RDMA/srp: fix use-after-free of a request in srp_destroy_qp()

[Yehyeong Lee]

On 8/18/26 9:26 AM, Bart Van Assche wrote:
> This patch introduces a race condition because it associates a single
> CQE data structure with multiple concurrently submitted work requests.

Thanks for looking at this.

struct ib_cqe holds one member.  After this patch ch->reg_cqe.done and
ch->inv_cqe.done are written only in srp_create_ch_ib(), either before
the first QP exists or after srp_destroy_qp() has drained and destroyed
the previous one, and both writes store the same value every time.
srp_handle_qp_err() finds the channel through cq->cq_context, so neither
handler reads the cqe back.

The sharing is not new: one command posts up to mr_per_cmd REG_MR work
requests, all carrying &req->reg_cqe.  ib_srpt shares ch->zw_cqe the
same way this patch shares ch->reg_cqe.  Could you point at the two work
requests you have in mind?

One thing does get worse: "for CQE %p" in srp_handle_qp_err() used to
differ per request and now takes one of two values per channel.

If a completion has to identify its work request here, I accept that and
will drop patch 1.  Should patch 2 then carry Cc: stable?  Patch 1 is
the one tagged now.

Best regards,

Yehyeong Lee