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

["Estrin, Alex" <alex.estrin-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

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