Linux RDMA and InfiniBand development
 help / color / mirror / Atom feed
* [PATCH v2] RDMA/srpt: fix NULL deref when sending BUSY before target_init_cmd()
@ 2026-07-23  2:44 kensanya
       [not found] ` <20260723095120.GF110966@unreal>
  0 siblings, 1 reply; 5+ messages in thread
From: kensanya @ 2026-07-23  2:44 UTC (permalink / raw)
  To: bvanassche, jgg, leon; +Cc: linux-rdma, target-devel, linux-kernel, TanZheng

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.

target_send_busy() is only valid after a successful target_init_cmd().
If target_init_cmd() itself fails, free the send ioctx tag locally
with target_free_tag() instead of calling target_send_busy().

Fixes: 8b8807b9e982 ("scsi: RDMA/srpt: Fix handling of command / TMF 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>
---
v2:
- On target_init_cmd() failure, use target_free_tag() instead of
  target_send_busy()

 drivers/infiniband/ulp/srpt/ib_srpt.c | 36 +++++++++++++++++++--------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index f66cfd70c263..71af1fb0d380 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,25 @@ static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
 		break;
 	}
 
+	/*
+	 * Call target_init_cmd() before srpt_get_desc_tbl() so that
+	 * cmd->se_tfo is set if descriptor parsing fails and
+	 * target_send_busy() is used. data_length is filled in after a
+	 * successful srpt_get_desc_tbl().
+	 *
+	 * target_send_busy() is only valid after a successful
+	 * target_init_cmd(). On init failure free the tag locally.
+	 */
+	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);
+		target_free_tag(ch->sess, cmd);
+		return;
+	}
+
 	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 +1613,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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re:Re: [PATCH v2] RDMA/srpt: fix NULL deref when sending BUSY before target_init_cmd()
       [not found] ` <20260723095120.GF110966@unreal>
@ 2026-07-24  2:42   ` kensanya
  2026-07-26  7:55     ` Leon Romanovsky
  0 siblings, 1 reply; 5+ messages in thread
From: kensanya @ 2026-07-24  2:42 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: bvanassche, jgg, linux-rdma, target-devel, linux-kernel, TanZheng

At 2026-07-23 17:51:20, "Leon Romanovsky" <leon@kernel.org> wrote:
>On Thu, Jul 23, 2026 at 10:44:56AM +0800, kensanya@163.com wrote:
>> 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.
>> 
>> target_send_busy() is only valid after a successful target_init_cmd().
>> If target_init_cmd() itself fails, free the send ioctx tag locally
>> with target_free_tag() instead of calling target_send_busy().
>> 
>> Fixes: 8b8807b9e982 ("scsi: RDMA/srpt: Fix handling of command / TMF 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>
>> ---
>> v2:
>> - On target_init_cmd() failure, use target_free_tag() instead of
>>   target_send_busy()
>> 
>>  drivers/infiniband/ulp/srpt/ib_srpt.c | 36 +++++++++++++++++++--------
>>  1 file changed, 26 insertions(+), 10 deletions(-)
>> 
>> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
>> index f66cfd70c263..71af1fb0d380 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,25 @@ static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
>>  		break;
>>  	}
>>  
>> +	/*
>> +	 * Call target_init_cmd() before srpt_get_desc_tbl() so that
>> +	 * cmd->se_tfo is set if descriptor parsing fails and
>> +	 * target_send_busy() is used. data_length is filled in after a
>> +	 * successful srpt_get_desc_tbl().
>> +	 *
>> +	 * target_send_busy() is only valid after a successful
>> +	 * target_init_cmd(). On init failure free the tag locally.
>> +	 */
>> +	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);
>
>1. TCM_SIMPLE_TAG -> you need to rebase to latest rdma-next.

      I will modify and resend the patch.

>2. You are hardcoding dir, while it can be overwritten in srpt_get_desc_tbl().

      In the srpt_handle_cmd() function:
      &send_ioctx->cmd;
      srpt_get_desc_tbl(..., send_ioctx, ...) 
           ioctx->cmd.data_direction = *dir;

     Therefore, after successfully calling srpt_get_desc_tbl(), even if target_init_cmd() is
     passed DMA_NONE, cmd->data_direction should already have stored the actual direction.

     Do you mean  that after calling srpt_get_desc_tbl(), displaying the overwritten content
     would be better?I want to make sure I address the intent of your comment correctly.
