* [PATCH V3] rxe: Fix a sleep-in-atomic bug in post_one_send
@ 2017-06-01 8:28 Jia-Ju Bai
2017-06-01 9:32 ` Moni Shoua
2017-06-05 6:57 ` Yuval Shaia
0 siblings, 2 replies; 9+ messages in thread
From: Jia-Ju Bai @ 2017-06-01 8:28 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.
This patch corrects the mistakes in V2. (Thank Ram for pointing it out)
Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
---
drivers/infiniband/sw/rxe/rxe_verbs.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
index 83d709e..5293d15 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);
@@ -740,8 +740,11 @@ static int init_send_wqe(struct rxe_qp *qp, struct ib_send_wr *ibwr,
sge = ibwr->sg_list;
for (i = 0; i < num_sge; i++, sge++) {
- if (qp->is_user && copy_from_user(p, (__user void *)
- (uintptr_t)sge->addr, sge->length))
+ 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)
@@ -794,7 +797,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] 9+ messages in thread* Re: [PATCH V3] rxe: Fix a sleep-in-atomic bug in post_one_send
2017-06-01 8:28 [PATCH V3] rxe: Fix a sleep-in-atomic bug in post_one_send Jia-Ju Bai
@ 2017-06-01 9:32 ` Moni Shoua
2017-06-05 6:57 ` Yuval Shaia
1 sibling, 0 replies; 9+ messages in thread
From: Moni Shoua @ 2017-06-01 9:32 UTC (permalink / raw)
To: Jia-Ju Bai
Cc: Sean Hefty, Doug Ledford, Hal Rosenstock, Leon Romanovsky,
linux-rdma, Linux Kernel Mailinglist
On Thu, Jun 1, 2017 at 11:28 AM, Jia-Ju Bai <baijiaju1990@163.com> wrote:
> 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 patch corrects the mistakes in V2. (Thank Ram for pointing it out)
>
> Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
> ---
> drivers/infiniband/sw/rxe/rxe_verbs.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
> index 83d709e..5293d15 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);
> @@ -740,8 +740,11 @@ static int init_send_wqe(struct rxe_qp *qp, struct ib_send_wr *ibwr,
>
> sge = ibwr->sg_list;
> for (i = 0; i < num_sge; i++, sge++) {
> - if (qp->is_user && copy_from_user(p, (__user void *)
> - (uintptr_t)sge->addr, sge->length))
> + spin_unlock_irqrestore(&qp->sq.sq_lock, *flags);
Before the patch, copy_from_user() was called only if qp->is_user was
true. After the patch it is always called.
Second, I think that there is no flow that leads to this function
when qp->is user is true so maybe the correct action is to remove this
line completely
if (qp->is_user && copy_from_user(p, (__user void *)
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH V3] rxe: Fix a sleep-in-atomic bug in post_one_send
2017-06-01 8:28 [PATCH V3] rxe: Fix a sleep-in-atomic bug in post_one_send Jia-Ju Bai
2017-06-01 9:32 ` Moni Shoua
@ 2017-06-05 6:57 ` Yuval Shaia
1 sibling, 0 replies; 9+ messages in thread
From: Yuval Shaia @ 2017-06-05 6:57 UTC (permalink / raw)
To: Jia-Ju Bai
Cc: monis, sean.hefty, dledford, hal.rosenstock, leon, linux-rdma,
linux-kernel
On Thu, Jun 01, 2017 at 04:28:55PM +0800, Jia-Ju Bai wrote:
> 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 patch corrects the mistakes in V2. (Thank Ram for pointing it out)
This line should be added after "---" so it will not go into final commit
message.
>
> Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
> ---
> drivers/infiniband/sw/rxe/rxe_verbs.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
> index 83d709e..5293d15 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);
> @@ -740,8 +740,11 @@ static int init_send_wqe(struct rxe_qp *qp, struct ib_send_wr *ibwr,
>
> sge = ibwr->sg_list;
> for (i = 0; i < num_sge; i++, sge++) {
> - if (qp->is_user && copy_from_user(p, (__user void *)
> - (uintptr_t)sge->addr, sge->length))
> + 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)
> @@ -794,7 +797,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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH V3] rxe: Fix a sleep-in-atomic bug in post_one_send
@ 2017-06-05 7:39 Jia-Ju Bai
2017-06-05 7:44 ` Yuval Shaia
2017-06-05 8:30 ` Moni Shoua
0 siblings, 2 replies; 9+ messages in thread
From: Jia-Ju Bai @ 2017-06-05 7:39 UTC (permalink / raw)
To: yuval.shaia, 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.
Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
---
V3:
* It corrects the mistakes of remaining legacy code in V2.
(Thank Ram for pointing it out)
V2:
* The parameter "flags" is added to restore and save the irq status.
Thank Leon for good advice.
drivers/infiniband/sw/rxe/rxe_verbs.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
index 83d709e..5293d15 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);
@@ -740,8 +740,11 @@ static int init_send_wqe(struct rxe_qp *qp, struct ib_send_wr *ibwr,
sge = ibwr->sg_list;
for (i = 0; i < num_sge; i++, sge++) {
- if (qp->is_user && copy_from_user(p, (__user void *)
- (uintptr_t)sge->addr, sge->length))
+ 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)
@@ -794,7 +797,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] 9+ messages in thread* Re: [PATCH V3] rxe: Fix a sleep-in-atomic bug in post_one_send
2017-06-05 7:39 Jia-Ju Bai
@ 2017-06-05 7:44 ` Yuval Shaia
2017-06-05 8:30 ` Moni Shoua
1 sibling, 0 replies; 9+ messages in thread
From: Yuval Shaia @ 2017-06-05 7:44 UTC (permalink / raw)
To: Jia-Ju Bai
Cc: monis, sean.hefty, dledford, hal.rosenstock, leon, linux-rdma,
linux-kernel
On Mon, Jun 05, 2017 at 03:39:02PM +0800, Jia-Ju Bai wrote:
> 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.
>
>
> Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
> ---
> V3:
> * It corrects the mistakes of remaining legacy code in V2.
> (Thank Ram for pointing it out)
>
> V2:
> * The parameter "flags" is added to restore and save the irq status.
> Thank Leon for good advice.
>
My mistake for not mentioning it specifically but in addition to moving
such version history after the "---" you should also add (manually) a
terminating "---" after the block.
> drivers/infiniband/sw/rxe/rxe_verbs.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
> index 83d709e..5293d15 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);
> @@ -740,8 +740,11 @@ static int init_send_wqe(struct rxe_qp *qp, struct ib_send_wr *ibwr,
>
> sge = ibwr->sg_list;
> for (i = 0; i < num_sge; i++, sge++) {
> - if (qp->is_user && copy_from_user(p, (__user void *)
> - (uintptr_t)sge->addr, sge->length))
> + 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)
> @@ -794,7 +797,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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH V3] rxe: Fix a sleep-in-atomic bug in post_one_send
2017-06-05 7:39 Jia-Ju Bai
2017-06-05 7:44 ` Yuval Shaia
@ 2017-06-05 8:30 ` Moni Shoua
2017-06-05 8:40 ` Jia-Ju Bai
1 sibling, 1 reply; 9+ messages in thread
From: Moni Shoua @ 2017-06-05 8:30 UTC (permalink / raw)
To: Jia-Ju Bai
Cc: yuval.shaia, Sean Hefty, Doug Ledford, Hal Rosenstock,
Leon Romanovsky, linux-rdma, Linux Kernel Mailinglist
> - if (qp->is_user && copy_from_user(p, (__user void *)
> - (uintptr_t)sge->addr, sge->length))
> + 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;
qp-_is_user is always false in this function (flow starts from
rxe_post_send_kernel) so this line is a dead code
In fact, this patch seems to add a serious bug when it uses
copy_from_user() from a non user pointer.
Do you agree?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V3] rxe: Fix a sleep-in-atomic bug in post_one_send
2017-06-05 8:30 ` Moni Shoua
@ 2017-06-05 8:40 ` Jia-Ju Bai
2017-06-05 9:21 ` Moni Shoua
0 siblings, 1 reply; 9+ messages in thread
From: Jia-Ju Bai @ 2017-06-05 8:40 UTC (permalink / raw)
To: Moni Shoua
Cc: yuval.shaia, Sean Hefty, Doug Ledford, Hal Rosenstock,
Leon Romanovsky, linux-rdma, Linux Kernel Mailinglist
On 06/05/2017 04:30 PM, Moni Shoua wrote:
>> - if (qp->is_user&& copy_from_user(p, (__user void *)
>> - (uintptr_t)sge->addr, sge->length))
>> + 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;
> qp-_is_user is always false in this function (flow starts from
> rxe_post_send_kernel) so this line is a dead code
> In fact, this patch seems to add a serious bug when it uses
> copy_from_user() from a non user pointer.
> Do you agree?
I agree.
So, it is fine to me to remove this line, as you said in the former email:
> Second, I think that there is no flow that leads to this function
> when qp->is user is true so maybe the correct action is to remove this
> line completely
> if (qp->is_user&& copy_from_user(p, (__user void *)
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH V3] rxe: Fix a sleep-in-atomic bug in post_one_send
@ 2017-06-05 7:55 Jia-Ju Bai
0 siblings, 0 replies; 9+ messages in thread
From: Jia-Ju Bai @ 2017-06-05 7:55 UTC (permalink / raw)
To: yuval.shaia, 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.
Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
---
V3:
* It corrects the mistakes of remaining legacy code in V2.
(Thank Ram for pointing it out)
V2:
* The parameter "flags" is added to restore and save the irq status.
Thank Leon for good advice.
---
drivers/infiniband/sw/rxe/rxe_verbs.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
index 83d709e..5293d15 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);
@@ -740,8 +740,11 @@ static int init_send_wqe(struct rxe_qp *qp, struct ib_send_wr *ibwr,
sge = ibwr->sg_list;
for (i = 0; i < num_sge; i++, sge++) {
- if (qp->is_user && copy_from_user(p, (__user void *)
- (uintptr_t)sge->addr, sge->length))
+ 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)
@@ -794,7 +797,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] 9+ messages in thread
end of thread, other threads:[~2017-06-05 9:21 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-01 8:28 [PATCH V3] rxe: Fix a sleep-in-atomic bug in post_one_send Jia-Ju Bai
2017-06-01 9:32 ` Moni Shoua
2017-06-05 6:57 ` Yuval Shaia
-- strict thread matches above, loose matches on Subject: below --
2017-06-05 7:39 Jia-Ju Bai
2017-06-05 7:44 ` Yuval Shaia
2017-06-05 8:30 ` Moni Shoua
2017-06-05 8:40 ` Jia-Ju Bai
2017-06-05 9:21 ` Moni Shoua
2017-06-05 7:55 Jia-Ju Bai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox