[PATCH] ib_srpt: Add missing ioctx->buf + ->dma alloc_session_cb init

[PATCH] ib_srpt: Add missing ioctx->buf + ->dma alloc_session_cb init

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch addresses the v4.6-rc1 breakage for ib_srpt, and
adds the missing srpt_alloc_session_cb() to setup existing
srpt_ioctx buf + dma resources for srpt_send_ioctx->ioctx.

Also, add a srpt_free_sess_cmd_map_res() to do the clean
during shutdown in srpt_release_channel_work(), and drop
the now left-over ch->ioctx_ring[].

Reported-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Doug Ledford <dledford@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 79 +++++++++++++++++++++++++++--------
 drivers/infiniband/ulp/srpt/ib_srpt.h |  2 -
 2 files changed, 61 insertions(+), 20 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 0bd3cb2..c4e0a0a 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1266,7 +1266,9 @@ static struct srpt_send_ioctx *srpt_get_send_ioctx(struct srpt_rdma_ch *ch)
 {
 	struct se_session *se_sess;
 	struct srpt_send_ioctx *ioctx;
-	int tag;
+	void *buf;
+	dma_addr_t dma;
+	int tag, index;
 
 	BUG_ON(!ch);
 	se_sess = ch->sess;
@@ -1277,12 +1279,19 @@ static struct srpt_send_ioctx *srpt_get_send_ioctx(struct srpt_rdma_ch *ch)
 		return NULL;
 	}
 	ioctx = &((struct srpt_send_ioctx *)se_sess->sess_cmd_map)[tag];
+	buf = ioctx->ioctx.buf;
+	dma = ioctx->ioctx.dma;
+	index = ioctx->ioctx.index;
+
 	memset(ioctx, 0, sizeof(struct srpt_send_ioctx));
 	ioctx->ch = ch;
 	spin_lock_init(&ioctx->spinlock);
 	ioctx->state = SRPT_STATE_NEW;
 	init_completion(&ioctx->tx_done);
 
+	ioctx->ioctx.buf = buf;
+	ioctx->ioctx.dma = dma;
+	ioctx->ioctx.index = index;
 	ioctx->cmd.map_tag = tag;
 
 	return ioctx;
@@ -1961,6 +1970,24 @@ static void srpt_free_ch(struct kref *kref)
 	kfree(ch);
 }
 
+static void srpt_free_sess_cmd_map_res(struct se_session *se_sess)
+{
+	struct srpt_rdma_ch *ch = se_sess->fabric_sess_ptr;
+	struct srpt_device *sdev = ch->sport->sdev;
+	struct srpt_send_ioctx *ioctx;
+	u32 i, dma_size = ch->rsp_size;
+
+	for (i = 0; i < ch->rq_size; i++) {
+		ioctx = &((struct srpt_send_ioctx *)se_sess->sess_cmd_map)[i];
+
+		if (!ib_dma_mapping_error(sdev->device, ioctx->ioctx.dma))
+			ib_dma_unmap_single(sdev->device, ioctx->ioctx.dma,
+					    dma_size, DMA_TO_DEVICE);
+		if (ioctx->ioctx.buf)
+			kfree(ioctx->ioctx.buf);
+	}
+}
+
 static void srpt_release_channel_work(struct work_struct *w)
 {
 	struct srpt_rdma_ch *ch;
@@ -1981,6 +2008,7 @@ static void srpt_release_channel_work(struct work_struct *w)
 	target_wait_for_sess_cmds(se_sess);
 
 	transport_deregister_session_configfs(se_sess);
+	srpt_free_sess_cmd_map_res(se_sess);
 	transport_deregister_session(se_sess);
 	ch->sess = NULL;
 
@@ -1988,10 +2016,6 @@ static void srpt_release_channel_work(struct work_struct *w)
 
 	srpt_destroy_ch_ib(ch);
 
-	srpt_free_ioctx_ring((struct srpt_ioctx **)ch->ioctx_ring,
-			     ch->sport->sdev, ch->rq_size,
-			     ch->rsp_size, DMA_TO_DEVICE);
-
 	mutex_lock(&sdev->mutex);
 	list_del_init(&ch->list);
 	if (ch->release_done)
@@ -2003,6 +2027,35 @@ static void srpt_release_channel_work(struct work_struct *w)
 	kref_put(&ch->kref, srpt_free_ch);
 }
 
+static int srpt_alloc_session_cb(struct se_portal_group *se_tpg,
+				 struct se_session *se_sess, void *p)
+{
+	struct srpt_rdma_ch *ch = p;
+	struct srpt_device *sdev = ch->sport->sdev;
+	struct srpt_send_ioctx *ioctx;
+	u32 i, dma_size = ch->rsp_size;
+
+	for (i = 0; i < ch->rq_size; i++) {
+		ioctx = &((struct srpt_send_ioctx *)se_sess->sess_cmd_map)[i];
+		ioctx->ioctx.index = i;
+
+		ioctx->ioctx.buf = kmalloc(dma_size, GFP_KERNEL);
+		if (!ioctx->ioctx.buf)
+			goto err_free_res;
+
+		ioctx->ioctx.dma = ib_dma_map_single(sdev->device, ioctx->ioctx.buf,
+						     dma_size, DMA_TO_DEVICE);
+		if (ib_dma_mapping_error(sdev->device, ioctx->ioctx.dma))
+			goto err_free_res;
+	}
+
+	return 0;
+
+err_free_res:
+	srpt_free_sess_cmd_map_res(se_sess);
+	return -ENOMEM;
+}
+
 /**
  * srpt_cm_req_recv() - Process the event IB_CM_REQ_RECEIVED.
  *
@@ -2136,20 +2189,13 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 	INIT_LIST_HEAD(&ch->cmd_wait_list);
 	ch->rsp_size = ch->sport->port_attrib.srp_max_rsp_size;
 
-	ch->ioctx_ring = (struct srpt_send_ioctx **)
-		srpt_alloc_ioctx_ring(ch->sport->sdev, ch->rq_size,
-				      sizeof(*ch->ioctx_ring[0]),
-				      ch->rsp_size, DMA_TO_DEVICE);
-	if (!ch->ioctx_ring)
-		goto free_ch;
-
 	ret = srpt_create_ch_ib(ch);
 	if (ret) {
 		rej->reason = cpu_to_be32(
 			      SRP_LOGIN_REJ_INSUFFICIENT_RESOURCES);
 		pr_err("rejected SRP_LOGIN_REQ because creating"
 		       " a new RDMA channel failed.\n");
-		goto free_ring;
+		goto free_ch;
 	}
 
 	ret = srpt_ch_qp_rtr(ch, ch->qp);
@@ -2175,7 +2221,8 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 try_again:
 	ch->sess = target_alloc_session(&sport->port_tpg_1, ch->rq_size,
 					sizeof(struct srpt_send_ioctx),
-					TARGET_PROT_NORMAL, p, ch, NULL);
+					TARGET_PROT_NORMAL, p, ch,
+					srpt_alloc_session_cb);
 	if (IS_ERR(ch->sess)) {
 		pr_info("Rejected login because no ACL has been"
 			" configured yet for initiator %s.\n", p);
@@ -2240,10 +2287,6 @@ release_channel:
 destroy_ib:
 	srpt_destroy_ch_ib(ch);
 
-free_ring:
-	srpt_free_ioctx_ring((struct srpt_ioctx **)ch->ioctx_ring,
-			     ch->sport->sdev, ch->rq_size,
-			     ch->rsp_size, DMA_TO_DEVICE);
 free_ch:
 	kfree(ch);
 
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h
index ca288f0..a0e3403 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.h
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.h
@@ -251,7 +251,6 @@ enum rdma_ch_state {
  * @spinlock:      Protects free_list and state.
  * @free_list:     Head of list with free send I/O contexts.
  * @state:         channel state. See also enum rdma_ch_state.
- * @ioctx_ring:    Send ring.
  * @list:          Node for insertion in the srpt_device.rch_list list.
  * @cmd_wait_list: List of SCSI commands that arrived before the RTU event. This
  *                 list contains struct srpt_ioctx elements and is protected
@@ -279,7 +278,6 @@ struct srpt_rdma_ch {
 	spinlock_t		spinlock;
 	struct list_head	free_list;
 	enum rdma_ch_state	state;
-	struct srpt_send_ioctx	**ioctx_ring;
 	struct list_head	list;
 	struct list_head	cmd_wait_list;
 	struct se_session	*sess;
-- 
1.9.1

Re: [PATCH] ib_srpt: Add missing ioctx->buf + ->dma alloc_session_cb init

[kbuild test robot]

Hi Nicholas,

[auto build test WARNING on v4.6-rc2]
[also build test WARNING on next-20160404]
[cannot apply to rdma/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Nicholas-A-Bellinger/ib_srpt-Add-missing-ioctx-buf-dma-alloc_session_cb-init/20160405-023227


coccinelle warnings: (new ones prefixed by >>)

>> drivers/infiniband/ulp/srpt/ib_srpt.c:1987:3-8: WARNING: NULL check before freeing functions like kfree, debugfs_remove, debugfs_remove_recursive or usb_free_urb is not needed. Maybe consider reorganizing relevant code to avoid passing NULL values.

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH] ib_srpt: Add missing ioctx->buf + ->dma alloc_session_cb init

[Nicholas A. Bellinger]

On Tue, 2016-04-05 at 03:11 +0800, kbuild test robot wrote:
> Hi Nicholas,
> 
> [auto build test WARNING on v4.6-rc2]
> [also build test WARNING on next-20160404]
> [cannot apply to rdma/master]
> [if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Nicholas-A-Bellinger/ib_srpt-Add-missing-ioctx-buf-dma-alloc_session_cb-init/20160405-023227
> 
> 
> coccinelle warnings: (new ones prefixed by >>)
> 
> >> drivers/infiniband/ulp/srpt/ib_srpt.c:1987:3-8: WARNING: NULL check
> before freeing functions like kfree, debugfs_remove,
> debugfs_remove_recursive or usb_free_urb is not needed. Maybe consider
> reorganizing relevant code to avoid passing NULL values.
> 
> Please review and possibly fold the followup patch.
> 

Folding into the original patch.

Thanks Fengguang!

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ib_srpt: Add missing ioctx->buf + ->dma alloc_session_cb init

[Bart Van Assche]

On 04/04/2016 11:28 AM, Nicholas A. Bellinger wrote:
> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index 0bd3cb2..c4e0a0a 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> @@ -1266,7 +1266,9 @@ static struct srpt_send_ioctx *srpt_get_send_ioctx(struct srpt_rdma_ch *ch)
>   {
>   	struct se_session *se_sess;
>   	struct srpt_send_ioctx *ioctx;
> -	int tag;
> +	void *buf;
> +	dma_addr_t dma;
> +	int tag, index;
>
>   	BUG_ON(!ch);
>   	se_sess = ch->sess;
> @@ -1277,12 +1279,19 @@ static struct srpt_send_ioctx *srpt_get_send_ioctx(struct srpt_rdma_ch *ch)
>   		return NULL;
>   	}
>   	ioctx = &((struct srpt_send_ioctx *)se_sess->sess_cmd_map)[tag];
> +	buf = ioctx->ioctx.buf;
> +	dma = ioctx->ioctx.dma;
> +	index = ioctx->ioctx.index;
> +
>   	memset(ioctx, 0, sizeof(struct srpt_send_ioctx));
>   	ioctx->ch = ch;
>   	spin_lock_init(&ioctx->spinlock);
>   	ioctx->state = SRPT_STATE_NEW;
>   	init_completion(&ioctx->tx_done);
>
> +	ioctx->ioctx.buf = buf;
> +	ioctx->ioctx.dma = dma;
> +	ioctx->ioctx.index = index;
>   	ioctx->cmd.map_tag = tag;
>
>   	return ioctx;
> @@ -1961,6 +1970,24 @@ static void srpt_free_ch(struct kref *kref)
>   	kfree(ch);
>   }

These assignments should happen once, namely just after I/O context 
allocation, instead of performing these assignments during every 
srpt_get_send_ioctx() call.

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ib_srpt: Add missing ioctx->buf + ->dma alloc_session_cb init

[Nicholas A. Bellinger]

On Mon, 2016-04-04 at 11:40 -0700, Bart Van Assche wrote:
> On 04/04/2016 11:28 AM, Nicholas A. Bellinger wrote:
> > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > index 0bd3cb2..c4e0a0a 100644
> > --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > @@ -1266,7 +1266,9 @@ static struct srpt_send_ioctx *srpt_get_send_ioctx(struct srpt_rdma_ch *ch)
> >   {
> >   	struct se_session *se_sess;
> >   	struct srpt_send_ioctx *ioctx;
> > -	int tag;
> > +	void *buf;
> > +	dma_addr_t dma;
> > +	int tag, index;
> >
> >   	BUG_ON(!ch);
> >   	se_sess = ch->sess;
> > @@ -1277,12 +1279,19 @@ static struct srpt_send_ioctx *srpt_get_send_ioctx(struct srpt_rdma_ch *ch)
> >   		return NULL;
> >   	}
> >   	ioctx = &((struct srpt_send_ioctx *)se_sess->sess_cmd_map)[tag];
> > +	buf = ioctx->ioctx.buf;
> > +	dma = ioctx->ioctx.dma;
> > +	index = ioctx->ioctx.index;
> > +
> >   	memset(ioctx, 0, sizeof(struct srpt_send_ioctx));
> >   	ioctx->ch = ch;
> >   	spin_lock_init(&ioctx->spinlock);
> >   	ioctx->state = SRPT_STATE_NEW;
> >   	init_completion(&ioctx->tx_done);
> >
> > +	ioctx->ioctx.buf = buf;
> > +	ioctx->ioctx.dma = dma;
> > +	ioctx->ioctx.index = index;
> >   	ioctx->cmd.map_tag = tag;
> >
> >   	return ioctx;
> > @@ -1961,6 +1970,24 @@ static void srpt_free_ch(struct kref *kref)
> >   	kfree(ch);
> >   }
> 
> These assignments should happen once, namely just after I/O context 
> allocation, instead of performing these assignments during every 
> srpt_get_send_ioctx() call.
> 

No, the entire structure is being cleared each time, just like what
every other driver is doing here.

Don't try to micro-optimize to avoid doing memset.



--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ib_srpt: Add missing ioctx->buf + ->dma alloc_session_cb init

[Bart Van Assche]

On 04/04/2016 03:41 PM, Nicholas A. Bellinger wrote:
> On Mon, 2016-04-04 at 11:40 -0700, Bart Van Assche wrote:
>> On 04/04/2016 11:28 AM, Nicholas A. Bellinger wrote:
>>> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
>>> index 0bd3cb2..c4e0a0a 100644
>>> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
>>> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
>>> @@ -1266,7 +1266,9 @@ static struct srpt_send_ioctx *srpt_get_send_ioctx(struct srpt_rdma_ch *ch)
>>>    {
>>>    	struct se_session *se_sess;
>>>    	struct srpt_send_ioctx *ioctx;
>>> -	int tag;
>>> +	void *buf;
>>> +	dma_addr_t dma;
>>> +	int tag, index;
>>>
>>>    	BUG_ON(!ch);
>>>    	se_sess = ch->sess;
>>> @@ -1277,12 +1279,19 @@ static struct srpt_send_ioctx *srpt_get_send_ioctx(struct srpt_rdma_ch *ch)
>>>    		return NULL;
>>>    	}
>>>    	ioctx = &((struct srpt_send_ioctx *)se_sess->sess_cmd_map)[tag];
>>> +	buf = ioctx->ioctx.buf;
>>> +	dma = ioctx->ioctx.dma;
>>> +	index = ioctx->ioctx.index;
>>> +
>>>    	memset(ioctx, 0, sizeof(struct srpt_send_ioctx));
>>>    	ioctx->ch = ch;
>>>    	spin_lock_init(&ioctx->spinlock);
>>>    	ioctx->state = SRPT_STATE_NEW;
>>>    	init_completion(&ioctx->tx_done);
>>>
>>> +	ioctx->ioctx.buf = buf;
>>> +	ioctx->ioctx.dma = dma;
>>> +	ioctx->ioctx.index = index;
>>>    	ioctx->cmd.map_tag = tag;
>>>
>>>    	return ioctx;
>>> @@ -1961,6 +1970,24 @@ static void srpt_free_ch(struct kref *kref)
>>>    	kfree(ch);
>>>    }
>>
>> These assignments should happen once, namely just after I/O context
>> allocation, instead of performing these assignments during every
>> srpt_get_send_ioctx() call.
>
> No, the entire structure is being cleared each time, just like what
> every other driver is doing here.
>
> Don't try to micro-optimize to avoid doing memset.

But why to keep that memset(ioctx, ...) call? That memset() call is new. 
It was added during the 4.6-rc1 merge window.

Bart.

Re: [PATCH] ib_srpt: Add missing ioctx->buf + ->dma alloc_session_cb init

[Nicholas A. Bellinger]

On Mon, 2016-04-04 at 15:44 -0700, Bart Van Assche wrote:
> On 04/04/2016 03:41 PM, Nicholas A. Bellinger wrote:
> > On Mon, 2016-04-04 at 11:40 -0700, Bart Van Assche wrote:
> >> On 04/04/2016 11:28 AM, Nicholas A. Bellinger wrote:
> >>> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> >>> index 0bd3cb2..c4e0a0a 100644
> >>> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> >>> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> >>> @@ -1266,7 +1266,9 @@ static struct srpt_send_ioctx *srpt_get_send_ioctx(struct srpt_rdma_ch *ch)
> >>>    {
> >>>    	struct se_session *se_sess;
> >>>    	struct srpt_send_ioctx *ioctx;
> >>> -	int tag;
> >>> +	void *buf;
> >>> +	dma_addr_t dma;
> >>> +	int tag, index;
> >>>
> >>>    	BUG_ON(!ch);
> >>>    	se_sess = ch->sess;
> >>> @@ -1277,12 +1279,19 @@ static struct srpt_send_ioctx *srpt_get_send_ioctx(struct srpt_rdma_ch *ch)
> >>>    		return NULL;
> >>>    	}
> >>>    	ioctx = &((struct srpt_send_ioctx *)se_sess->sess_cmd_map)[tag];
> >>> +	buf = ioctx->ioctx.buf;
> >>> +	dma = ioctx->ioctx.dma;
> >>> +	index = ioctx->ioctx.index;
> >>> +
> >>>    	memset(ioctx, 0, sizeof(struct srpt_send_ioctx));
> >>>    	ioctx->ch = ch;
> >>>    	spin_lock_init(&ioctx->spinlock);
> >>>    	ioctx->state = SRPT_STATE_NEW;
> >>>    	init_completion(&ioctx->tx_done);
> >>>
> >>> +	ioctx->ioctx.buf = buf;
> >>> +	ioctx->ioctx.dma = dma;
> >>> +	ioctx->ioctx.index = index;
> >>>    	ioctx->cmd.map_tag = tag;
> >>>
> >>>    	return ioctx;
> >>> @@ -1961,6 +1970,24 @@ static void srpt_free_ch(struct kref *kref)
> >>>    	kfree(ch);
> >>>    }
> >>
> >> These assignments should happen once, namely just after I/O context
> >> allocation, instead of performing these assignments during every
> >> srpt_get_send_ioctx() call.
> >
> > No, the entire structure is being cleared each time, just like what
> > every other driver is doing here.
> >
> > Don't try to micro-optimize to avoid doing memset.
> 
> But why to keep that memset(ioctx, ...) call? That memset() call is new. 
> It was added during the 4.6-rc1 merge window.
> 

I like memset over explicit zero assignments after reuse so future
additions to srp_send_ioctx don't need extra code.

Either way, I don't really care about micro-optimizations right now.

Are you going to verify this patch..?

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ib_srpt: Add missing ioctx->buf + ->dma alloc_session_cb init

[Bart Van Assche]

On 04/04/2016 03:58 PM, Nicholas A. Bellinger wrote:
> Are you going to verify this patch..?

Hello Nic,

After that the ib_srpt driver has processed a few commands this patch 
also triggers a kernel crash (list corruption). I have a look myself 
into the percpu_ida conversion.

Bart.

Re: [PATCH] ib_srpt: Add missing ioctx->buf + ->dma alloc_session_cb init

[Nicholas A. Bellinger]

On Wed, 2016-04-06 at 08:31 -0700, Bart Van Assche wrote:
> On 04/04/2016 03:58 PM, Nicholas A. Bellinger wrote:
> > Are you going to verify this patch..?
> 
> Hello Nic,
> 
> After that the ib_srpt driver has processed a few commands this patch 
> also triggers a kernel crash (list corruption).

What, no backtrace..?

>  I have a look myself into the percpu_ida conversion.
> 

I found a pair of old ConnectX2 in IB mode and I'll be fixing this up
for v4.6-rc, so don't even bother.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ib_srpt: Add missing ioctx->buf + ->dma alloc_session_cb init

[Bart Van Assche]

On 04/07/2016 03:31 PM, Nicholas A. Bellinger wrote:
>> I will have a look myself into the percpu_ida conversion.
>
> I found a pair of old ConnectX2 in IB mode and I'll be fixing this up
> for v4.6-rc, so don't even bother.

Please make sure that you understand the ib_srpt driver before you try 
to make further modifications to that driver. The patch for percpu_ida 
allocation that is already in v4.6-rc1 introduces namely a severe race 
condition next to the bugs that I had already explained. The proper 
order for session setup is to allocate the send ioctx buffers first and 
after these buffers have been allocated to transition to RTR. You 
changed that order to transition first to RTR and next to allocate the 
send ioctx buffers. By making this change you introduced a race window 
during which any command that is received by the ib_srpt driver will try 
to allocate a send ioctx from a not yet initialized send ioctx ring.

Bart.

Re: [PATCH] ib_srpt: Add missing ioctx->buf + ->dma alloc_session_cb init

[Nicholas A. Bellinger]

On Thu, 2016-04-07 at 15:48 -0700, Bart Van Assche wrote:
> On 04/07/2016 03:31 PM, Nicholas A. Bellinger wrote:
> >> I will have a look myself into the percpu_ida conversion.
> >
> > I found a pair of old ConnectX2 in IB mode and I'll be fixing this up
> > for v4.6-rc, so don't even bother.
> 
> Please make sure that you understand the ib_srpt driver before you try 
> to make further modifications to that driver. The patch for percpu_ida 
> allocation that is already in v4.6-rc1 introduces namely a severe race 
> condition next to the bugs that I had already explained. The proper 
> order for session setup is to allocate the send ioctx buffers first and 
> after these buffers have been allocated to transition to RTR. You 
> changed that order to transition first to RTR and next to allocate the 
> send ioctx buffers. By making this change you introduced a race window 
> during which any command that is received by the ib_srpt driver will try 
> to allocate a send ioctx from a not yet initialized send ioctx ring.
> 

Great, then I'll move srpt_ch_qp_rtr() into the alloc_session callback
as well.

Really, given SRP has no security at all doing all of this allocation
and setup before the target even knows if the initiator is allowed to
login makes zero sense to me.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ib_srpt: fix ifnullfree.cocci warnings

[kbuild test robot]

drivers/infiniband/ulp/srpt/ib_srpt.c:1987:3-8: WARNING: NULL check before freeing functions like kfree, debugfs_remove, debugfs_remove_recursive or usb_free_urb is not needed. Maybe consider reorganizing relevant code to avoid passing NULL values.

 NULL check before some freeing functions is not needed.

 Based on checkpatch warning
 "kfree(NULL) is safe this check is probably not required"
 and kfreeaddr.cocci by Julia Lawall.

Generated by: scripts/coccinelle/free/ifnullfree.cocci

CC: Nicholas Bellinger <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org>
Signed-off-by: Fengguang Wu <fengguang.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---

 ib_srpt.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1983,8 +1983,7 @@ static void srpt_free_sess_cmd_map_res(s
 		if (!ib_dma_mapping_error(sdev->device, ioctx->ioctx.dma))
 			ib_dma_unmap_single(sdev->device, ioctx->ioctx.dma,
 					    dma_size, DMA_TO_DEVICE);
-		if (ioctx->ioctx.buf)
-			kfree(ioctx->ioctx.buf);
+		kfree(ioctx->ioctx.buf);
 	}
 }
 
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html