>
>Thanks

Thanks,
TanZheng

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Re: [PATCH v2] RDMA/srpt: fix NULL deref when sending BUSY before target_init_cmd()
  2026-07-24  2:42   ` kensanya
@ 2026-07-26  7:55     ` Leon Romanovsky
  2026-07-27  6:39       ` kensanya
  0 siblings, 1 reply; 5+ messages in thread
From: Leon Romanovsky @ 2026-07-26  7:55 UTC (permalink / raw)
  To: kensanya
  Cc: bvanassche, jgg, linux-rdma, target-devel, linux-kernel, TanZheng

On Fri, Jul 24, 2026 at 10:42:53AM +0800, kensanya wrote:
> At 2026-07-23 17:51:20, "Leon Romanovsky" <leon@kernel.org> wrote:
> >On Thu, Jul 23, 2026 at 10:44:56AM +0800, kensanya@163.com wrote:
> >> 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.
> >> 
> >> target_send_busy() is only valid after a successful target_init_cmd().
> >> If target_init_cmd() itself fails, free the send ioctx tag locally
> >> with target_free_tag() instead of calling target_send_busy().
> >> 
> >> Fixes: 8b8807b9e982 ("scsi: RDMA/srpt: Fix handling of command / TMF 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>
> >> ---
> >> v2:
> >> - On target_init_cmd() failure, use target_free_tag() instead of
> >>   target_send_busy()
> >> 
> >>  drivers/infiniband/ulp/srpt/ib_srpt.c | 36 +++++++++++++++++++--------
> >>  1 file changed, 26 insertions(+), 10 deletions(-)
> >> 
> >> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> >> index f66cfd70c263..71af1fb0d380 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,25 @@ static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
> >>  		break;
> >>  	}
> >>  
> >> +	/*
> >> +	 * Call target_init_cmd() before srpt_get_desc_tbl() so that
> >> +	 * cmd->se_tfo is set if descriptor parsing fails and
> >> +	 * target_send_busy() is used. data_length is filled in after a
> >> +	 * successful srpt_get_desc_tbl().
> >> +	 *
> >> +	 * target_send_busy() is only valid after a successful
> >> +	 * target_init_cmd(). On init failure free the tag locally.
> >> +	 */
> >> +	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);
> >
> >1. TCM_SIMPLE_TAG -> you need to rebase to latest rdma-next.
> 
>       I will modify and resend the patch.
> 
> >2. You are hardcoding dir, while it can be overwritten in srpt_get_desc_tbl().
> 
>       In the srpt_handle_cmd() function:
>       &send_ioctx->cmd;
>       srpt_get_desc_tbl(..., send_ioctx, ...) 
>            ioctx->cmd.data_direction = *dir;
> 
>      Therefore, after successfully calling srpt_get_desc_tbl(), even if target_init_cmd() is
>      passed DMA_NONE, cmd->data_direction should already have stored the actual direction.
> 
>      Do you mean  that after calling srpt_get_desc_tbl(), displaying the overwritten content
>      would be better?I want to make sure I address the intent of your comment correctly.

You need to do this correctly: target_init_cmd() must receive the
correct directory, as calculated by srpt_get_desc_tbl().

Thanks

> >
> >Thanks
> 
> Thanks,
> TanZheng

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re:Re: Re: [PATCH v2] RDMA/srpt: fix NULL deref when sending BUSY before target_init_cmd()
  2026-07-26  7:55     ` Leon Romanovsky
@ 2026-07-27  6:39       ` kensanya
  2026-07-27  9:00         ` Leon Romanovsky
  0 siblings, 1 reply; 5+ messages in thread
From: kensanya @ 2026-07-27  6:39 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: bvanassche, jgg, linux-rdma, target-devel, linux-kernel, TanZheng



