All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V2] rxe: Fix a sleep-in-atomic bug in post_one_send
@ 2017-06-01  7:45 ` Jia-Ju Bai
  0 siblings, 0 replies; 6+ messages in thread
From: Jia-Ju Bai @ 2017-06-01  7:45 UTC (permalink / raw)
  To: monis-VPRAkNaXOzVWk0Htik3J/w, sean.hefty-ral2JQCrhuEAvxtiuMwx3w,
	dledford-H+wXaHxf7aLQT0dZR+AlfA,
	hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w,
	leon-DgEjT+Ai2ygdnm+yROfE0A
  Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Jia-Ju Bai

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH V2] rxe: Fix a sleep-in-atomic bug in post_one_send
@ 2017-06-01  7:45 ` Jia-Ju Bai
  0 siblings, 0 replies; 6+ messages in thread
From: Jia-Ju Bai @ 2017-06-01  7:45 UTC (permalink / raw)
  To: monis, sean.hefty, dledford, hal.rosenstock, leon
  Cc: linux-rdma, linux-kernel, Jia-Ju Bai

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@163.com>
---
 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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* RE: [PATCH V2] rxe: Fix a sleep-in-atomic bug in post_one_send
  2017-06-01  7:45 ` Jia-Ju Bai
@ 2017-06-01  8:11     ` Amrani, Ram
  -1 siblings, 0 replies; 6+ messages in thread
From: Amrani, Ram @ 2017-06-01  8:11 UTC (permalink / raw)
  To: Jia-Ju Bai, monis-VPRAkNaXOzVWk0Htik3J/w,
	sean.hefty-ral2JQCrhuEAvxtiuMwx3w,
	dledford-H+wXaHxf7aLQT0dZR+AlfA,
	hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w,
	leon-DgEjT+Ai2ygdnm+yROfE0A
  Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA

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

...

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

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* RE: [PATCH V2] rxe: Fix a sleep-in-atomic bug in post_one_send
@ 2017-06-01  8:11     ` Amrani, Ram
  0 siblings, 0 replies; 6+ messages in thread
From: Amrani, Ram @ 2017-06-01  8:11 UTC (permalink / raw)
  To: Jia-Ju Bai, monis, sean.hefty, dledford, hal.rosenstock, leon
  Cc: linux-rdma, linux-kernel

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

...

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

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH V2] rxe: Fix a sleep-in-atomic bug in post_one_send
  2017-06-01  7:45 ` Jia-Ju Bai
@ 2017-06-01  9:28   ` kbuild test robot
  -1 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2017-06-01  9:28 UTC (permalink / raw)
  Cc: kbuild-all, monis, sean.hefty, dledford, hal.rosenstock, leon,
	linux-rdma, linux-kernel, Jia-Ju Bai

[-- Attachment #1: Type: text/plain, Size: 3191 bytes --]

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':
>> 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 <monis@mellanox.com>
:::::: CC: Doug Ledford <dledford@redhat.com>

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 59505 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH V2] rxe: Fix a sleep-in-atomic bug in post_one_send
@ 2017-06-01  9:28   ` kbuild test robot
  0 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2017-06-01  9:28 UTC (permalink / raw)
  To: Jia-Ju Bai
  Cc: kbuild-all, monis, sean.hefty, dledford, hal.rosenstock, leon,
	linux-rdma, linux-kernel, Jia-Ju Bai

[-- Attachment #1: Type: text/plain, Size: 3191 bytes --]

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':
>> 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 <monis@mellanox.com>
:::::: CC: Doug Ledford <dledford@redhat.com>

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 59505 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-06-01  9:29 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-01  7:45 [PATCH V2] rxe: Fix a sleep-in-atomic bug in post_one_send Jia-Ju Bai
2017-06-01  7:45 ` Jia-Ju Bai
     [not found] ` <1496303141-14925-1-git-send-email-baijiaju1990-9Onoh4P/yGk@public.gmane.org>
2017-06-01  8:11   ` Amrani, Ram
2017-06-01  8:11     ` Amrani, Ram
2017-06-01  9:28 ` kbuild test robot
2017-06-01  9:28   ` kbuild test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.