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 <[email protected]>
---
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
> 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
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 <[email protected]>
:::::: CC: Doug Ledford <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation