[PATCH V2] rxe: Fix a sleep-in-atomic bug in post_one_send

The driver may sleep under a spin lock, and the function call path is:
post_one_send (acquire the lock by spin_lock_irqsave)
  init_send_wqe
    copy_from_user --> may sleep

To fix it, the lock is released before copy_from_user, and the lock is
acquired again after this function. The parameter "flags" is used to
restore and save the irq status.
Thank Leon for good advice.

Signed-off-by: Jia-Ju Bai <baijiaju1990-9Onoh4P/yGk@public.gmane.org>
---
 drivers/infiniband/sw/rxe/rxe_verbs.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
index 83d709e..7dcdf67 100644
--- a/drivers/infiniband/sw/rxe/rxe_verbs.c
+++ b/drivers/infiniband/sw/rxe/rxe_verbs.c

@@ -721,11 +721,11 @@ static void init_send_wr(struct rxe_qp *qp, struct rxe_send_wr *wr,
 
 static int init_send_wqe(struct rxe_qp *qp, struct ib_send_wr *ibwr,
 			 unsigned int mask, unsigned int length,
-			 struct rxe_send_wqe *wqe)
+			 struct rxe_send_wqe *wqe, unsigned long *flags)
 {
 	int num_sge = ibwr->num_sge;
 	struct ib_sge *sge;
-	int i;
+	int i, err;
 	u8 *p;
 
 	init_send_wr(qp, &wqe->wr, ibwr);

@@ -742,7 +742,12 @@ static int init_send_wqe(struct rxe_qp *qp, struct ib_send_wr *ibwr,
 		for (i = 0; i < num_sge; i++, sge++) {
 			if (qp->is_user && copy_from_user(p, (__user void *)
 					    (uintptr_t)sge->addr, sge->length))
-				return -EFAULT;
+				spin_unlock_irqrestore(&qp->sq.sq_lock, *flags);
+				err = copy_from_user(p, (__user void *)
+					    (uintptr_t)sge->addr, sge->length);
+				spin_lock_irqsave(&qp->sq.sq_lock, *flags);
+				if (qp->is_user && err)
+					return -EFAULT;
 
 			else if (!qp->is_user)
 				memcpy(p, (void *)(uintptr_t)sge->addr,

@@ -794,7 +799,7 @@ static int post_one_send(struct rxe_qp *qp, struct ib_send_wr *ibwr,
 
 	send_wqe = producer_addr(sq->queue);
 
-	err = init_send_wqe(qp, ibwr, mask, length, send_wqe);
+	err = init_send_wqe(qp, ibwr, mask, length, send_wqe, &flags);
 	if (unlikely(err))
 		goto err1;
 

-- 
1.7.9.5


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

The driver may sleep under a spin lock, and the function call path is:
post_one_send (acquire the lock by spin_lock_irqsave)
  init_send_wqe
    copy_from_user --> may sleep

To fix it, the lock is released before copy_from_user, and the lock is
acquired again after this function. The parameter "flags" is used to
restore and save the irq status.
Thank Leon for good advice.

...

This isn't my area of expertise. Still something seems weird.
You are still calling 'copy_from_user' unprotected in the 'if'.
Also, did you mean to use curly brackets on the indented part after the first if?!

Ram

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

Hi Jia-Ju,

[auto build test WARNING on rdma/master]
[also build test WARNING on v4.12-rc3 next-20170601]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jia-Ju-Bai/rxe-Fix-a-sleep-in-atomic-bug-in-post_one_send/20170601-160036
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dledford/rdma.git master
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   drivers/infiniband/sw/rxe/rxe_verbs.c: In function 'init_send_wqe':

quoted

drivers/infiniband/sw/rxe/rxe_verbs.c:743:4: warning: this 'if' clause does not guard... [-Wmisleading-indentation]

       if (qp->is_user && copy_from_user(p, (__user void *)
       ^~
   drivers/infiniband/sw/rxe/rxe_verbs.c:746:5: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'
        err = copy_from_user(p, (__user void *)
        ^~~

vim +/if +743 drivers/infiniband/sw/rxe/rxe_verbs.c

8700e3e7 Moni Shoua 2016-06-16  727  	struct ib_sge *sge;
773c8e53 Jia-Ju Bai 2017-06-01  728  	int i, err;
8700e3e7 Moni Shoua 2016-06-16  729  	u8 *p;
8700e3e7 Moni Shoua 2016-06-16  730  
8700e3e7 Moni Shoua 2016-06-16  731  	init_send_wr(qp, &wqe->wr, ibwr);
8700e3e7 Moni Shoua 2016-06-16  732  
8700e3e7 Moni Shoua 2016-06-16  733  	if (qp_type(qp) == IB_QPT_UD ||
8700e3e7 Moni Shoua 2016-06-16  734  	    qp_type(qp) == IB_QPT_SMI ||
8700e3e7 Moni Shoua 2016-06-16  735  	    qp_type(qp) == IB_QPT_GSI)
8700e3e7 Moni Shoua 2016-06-16  736  		memcpy(&wqe->av, &to_rah(ud_wr(ibwr)->ah)->av, sizeof(wqe->av));
8700e3e7 Moni Shoua 2016-06-16  737  
8700e3e7 Moni Shoua 2016-06-16  738  	if (unlikely(ibwr->send_flags & IB_SEND_INLINE)) {
8700e3e7 Moni Shoua 2016-06-16  739  		p = wqe->dma.inline_data;
8700e3e7 Moni Shoua 2016-06-16  740  
8700e3e7 Moni Shoua 2016-06-16  741  		sge = ibwr->sg_list;
8700e3e7 Moni Shoua 2016-06-16  742  		for (i = 0; i < num_sge; i++, sge++) {
8700e3e7 Moni Shoua 2016-06-16 @743  			if (qp->is_user && copy_from_user(p, (__user void *)
8700e3e7 Moni Shoua 2016-06-16  744  					    (uintptr_t)sge->addr, sge->length))
773c8e53 Jia-Ju Bai 2017-06-01  745  				spin_unlock_irqrestore(&qp->sq.sq_lock, *flags);
773c8e53 Jia-Ju Bai 2017-06-01  746  				err = copy_from_user(p, (__user void *)
773c8e53 Jia-Ju Bai 2017-06-01  747  					    (uintptr_t)sge->addr, sge->length);
773c8e53 Jia-Ju Bai 2017-06-01  748  				spin_lock_irqsave(&qp->sq.sq_lock, *flags);
773c8e53 Jia-Ju Bai 2017-06-01  749  				if (qp->is_user && err)
8700e3e7 Moni Shoua 2016-06-16  750  					return -EFAULT;
8700e3e7 Moni Shoua 2016-06-16  751  

:::::: The code at line 743 was first introduced by commit
:::::: 8700e3e7c4857d28ebaa824509934556da0b3e76 Soft RoCE driver

:::::: TO: Moni Shoua [off-list ref]
:::::: CC: Doug Ledford [off-list ref]

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