At 2026-07-26 15:55:11, "Leon Romanovsky" <leon@kernel.org> wrote:
>On Fri, Jul 24, 2026 at 10:42:53AM +0800, kensanya wrote:
>> At 2026-07-23 17:51:20, "Leon Romanovsky" <leon@kernel.org> wrote:
>> >On Thu, Jul 23, 2026 at 10:44:56AM +0800, kensanya@163.com wrote:
>> >> 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.
>> >> 
>> >> target_send_busy() is only valid after a successful target_init_cmd().
>> >> If target_init_cmd() itself fails, free the send ioctx tag locally
>> >> with target_free_tag() instead of calling target_send_busy().
>> >> 
>> >> Fixes: 8b8807b9e982 ("scsi: RDMA/srpt: Fix handling of command / TMF 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>
>> >> ---
>> >> v2:
>> >> - On target_init_cmd() failure, use target_free_tag() instead of
>> >>   target_send_busy()
>> >> 
>> >>  drivers/infiniband/ulp/srpt/ib_srpt.c | 36 +++++++++++++++++++--------
>> >>  1 file changed, 26 insertions(+), 10 deletions(-)
>> >> 
>> >> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
>> >> index f66cfd70c263..71af1fb0d380 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,25 @@ static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
>> >>  		break;
>> >>  	}
>> >>  
>> >> +	/*
>> >> +	 * Call target_init_cmd() before srpt_get_desc_tbl() so that
>> >> +	 * cmd->se_tfo is set if descriptor parsing fails and
>> >> +	 * target_send_busy() is used. data_length is filled in after a
>> >> +	 * successful srpt_get_desc_tbl().
>> >> +	 *
>> >> +	 * target_send_busy() is only valid after a successful
>> >> +	 * target_init_cmd(). On init failure free the tag locally.
>> >> +	 */
>> >> +	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);
>> >
>> >1. TCM_SIMPLE_TAG -> you need to rebase to latest rdma-next.
>> 
>>       I will modify and resend the patch.
>> 
>> >2. You are hardcoding dir, while it can be overwritten in srpt_get_desc_tbl().
>> 
>>       In the srpt_handle_cmd() function:
>>       &send_ioctx->cmd;
>>       srpt_get_desc_tbl(..., send_ioctx, ...) 
>>            ioctx->cmd.data_direction = *dir;
>> 
>>      Therefore, after successfully calling srpt_get_desc_tbl(), even if target_init_cmd() is
>>      passed DMA_NONE, cmd->data_direction should already have stored the actual direction.
>> 
>>      Do you mean  that after calling srpt_get_desc_tbl(), displaying the overwritten content
>>      would be better?I want to make sure I address the intent of your comment correctly.
>
>You need to do this correctly: target_init_cmd() must receive the
>correct directory, as calculated by srpt_get_desc_tbl().
>
>Thanks
>
>> >
>> >Thanks
>> 
>> Thanks,
>> TanZheng

There is a contradiction between the two suggestions:
- Bart suggests calling target_init_cmd() before calling
   srpt_get_desc_tbl() (using a placeholder) to ensure that
   target_send_busy() can be safely executed if the
   descriptor parsing fails.
- Leon points out that target_init_cmd() must receive the
  actual data direction calculated by srpt_get_desc_tbl(),
  rather than a placeholder like DMA_NONE that will
  be overwritten later.

If we continue to modify the current patch, the only way I
can think of is to extract the direction parsing logic from
srpt_get_desc_tbl and place it before target_init_cmd, so
that target_init_cmd can receive the correct direction.
How about this?

Thanks,
TanZheng

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Re: Re: [PATCH v2] RDMA/srpt: fix NULL deref when sending BUSY before target_init_cmd()
  2026-07-27  6:39       ` kensanya
@ 2026-07-27  9:00         ` Leon Romanovsky
  0 siblings, 0 replies; 5+ messages in thread
From: Leon Romanovsky @ 2026-07-27  9:00 UTC (permalink / raw)
  To: kensanya
  Cc: bvanassche, jgg, linux-rdma, target-devel, linux-kernel, TanZheng

