diff mbox

[06/15] IB/srpt: Simplify srpt_handle_tsk_mgmt()

Message ID 568BD1D2.1030609@sandisk.com (mailing list archive)
State Superseded
Headers show

Commit Message

Bart Van Assche Jan. 5, 2016, 2:23 p.m. UTC 
Let the target core check task existence instead of the SRP target
driver.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 54 ++---------------------------------
 1 file changed, 2 insertions(+), 52 deletions(-)

Comments

Christoph Hellwig Jan. 6, 2016, 5 a.m. UTC | #1 
On Tue, Jan 05, 2016 at 03:23:14PM +0100, Bart Van Assche wrote:
> Let the target core check task existence instead of the SRP target
> driver.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sagi Grimberg Jan. 6, 2016, 2:02 p.m. UTC | #2 
Reviewed-by: Sagi Grimberg <sagig@mellanox.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Estrin, Alex Jan. 26, 2016, 4:57 p.m. UTC | #3 
Hi Doug, Bart,

Is it possible to expedite this patch as it actually fixes a real bug in ib_srpt?

If srp target receives ABORT TASK, it will crash the kernel while trying to respond.
We were able to reproduce it under quite heavy load:

[78395.442496] BUG: unable to handle kernel NULL pointer dereference at
0000000000000001
[78395.442534] IP: [<ffffffffa0565f37>] srpt_handle_new_iu+0x6d7/0x790
[ib_srpt]
[78395.442564] PGD 0
[78395.442574] Oops: 0002 [#1] SMP
....
[78395.443443] Call Trace:
[78395.443457]  [<ffffffffa05660ce>] srpt_process_completion+0xde/0x570
[ib_srpt]
[78395.443484]  [<ffffffffa056669f>] srpt_compl_thread+0x13f/0x160 [ib_srpt]
[78395.444406]  [<ffffffff81098230>] ? wake_up_bit+0x30/0x30
[78395.445307]  [<ffffffffa0566560>] ? srpt_process_completion+0x570/0x570
[ib_srpt]
[78395.446218]  [<ffffffff8109726f>] kthread+0xcf/0xe0
[78395.447125]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
[78395.448033]  [<ffffffff81613cfc>] ret_from_fork+0x7c/0xb0
[78395.448927]  [<ffffffff810971a0>] ? kthread_create_on_node+0x140/0x140
[78395.449814] Code: 1a 01 00 00 01 74 a3 48 8b 7d a0 89 55 b8 48 89 4d c0 e8
fd 20 00 00 8b 55 b8 48 8b 4d c0 eb 8a 0f 1f 40 00 49 8b 85 f8 00 00 00 <c6> 40
01 01 e9 44 fd ff ff 49 8b b
4 24 b8 04 00 00 49 8d 94 24
[78395.451693] RIP  [<ffffffffa0565f37>] srpt_handle_new_iu+0x6d7/0x790
[ib_srpt]
[78395.452603]  RSP <ffff88083e80fd70>
[78395.453498] CR2: 0000000000000001

Trace was obtained on RHEL7.1 distro, but could happened on any kernel,
so I believe this patch should go to stable as well.
Please see below the hit scenario(fixed by this patch).

Fixes: 3e4f574857ee ("ib_srpt: Convert TMR path to target_submit_tmr")
Tested-by: Alex Estrin <alex.estrin@intel.com>



> -----Original Message-----

> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-owner@vger.kernel.org] On

> Behalf Of Bart Van Assche

> Sent: Tuesday, January 05, 2016 9:23 AM

> To: Doug Ledford

> Cc: Christoph Hellwig; linux-rdma@vger.kernel.org

> Subject: [PATCH 06/15] IB/srpt: Simplify srpt_handle_tsk_mgmt()

> 

> Let the target core check task existence instead of the SRP target

> driver.

> 

> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>

> Cc: Christoph Hellwig <hch@lst.de>

> ---

>  drivers/infiniband/ulp/srpt/ib_srpt.c | 54 ++---------------------------------

>  1 file changed, 2 insertions(+), 52 deletions(-)

> 

> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c

> b/drivers/infiniband/ulp/srpt/ib_srpt.c

> index fc19203..9cb1a14 100644

> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c

> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c

> @@ -1554,47 +1554,6 @@ send_sense:

>  	return -1;

>  }

> 

> -/**

> - * srpt_rx_mgmt_fn_tag() - Process a task management function by tag.

> - * @ch: RDMA channel of the task management request.

> - * @fn: Task management function to perform.

> - * @req_tag: Tag of the SRP task management request.

> - * @mgmt_ioctx: I/O context of the task management request.

> - *

> - * Returns zero if the target core will process the task management

> - * request asynchronously.

> - *

> - * Note: It is assumed that the initiator serializes tag-based task management

> - * requests.

> - */

> -static int srpt_rx_mgmt_fn_tag(struct srpt_send_ioctx *ioctx, u64 tag)

> -{

> -	struct srpt_device *sdev;

> -	struct srpt_rdma_ch *ch;

> -	struct srpt_send_ioctx *target;

> -	int ret, i;

> -

> -	ret = -EINVAL;

> -	ch = ioctx->ch;

> -	BUG_ON(!ch);

> -	BUG_ON(!ch->sport);

> -	sdev = ch->sport->sdev;

> -	BUG_ON(!sdev);

> -	spin_lock_irq(&sdev->spinlock);

> -	for (i = 0; i < ch->rq_size; ++i) {

> -		target = ch->ioctx_ring[i];

> -		if (target->cmd.se_lun == ioctx->cmd.se_lun &&

> -		    target->cmd.tag == tag &&

> -		    srpt_get_cmd_state(target) != SRPT_STATE_DONE) {

> -			ret = 0;

> -			/* now let the target core abort &target->cmd; */

> -			break;

> -		}

> -	}

> -	spin_unlock_irq(&sdev->spinlock);

> -	return ret;

> -}

> -

>  static int srp_tmr_to_tcm(int fn)

