[PATCH] RDMA/cxgb4: don't declare wr_wait objects on the stack.

[PATCH] RDMA/cxgb4: don't declare wr_wait objects on the stack.

[Steve Wise]

There exists a race condition when using c4iw_wr_wait objects that are
declared on the stack.  This was being done in a few places where we are
sending work requests to the FW and awaiting replies, but we don't have
an endpoint structure with an embedded c4iw_wr_wait struct.  So the code
was allocating it locally on the stack.  Bad design. The race is this:

thread on cpuX declares the wait object on the stack, then posts
a firmware WR with that wait object ptr as the cookie to be returned
in the WR reply.  This thread will proceed to block in wait_event_timeout()
but before it does:

An interrupt runs on cpuY with the WR reply.  fw6_msg() handles this
and calls c4iw_wake_up().  c4iw_wake_up() sets the condition variable in
the c4iw_wr_wait object to TRUE and will call wake_up(), but before it
calls wake_up():

The thread on cpuX calls c4iw_wait_for_reply(), which calls
wait_event_timeout().  The wait_event_timeout() macro checks the condition
variable and returns immediately since it is TRUE.  So this thread
never blocks/sleeps. The function then returns effectively deallocating
the c4iw_wr_wait object that was on the stack.

So at this point cpuY has a pointer to the c4iw_wr_wait object that is no
longer valid.  Further its pointing to a stack frame that might now be in
use by some other context/thread.  So cpuY continues execution and calls
wake_up() on a ptr to a wait object that as been effectively deallocated.

This race, when it hits, can cause a crash in wake_up(), which I've seen
under heavy stress. It can also corrupt the referenced stack which can
cause any number of failures.

The fix:

Allocate temporary c4iw_wr_wait objects and keep references on them so that it
is never freed until both execution paths are done using it.

Signed-off-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
---

 drivers/infiniband/hw/cxgb4/cm.c       |   12 +++++++++++-
 drivers/infiniband/hw/cxgb4/cq.c       |   33 ++++++++++++++++++++++----------
 drivers/infiniband/hw/cxgb4/iw_cxgb4.h |   29 +++++++++++++++++++++++++++-
 drivers/infiniband/hw/cxgb4/mem.c      |   10 ++++++----
 drivers/infiniband/hw/cxgb4/qp.c       |   16 +++++++++++-----
 5 files changed, 79 insertions(+), 21 deletions(-)

diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c
index d7ee70f..fcbc803 100644
--- a/drivers/infiniband/hw/cxgb4/cm.c
+++ b/drivers/infiniband/hw/cxgb4/cm.c
@@ -255,6 +255,13 @@ static void *alloc_ep(int size, gfp_t gfp)
 	return epc;
 }
 
+void _c4iw_free_tmp_wr_wait(struct kref *kref)
+{
+	struct c4iw_wr_wait *wr_waitp;
+	wr_waitp = container_of(kref, struct c4iw_wr_wait, kref);
+	kfree(wr_waitp);
+}
+
 void _c4iw_free_ep(struct kref *kref)
 {
 	struct c4iw_ep *ep;
@@ -2284,8 +2291,11 @@ static int fw6_msg(struct c4iw_dev *dev, struct sk_buff *skb)
 		ret = (int)((be64_to_cpu(rpl->data[0]) >> 8) & 0xff);
 		wr_waitp = (struct c4iw_wr_wait *)(__force unsigned long) rpl->data[1];
 		PDBG("%s wr_waitp %p ret %u\n", __func__, wr_waitp, ret);
-		if (wr_waitp)
+		if (wr_waitp) {
 			c4iw_wake_up(wr_waitp, ret ? -ret : 0);
+			if (wr_waitp->tmp)
+				c4iw_put_tmp_wr_wait(wr_waitp);
+		}
 		kfree_skb(skb);
 		break;
 	case 2:
diff --git a/drivers/infiniband/hw/cxgb4/cq.c b/drivers/infiniband/hw/cxgb4/cq.c
index 8d8f8ad..4fbe82e 100644
--- a/drivers/infiniband/hw/cxgb4/cq.c
+++ b/drivers/infiniband/hw/cxgb4/cq.c
@@ -38,7 +38,7 @@ static int destroy_cq(struct c4iw_rdev *rdev, struct t4_cq *cq,
 	struct fw_ri_res_wr *res_wr;
 	struct fw_ri_res *res;
 	int wr_len;
-	struct c4iw_wr_wait wr_wait;
+	struct c4iw_wr_wait *wr_waitp;
 	struct sk_buff *skb;
 	int ret;
 
@@ -46,6 +46,13 @@ static int destroy_cq(struct c4iw_rdev *rdev, struct t4_cq *cq,
 	skb = alloc_skb(wr_len, GFP_KERNEL);
 	if (!skb)
 		return -ENOMEM;
+
+	wr_waitp = c4iw_get_tmp_wr_wait();
+	if (!wr_waitp) {
+		kfree_skb(skb);
+		return -ENOMEM;
+	}
+		
 	set_wr_txq(skb, CPL_PRIORITY_CONTROL, 0);
 
 	res_wr = (struct fw_ri_res_wr *)__skb_put(skb, wr_len);
@@ -55,16 +62,15 @@ static int destroy_cq(struct c4iw_rdev *rdev, struct t4_cq *cq,
 			V_FW_RI_RES_WR_NRES(1) |
 			FW_WR_COMPL(1));
 	res_wr->len16_pkd = cpu_to_be32(DIV_ROUND_UP(wr_len, 16));
-	res_wr->cookie = (unsigned long) &wr_wait;
+	res_wr->cookie = (unsigned long) wr_waitp;
 	res = res_wr->res;
 	res->u.cq.restype = FW_RI_RES_TYPE_CQ;
 	res->u.cq.op = FW_RI_RES_OP_RESET;
 	res->u.cq.iqid = cpu_to_be32(cq->cqid);
 
-	c4iw_init_wr_wait(&wr_wait);
 	ret = c4iw_ofld_send(rdev, skb);
 	if (!ret) {
-		ret = c4iw_wait_for_reply(rdev, &wr_wait, 0, 0, __func__);
+		ret = c4iw_wait_for_reply(rdev, wr_waitp, 0, 0, __func__);
 	}
 
 	kfree(cq->sw_queue);
@@ -82,7 +88,7 @@ static int create_cq(struct c4iw_rdev *rdev, struct t4_cq *cq,
 	struct fw_ri_res *res;
 	int wr_len;
 	int user = (uctx != &rdev->uctx);
-	struct c4iw_wr_wait wr_wait;
+	struct c4iw_wr_wait *wr_waitp;
 	int ret;
 	struct sk_buff *skb;
 
@@ -116,6 +122,13 @@ static int create_cq(struct c4iw_rdev *rdev, struct t4_cq *cq,
 		ret = -ENOMEM;
 		goto err4;
 	}
+
+	wr_waitp = c4iw_get_tmp_wr_wait();
+	if (!wr_waitp) {
+		ret = -ENOMEM;
+		goto err5;
+	}
+
 	set_wr_txq(skb, CPL_PRIORITY_CONTROL, 0);
 
 	res_wr = (struct fw_ri_res_wr *)__skb_put(skb, wr_len);
@@ -125,7 +138,7 @@ static int create_cq(struct c4iw_rdev *rdev, struct t4_cq *cq,
 			V_FW_RI_RES_WR_NRES(1) |
 			FW_WR_COMPL(1));
 	res_wr->len16_pkd = cpu_to_be32(DIV_ROUND_UP(wr_len, 16));
-	res_wr->cookie = (unsigned long) &wr_wait;
+	res_wr->cookie = (unsigned long) wr_waitp;
 	res = res_wr->res;
 	res->u.cq.restype = FW_RI_RES_TYPE_CQ;
 	res->u.cq.op = FW_RI_RES_OP_WRITE;
@@ -144,13 +157,11 @@ static int create_cq(struct c4iw_rdev *rdev, struct t4_cq *cq,
 	res->u.cq.iqsize = cpu_to_be16(cq->size);
 	res->u.cq.iqaddr = cpu_to_be64(cq->dma_addr);
 
-	c4iw_init_wr_wait(&wr_wait);
-
 	ret = c4iw_ofld_send(rdev, skb);
 	if (ret)
 		goto err4;
-	PDBG("%s wait_event wr_wait %p\n", __func__, &wr_wait);
-	ret = c4iw_wait_for_reply(rdev, &wr_wait, 0, 0, __func__);
+	PDBG("%s wait_event wr_wait %p\n", __func__, wr_waitp);
+	ret = c4iw_wait_for_reply(rdev, wr_waitp, 0, 0, __func__);
 	if (ret)
 		goto err4;
 
@@ -163,6 +174,8 @@ static int create_cq(struct c4iw_rdev *rdev, struct t4_cq *cq,
 		cq->ugts &= PAGE_MASK;
 	}
 	return 0;
+err5:
+	kfree_skb(skb);
 err4:
 	dma_free_coherent(&rdev->lldi.pdev->dev, cq->memsize, cq->queue,
 			  dma_unmap_addr(cq, mapping));
diff --git a/drivers/infiniband/hw/cxgb4/iw_cxgb4.h b/drivers/infiniband/hw/cxgb4/iw_cxgb4.h
index 35d2a5d..63d9ca8 100644
--- a/drivers/infiniband/hw/cxgb4/iw_cxgb4.h
+++ b/drivers/infiniband/hw/cxgb4/iw_cxgb4.h
@@ -139,15 +139,39 @@ struct c4iw_wr_wait {
 	wait_queue_head_t wait;
 	unsigned long status;
 	int ret;
+	struct kref kref;
+	int tmp;
 };
 
 static inline void c4iw_init_wr_wait(struct c4iw_wr_wait *wr_waitp)
 {
+	wr_waitp->tmp = 0;
 	wr_waitp->ret = 0;
 	wr_waitp->status = 0;
 	init_waitqueue_head(&wr_waitp->wait);
 }
 
+static inline struct c4iw_wr_wait *c4iw_get_tmp_wr_wait(void)
+{
+	struct c4iw_wr_wait *wr_waitp;
+
+	wr_waitp = kmalloc(sizeof *wr_waitp, GFP_KERNEL);
+	if (wr_waitp) {
+		c4iw_init_wr_wait(wr_waitp);
+		kref_init(&wr_waitp->kref);
+		kref_get(&wr_waitp->kref);
+		wr_waitp->tmp = 1;
+	}
+	return wr_waitp;
+}
+
+void _c4iw_free_tmp_wr_wait(struct kref *kref);
+
+static inline void c4iw_put_tmp_wr_wait(struct c4iw_wr_wait *wr_waitp)
+{
+	kref_put(&wr_waitp->kref, _c4iw_free_tmp_wr_wait);
+}
+
 static inline void c4iw_wake_up(struct c4iw_wr_wait *wr_waitp, int ret)
 {
 	wr_waitp->ret = ret;
@@ -180,7 +204,10 @@ static inline int c4iw_wait_for_reply(struct c4iw_rdev *rdev,
 	if (wr_waitp->ret)
 		PDBG("%s: FW reply %d tid %u qpid %u\n",
 		     pci_name(rdev->lldi.pdev), wr_waitp->ret, hwtid, qpid);
-	return wr_waitp->ret;
+	ret = wr_waitp->ret;
+	if (wr_waitp->tmp)
+		c4iw_put_tmp_wr_wait(wr_waitp);
+	return ret;
 }
 
 struct c4iw_dev {
diff --git a/drivers/infiniband/hw/cxgb4/mem.c b/drivers/infiniband/hw/cxgb4/mem.c
index 273ffe4..685928d 100644
--- a/drivers/infiniband/hw/cxgb4/mem.c
+++ b/drivers/infiniband/hw/cxgb4/mem.c
@@ -46,12 +46,14 @@ static int write_adapter_mem(struct c4iw_rdev *rdev, u32 addr, u32 len,
 	struct ulptx_idata *sc;
 	u8 wr_len, *to_dp, *from_dp;
 	int copy_len, num_wqe, i, ret = 0;
-	struct c4iw_wr_wait wr_wait;
+	struct c4iw_wr_wait *wr_waitp;
 
 	addr &= 0x7FFFFFF;
 	PDBG("%s addr 0x%x len %u\n", __func__, addr, len);
 	num_wqe = DIV_ROUND_UP(len, C4IW_MAX_INLINE_SIZE);
-	c4iw_init_wr_wait(&wr_wait);
+	wr_waitp = c4iw_get_tmp_wr_wait();
+	if (!wr_waitp)
+		return -ENOMEM;
 	for (i = 0; i < num_wqe; i++) {
 
 		copy_len = len > C4IW_MAX_INLINE_SIZE ? C4IW_MAX_INLINE_SIZE :
@@ -71,7 +73,7 @@ static int write_adapter_mem(struct c4iw_rdev *rdev, u32 addr, u32 len,
 		if (i == (num_wqe-1)) {
 			req->wr.wr_hi = cpu_to_be32(FW_WR_OP(FW_ULPTX_WR) |
 						    FW_WR_COMPL(1));
-			req->wr.wr_lo = (__force __be64)(unsigned long) &wr_wait;
+			req->wr.wr_lo = (__force __be64)(unsigned long) wr_waitp;
 		} else
 			req->wr.wr_hi = cpu_to_be32(FW_WR_OP(FW_ULPTX_WR));
 		req->wr.wr_mid = cpu_to_be32(
@@ -103,7 +105,7 @@ static int write_adapter_mem(struct c4iw_rdev *rdev, u32 addr, u32 len,
 		len -= C4IW_MAX_INLINE_SIZE;
 	}
 
-	ret = c4iw_wait_for_reply(rdev, &wr_wait, 0, 0, __func__);
+	ret = c4iw_wait_for_reply(rdev, wr_waitp, 0, 0, __func__);
 	return ret;
 }
 
diff --git a/drivers/infiniband/hw/cxgb4/qp.c b/drivers/infiniband/hw/cxgb4/qp.c
index a1824a5..d295a20 100644
--- a/drivers/infiniband/hw/cxgb4/qp.c
+++ b/drivers/infiniband/hw/cxgb4/qp.c
@@ -115,7 +115,7 @@ static int create_qp(struct c4iw_rdev *rdev, struct t4_wq *wq,
 	struct fw_ri_res_wr *res_wr;
 	struct fw_ri_res *res;
 	int wr_len;
-	struct c4iw_wr_wait wr_wait;
+	struct c4iw_wr_wait *wr_waitp;
 	struct sk_buff *skb;
 	int ret;
 	int eqsize;
@@ -191,6 +191,12 @@ static int create_qp(struct c4iw_rdev *rdev, struct t4_wq *wq,
 		ret = -ENOMEM;
 		goto err7;
 	}
+	wr_waitp = c4iw_get_tmp_wr_wait();
+	if (!wr_waitp) {
+		ret = -ENOMEM;
+		goto err8;
+	}
+		
 	set_wr_txq(skb, CPL_PRIORITY_CONTROL, 0);
 
 	res_wr = (struct fw_ri_res_wr *)__skb_put(skb, wr_len);
@@ -200,7 +206,7 @@ static int create_qp(struct c4iw_rdev *rdev, struct t4_wq *wq,
 			V_FW_RI_RES_WR_NRES(2) |
 			FW_WR_COMPL(1));
 	res_wr->len16_pkd = cpu_to_be32(DIV_ROUND_UP(wr_len, 16));
-	res_wr->cookie = (unsigned long) &wr_wait;
+	res_wr->cookie = (unsigned long) wr_waitp;
 	res = res_wr->res;
 	res->u.sqrq.restype = FW_RI_RES_TYPE_SQ;
 	res->u.sqrq.op = FW_RI_RES_OP_WRITE;
@@ -250,12 +256,10 @@ static int create_qp(struct c4iw_rdev *rdev, struct t4_wq *wq,
 	res->u.sqrq.eqid = cpu_to_be32(wq->rq.qid);
 	res->u.sqrq.eqaddr = cpu_to_be64(wq->rq.dma_addr);
 
-	c4iw_init_wr_wait(&wr_wait);
-
 	ret = c4iw_ofld_send(rdev, skb);
 	if (ret)
 		goto err7;
-	ret = c4iw_wait_for_reply(rdev, &wr_wait, 0, wq->sq.qid, __func__);
+	ret = c4iw_wait_for_reply(rdev, wr_waitp, 0, wq->sq.qid, __func__);
 	if (ret)
 		goto err7;
 
@@ -264,6 +268,8 @@ static int create_qp(struct c4iw_rdev *rdev, struct t4_wq *wq,
 	     (unsigned long long)wq->sq.udb, (unsigned long long)wq->rq.udb);
 
 	return 0;
+err8:
+	kfree_skb(skb);
 err7:
 	dma_free_coherent(&(rdev->lldi.pdev->dev),
 			  wq->rq.memsize, wq->rq.queue,

--
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] RDMA/cxgb4: don't declare wr_wait objects on the stack.

[Steve Wise]

Roland, I need to recall this patch.  It appears to have some problem.


Steve.

--
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] RDMA/cxgb4: don't declare wr_wait objects on the stack.

[Roland Dreier]

On Mon, May 16, 2011 at 8:18 AM, Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+v7I7tHvgBF7@public.gmane.orgm> wrote:
> Roland, I need to recall this patch.  It appears to have some problem.

Good thing I didn't apply it yet.

I did think it should be possible to declare wait objects on the stack... the
origin of completions was to handle exactly that issue; the older semaphore
structure was vulnerable to the wakeup after free, but completions handle
that.  But I didn't look at the cxgb4 code at all...

 - R.
--
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] RDMA/cxgb4: don't declare wr_wait objects on the stack.

[Steve Wise]

On 05/16/2011 10:53 AM, Roland Dreier wrote:
> On Mon, May 16, 2011 at 8:18 AM, Steve Wise<swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>  wrote:
>> Roland, I need to recall this patch.  It appears to have some problem.
> Good thing I didn't apply it yet.
>
> I did think it should be possible to declare wait objects on the stack... the
> origin of completions was to handle exactly that issue; the older semaphore
> structure was vulnerable to the wakeup after free, but completions handle
> that.  But I didn't look at the cxgb4 code at all...

I'm pretty sure the race exists as described in my commit comment.  And I don't see how the wait object code could avoid 
this issue.  Unless I'm just all wrong. :)


--
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] RDMA/cxgb4: don't declare wr_wait objects on the stack.

[Steve Wise]

On 05/16/2011 10:56 AM, Steve Wise wrote:
> On 05/16/2011 10:53 AM, Roland Dreier wrote:
>> On Mon, May 16, 2011 at 8:18 AM, Steve Wise<swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>  wrote:
>>> Roland, I need to recall this patch.  It appears to have some problem.
>> Good thing I didn't apply it yet.
>>
>> I did think it should be possible to declare wait objects on the stack... the
>> origin of completions was to handle exactly that issue; the older semaphore
>> structure was vulnerable to the wakeup after free, but completions handle
>> that.  But I didn't look at the cxgb4 code at all...
>
> I'm pretty sure the race exists as described in my commit comment.  And I don't see how the wait object code could 
> avoid this issue.  Unless I'm just all wrong. :)
>
>

FYI:

So the crash I kept seeing was that my wake up path would end up accessing a freed wait object.  I added a 100us udelay 
in this path just after setting the wait condition to true, but before calling wake_up().  With this delay, I hit the 
crash much more quickly.  So in my mind I proved that the race exists.   I then added this commit to get rid of on-stack 
wait objects, but left the 100us delay in my code and tested that the crash goes away.  So I proved, in my mind, that 1) 
this was a real issue, and 2) my fix worked.  I then ran some regression tests to convince myself the fix was stable 
before posting the patch on Friday.  However our QA seems to be seeing some issues with this patch on various backports 
with older kernels, so I need to investigate this further.  Thus I recalled the 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] RDMA/cxgb4: don't declare wr_wait objects on the stack.

[Steve Wise]

On 05/16/2011 11:05 AM, Steve Wise wrote:
> On 05/16/2011 10:56 AM, Steve Wise wrote:
>> On 05/16/2011 10:53 AM, Roland Dreier wrote:
>>> On Mon, May 16, 2011 at 8:18 AM, Steve Wise<swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>  wrote:
>>>> Roland, I need to recall this patch.  It appears to have some problem.
>>> Good thing I didn't apply it yet.
>>>
>>> I did think it should be possible to declare wait objects on the stack... the
>>> origin of completions was to handle exactly that issue; the older semaphore
>>> structure was vulnerable to the wakeup after free, but completions handle
>>> that.  But I didn't look at the cxgb4 code at all...
>>
>> I'm pretty sure the race exists as described in my commit comment.  And I don't see how the wait object code could 
>> avoid this issue.  Unless I'm just all wrong. :)
>>
>>
>
> FYI:
>
> So the crash I kept seeing was that my wake up path would end up accessing a freed wait object.  I added a 100us 
> udelay in this path just after setting the wait condition to true, but before calling wake_up().  With this delay, I 
> hit the crash much more quickly.  So in my mind I proved that the race exists.   I then added this commit to get rid 
> of on-stack wait objects, but left the 100us delay in my code and tested that the crash goes away.  So I proved, in my 
> mind, that 1) this was a real issue, and 2) my fix worked.  I then ran some regression tests to convince myself the 
> fix was stable before posting the patch on Friday.  However our QA seems to be seeing some issues with this patch on 
> various backports with older kernels, so I need to investigate this further.  Thus I recalled the patch.
>
>

Roland,  complete() and wait_for_completion() is exactly what I need to be using. It solves the race condition and 
allows on-stack declarations.

I'll re-post a new patch once its ready and tested.

Thanks,

Steve.





--
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