On Mon, Jul 27, 2026 at 02:39:08PM +0800, kensanya wrote:
> 
> 
> At 2026-07-26 15:55:11, "Leon Romanovsky" <leon@kernel.org> wrote:
> >On Fri, Jul 24, 2026 at 10:42:53AM +0800, kensanya wrote:
> >> At 2026-07-23 17:51:20, "Leon Romanovsky" <leon@kernel.org> wrote:
> >> >On Thu, Jul 23, 2026 at 10:44:56AM +0800, kensanya@163.com wrote:
> >> >> 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.
> >> >> 
> >> >> target_send_busy() is only valid after a successful target_init_cmd().
> >> >> If target_init_cmd() itself fails, free the send ioctx tag locally
> >> >> with target_free_tag() instead of calling target_send_busy().
> >> >> 
> >> >> Fixes: 8b8807b9e982 ("scsi: RDMA/srpt: Fix handling of command / TMF 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>
> >> >> ---
> >> >> v2:
> >> >> - On target_init_cmd() failure, use target_free_tag() instead of
> >> >>   target_send_busy()
> >> >> 
> >> >>  drivers/infiniband/ulp/srpt/ib_srpt.c | 36 +++++++++++++++++++--------
> >> >>  1 file changed, 26 insertions(+), 10 deletions(-)
> >> >> 
> >> >> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> >> >> index f66cfd70c263..71af1fb0d380 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,25 @@ static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
> >> >>  		break;
> >> >>  	}
> >> >>  
> >> >> +	/*
> >> >> +	 * Call target_init_cmd() before srpt_get_desc_tbl() so that
> >> >> +	 * cmd->se_tfo is set if descriptor parsing fails and
> >> >> +	 * target_send_busy() is used. data_length is filled in after a
> >> >> +	 * successful srpt_get_desc_tbl().
> >> >> +	 *
> >> >> +	 * target_send_busy() is only valid after a successful
> >> >> +	 * target_init_cmd(). On init failure free the tag locally.
> >> >> +	 */
> >> >> +	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);
> >> >
> >> >1. TCM_SIMPLE_TAG -> you need to rebase to latest rdma-next.
> >> 
> >>       I will modify and resend the patch.
> >> 
> >> >2. You are hardcoding dir, while it can be overwritten in srpt_get_desc_tbl().
> >> 
> >>       In the srpt_handle_cmd() function:
> >>       &send_ioctx->cmd;
> >>       srpt_get_desc_tbl(..., send_ioctx, ...) 
> >>            ioctx->cmd.data_direction = *dir;
> >> 
> >>      Therefore, after successfully calling srpt_get_desc_tbl(), even if target_init_cmd() is
> >>      passed DMA_NONE, cmd->data_direction should already have stored the actual direction.
> >> 
> >>      Do you mean  that after calling srpt_get_desc_tbl(), displaying the overwritten content
> >>      would be better?I want to make sure I address the intent of your comment correctly.
> >
> >You need to do this correctly: target_init_cmd() must receive the
> >correct directory, as calculated by srpt_get_desc_tbl().
> >
> >Thanks
> >
> >> >
> >> >Thanks
> >> 
> >> Thanks,
> >> TanZheng
> 
> There is a contradiction between the two suggestions:
> - Bart suggests calling target_init_cmd() before calling
>    srpt_get_desc_tbl() (using a placeholder) to ensure that
>    target_send_busy() can be safely executed if the
>    descriptor parsing fails.
> - Leon points out that target_init_cmd() must receive the
>   actual data direction calculated by srpt_get_desc_tbl(),
>   rather than a placeholder like DMA_NONE that will
>   be overwritten later.
> 
> If we continue to modify the current patch, the only way I
> can think of is to extract the direction parsing logic from
> srpt_get_desc_tbl and place it before target_init_cmd, so
> that target_init_cmd can receive the correct direction.
> How about this?

Say it to your AI tool that it is not contradiction, but wrongly
implemented Bart's suggestion.

Thanks

> 
> Thanks,
> TanZheng

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-07-27  9:00 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-23  2:44 [PATCH v2] RDMA/srpt: fix NULL deref when sending BUSY before target_init_cmd() kensanya
     [not found] ` <20260723095120.GF110966@unreal>
2026-07-24  2:42   ` kensanya
2026-07-26  7:55     ` Leon Romanovsky
2026-07-27  6:39       ` kensanya
2026-07-27  9:00         ` Leon Romanovsky

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox