From: TanZheng <tanzheng@kylinos.cn>

If srpt_get_desc_tbl() fails, srpt_handle_cmd() jumps to
target_send_busy() before target_init_cmd() has set cmd->se_tfo.
target_send_busy() then dereferences a NULL se_tfo.

Call target_init_cmd() first with placeholder data_length and
data_direction, parse the descriptor table next, then fill in the
real data_length. That way target_send_busy() is safe if parsing
fails.

Fixes: 8b8807b9e982 ("scsi: RDMA/srpt: Fix handling of command / TMR submission failure")
Link: https://lore.kernel.org/all/20260717025402.64054-1-kensanya@163.com/
Suggested-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: TanZheng <tanzheng@kylinos.cn>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 31 ++++++++++++++++++---------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index f66cfd70c263..379718453a7d 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1558,8 +1558,8 @@ static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
 	struct srp_cmd *srp_cmd;
 	struct scatterlist *sg = NULL;
 	unsigned sg_cnt = 0;
-	u64 data_len;
-	enum dma_data_direction dir;
+	u64 data_len = 0;
+	enum dma_data_direction dir = DMA_NONE;
 	int rc;
 
 	BUG_ON(!send_ioctx);
@@ -1584,6 +1584,20 @@ static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
 		break;
 	}
 
+	/*
+	 * Before calling srpt_get_desc_tbl(), call target_init_cmd() first
+	 * to set cmd->se_tfo and use target_send_busy(). The data_length
+	 * will be filled after the successful call of srpt_get_desc_tbl().
+	 */
+	rc = target_init_cmd(cmd, ch->sess, &send_ioctx->sense_data[0],
+			     scsilun_to_int(&srp_cmd->lun), 0 /* data_len */,
+			     TCM_SIMPLE_TAG, DMA_NONE, TARGET_SCF_ACK_KREF);
+	if (rc != 0) {
+		pr_debug("target_init_cmd() returned %d for tag %#llx\n", rc,
+			 srp_cmd->tag);
+		goto busy;
+	}
+
 	rc = srpt_get_desc_tbl(recv_ioctx, send_ioctx, srp_cmd, &dir,
 			       &sg, &sg_cnt, &data_len, ch->imm_data_offset);
 	if (rc) {
@@ -1594,14 +1608,11 @@ static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
 		goto busy;
 	}
 
-	rc = target_init_cmd(cmd, ch->sess, &send_ioctx->sense_data[0],
-			     scsilun_to_int(&srp_cmd->lun), data_len,
-			     TCM_SIMPLE_TAG, dir, TARGET_SCF_ACK_KREF);
-	if (rc != 0) {
-		pr_debug("target_submit_cmd() returned %d for tag %#llx\n", rc,
-			 srp_cmd->tag);
-		goto busy;
-	}
+	/*
+	 * Replace the placeholder length from target_init_cmd(). Direction
+	 * was already set inside srpt_get_desc_tbl() for srpt_alloc_rw_ctxs().
+	 */
+	cmd->data_length = data_len;
 
 	if (target_submit_prep(cmd, srp_cmd->cdb, sg, sg_cnt, NULL, 0, NULL, 0,
 			       GFP_KERNEL))
-- 
2.25.1

On 7/19/26 7:59 PM, kensanya@163.com wrote:
> +	rc = target_init_cmd(cmd, ch->sess, &send_ioctx->sense_data[0],
> +			     scsilun_to_int(&srp_cmd->lun), 0 /* data_len */,
> +			     TCM_SIMPLE_TAG, DMA_NONE, TARGET_SCF_ACK_KREF);
> +	if (rc != 0) {
> +		pr_debug("target_init_cmd() returned %d for tag %#llx\n", rc,
> +			 srp_cmd->tag);
> +		goto busy;
> +	}

This will cause target_send_busy() to be called on an uninitialized 
command and that isn't allowed, isn't it?

Thanks,

Bart.

At 2026-07-21 00:31:58, "Bart Van Assche" <bvanassche@acm.org> wrote:
>On 7/19/26 7:59 PM, kensanya@163.com wrote:
>> +	rc = target_init_cmd(cmd, ch->sess, &send_ioctx->sense_data[0],
>> +			     scsilun_to_int(&srp_cmd->lun), 0 /* data_len */,
>> +			     TCM_SIMPLE_TAG, DMA_NONE, TARGET_SCF_ACK_KREF);
>> +	if (rc != 0) {
>> +		pr_debug("target_init_cmd() returned %d for tag %#llx\n", rc,
>> +			 srp_cmd->tag);
>> +		goto busy;
>> +	}
>
>This will cause target_send_busy() to be called on an uninitialized 
>command and that isn't allowed, isn't it?
>
>Thanks,
>
>Bart.

You are right that target_send_busy() must not be used when
target_init_cmd() fails: the command is not fully set up for the
target-core completion path (e.g. cmd_cnt was not taken on
-ESHUTDOWN). That goto-busy-on-init-failure path already existed
before the reorder.

How about the following plan?
  - If target_init_cmd() fails:
      release the send ioctx locally via target_free_tag(ch->sess, cmd)
      and do not call target_send_busy()
  - If target_init_cmd() succeeds but a later step fails
    (e.g. srpt_get_desc_tbl()):
      keep using target_send_busy(cmd)

Thanks，
TanZheng

On 7/21/26 6:52 PM, kensanya wrote:
> How about the following plan?
>    - If target_init_cmd() fails:
>        release the send ioctx locally via target_free_tag(ch->sess, cmd)
>        and do not call target_send_busy()
>    - If target_init_cmd() succeeds but a later step fails
>      (e.g. srpt_get_desc_tbl()):
>        keep using target_send_busy(cmd)

That sounds good to me.

Thanks,

Bart.

At 2026-07-23 02:10:13, "Bart Van Assche" <bvanassche@acm.org> wrote:
>On 7/21/26 6:52 PM, kensanya wrote:
>> How about the following plan?
>>    - If target_init_cmd() fails:
>>        release the send ioctx locally via target_free_tag(ch->sess, cmd)
>>        and do not call target_send_busy()
>>    - If target_init_cmd() succeeds but a later step fails
>>      (e.g. srpt_get_desc_tbl()):
>>        keep using target_send_busy(cmd)
>
>That sounds good to me.
>
>Thanks,
>
>Bart.

I will send the v2 patch later.

Thanks,
TanZheng