>  {

>  	switch (fn) {

> @@ -1628,7 +1587,6 @@ static void srpt_handle_tsk_mgmt(struct srpt_rdma_ch *ch,

>  	struct srp_tsk_mgmt *srp_tsk;

>  	struct se_cmd *cmd;

>  	struct se_session *sess = ch->sess;

> -	uint32_t tag = 0;

>  	int tcm_tmr;

>  	int rc;

> 

> @@ -1649,18 +1607,10 @@ static void srpt_handle_tsk_mgmt(struct srpt_rdma_ch *ch,

>  			TMR_TASK_MGMT_FUNCTION_NOT_SUPPORTED;

>  		goto fail;

>  	}

> -	if (srp_tsk->tsk_mgmt_func == SRP_TSK_ABORT_TASK) {

> -		rc = srpt_rx_mgmt_fn_tag(send_ioctx, srp_tsk->task_tag);

> -		if (rc < 0) {

> -			send_ioctx->cmd.se_tmr_req->response =

                                    ^^^^^^^^^^^^^^^^^^^^   !!!
se_tmr_req pointer was cleared a few lines of code earlier:

srpt_handle_new_iu() {
	.....
	send_ioctx = srpt_get_send_ioctx(ch) {
				.....
				memset(&ioctx->cmd, 0 ... !!! re-init se_cmd structure for response.
			}
	...
	srpt_handle_tsk_mgmt(ch, recv_ioctx, send_ioctx) {
	...
	And here we got it !!!
		send_ioctx->cmd.se_tmr_req->response =   
					TMR_TASK_DOES_NOT_EXIST;

> -			goto fail;

> -		}

> -		tag = srp_tsk->task_tag;

> -	}

>  	rc = target_submit_tmr(&send_ioctx->cmd, sess, NULL,

>  			       scsilun_to_int(&srp_tsk->lun), srp_tsk, tcm_tmr,

> -			       GFP_KERNEL, tag, TARGET_SCF_ACK_KREF);

> +			       GFP_KERNEL, srp_tsk->task_tag,

> +			       TARGET_SCF_ACK_KREF);

>  	if (rc != 0) {

>  		send_ioctx->cmd.se_tmr_req->response = TMR_FUNCTION_REJECTED;

>  		goto fail;

> --

> 2.1.4

> 

> --

> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in

> the body of a message to majordomo@vger.kernel.org

> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Van Assche Jan. 26, 2016, 5:32 p.m. UTC | #4 
On 01/26/16 08:57, Estrin, Alex wrote:
> Is it possible to expedite this patch as it actually fixes a real bug in ib_srpt?
>
> If srp target receives ABORT TASK, it will crash the kernel while trying to respond.
> We were able to reproduce it under quite heavy load [ ... ]

Hello Alex,

Thank you for the feedback. Since the kernel 4.5 merge has been closed I 
will retest and resend the whole patch series. I would like to see all 
of these patches to be integrated in the upstream kernel instead of only 
this single patch. I will Cc stable and I will also add your Tested-by tag.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Estrin, Alex Jan. 26, 2016, 6:35 p.m. UTC | #5 
> 

> Thank you for the feedback. Since the kernel 4.5 merge has been closed I

> will retest and resend the whole patch series. 


Bart,

In this case I would suggest to extend the patch by removing
this unlikely check, since it is also taken care of in target:

@@ -1843,25 +1801,10 @@ static void srpt_handle_tsk_mgmt(struct srpt_rdma_ch *ch,
        srpt_set_cmd_state(send_ioctx, SRPT_STATE_MGMT);
        send_ioctx->tag = srp_tsk->tag;
        tcm_tmr = srp_tmr_to_tcm(srp_tsk->tsk_mgmt_func);
-       if (tcm_tmr < 0) {
-               send_ioctx->cmd.se_tmr_req->response =
-                       TMR_TASK_MGMT_FUNCTION_NOT_SUPPORTED;
-               goto fail;
-       }

Thanks,
Alex.
diff mbox

Patch

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index fc19203..9cb1a14 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1554,47 +1554,6 @@  send_sense:
 	return -1;
 }
 
-/**
- * srpt_rx_mgmt_fn_tag() - Process a task management function by tag.
- * @ch: RDMA channel of the task management request.
- * @fn: Task management function to perform.
- * @req_tag: Tag of the SRP task management request.
- * @mgmt_ioctx: I/O context of the task management request.
- *
- * Returns zero if the target core will process the task management
- * request asynchronously.
- *
- * Note: It is assumed that the initiator serializes tag-based task management
- * requests.
- */
-static int srpt_rx_mgmt_fn_tag(struct srpt_send_ioctx *ioctx, u64 tag)
-{
-	struct srpt_device *sdev;
-	struct srpt_rdma_ch *ch;
-	struct srpt_send_ioctx *target;
-	int ret, i;
-
-	ret = -EINVAL;
-	ch = ioctx->ch;
-	BUG_ON(!ch);
-	BUG_ON(!ch->sport);
-	sdev = ch->sport->sdev;
-	BUG_ON(!sdev);
-	spin_lock_irq(&sdev->spinlock);
-	for (i = 0; i < ch->rq_size; ++i) {
-		target = ch->ioctx_ring[i];
-		if (target->cmd.se_lun == ioctx->cmd.se_lun &&
-		    target->cmd.tag == tag &&
-		    srpt_get_cmd_state(target) != SRPT_STATE_DONE) {
-			ret = 0;
-			/* now let the target core abort &target->cmd; */
-			break;
-		}
-	}
-	spin_unlock_irq(&sdev->spinlock);
-	return ret;
-}
-
 static int srp_tmr_to_tcm(int fn)
 {
 	switch (fn) {
@@ -1628,7 +1587,6 @@  static void srpt_handle_tsk_mgmt(struct srpt_rdma_ch *ch,
 	struct srp_tsk_mgmt *srp_tsk;
 	struct se_cmd *cmd;
 	struct se_session *sess = ch->sess;
-	uint32_t tag = 0;
 	int tcm_tmr;
 	int rc;
 
@@ -1649,18 +1607,10 @@  static void srpt_handle_tsk_mgmt(struct srpt_rdma_ch *ch,
 			TMR_TASK_MGMT_FUNCTION_NOT_SUPPORTED;
 		goto fail;
 	}
-	if (srp_tsk->tsk_mgmt_func == SRP_TSK_ABORT_TASK) {
-		rc = srpt_rx_mgmt_fn_tag(send_ioctx, srp_tsk->task_tag);
-		if (rc < 0) {
-			send_ioctx->cmd.se_tmr_req->response =
-					TMR_TASK_DOES_NOT_EXIST;
-			goto fail;
-		}
-		tag = srp_tsk->task_tag;
-	}
 	rc = target_submit_tmr(&send_ioctx->cmd, sess, NULL,
 			       scsilun_to_int(&srp_tsk->lun), srp_tsk, tcm_tmr,
-			       GFP_KERNEL, tag, TARGET_SCF_ACK_KREF);
+			       GFP_KERNEL, srp_tsk->task_tag,
+			       TARGET_SCF_ACK_KREF);
 	if (rc != 0) {
 		send_ioctx->cmd.se_tmr_req->response = TMR_FUNCTION_REJECTED;
 		goto fail;