Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1654918 > unrolled thread
| Started by | Jia-Ju Bai <baijiaju1990@163.com> |
|---|---|
| First post | 2017-06-01 10:30 +0200 |
| Last post | 2017-06-05 09:00 +0200 |
| Articles | 3 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH V3] rxe: Fix a sleep-in-atomic bug in post_one_send Jia-Ju Bai <baijiaju1990@163.com> - 2017-06-01 10:30 +0200
Re: [PATCH V3] rxe: Fix a sleep-in-atomic bug in post_one_send Moni Shoua <monis@mellanox.com> - 2017-06-01 11:40 +0200
Re: [PATCH V3] rxe: Fix a sleep-in-atomic bug in post_one_send Yuval Shaia <yuval.shaia@oracle.com> - 2017-06-05 09:00 +0200
| From | Jia-Ju Bai <baijiaju1990@163.com> |
|---|---|
| Date | 2017-06-01 10:30 +0200 |
| Subject | [PATCH V3] rxe: Fix a sleep-in-atomic bug in post_one_send |
| Message-ID | <tNtEu-9b-1@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Moni Shoua <monis@mellanox.com> |
|---|---|
| Date | 2017-06-01 11:40 +0200 |
| Message-ID | <tNuKe-LA-9@gated-at.bofh.it> |
| In reply to | #1654918 |
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 *)
[toc] | [prev] | [next] | [standalone]
| From | Yuval Shaia <yuval.shaia@oracle.com> |
|---|---|
| Date | 2017-06-05 09:00 +0200 |
| Message-ID | <tOU9z-7TO-1@gated-at.bofh.it> |
| In reply to | #1654918 |
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web