Re: [PATCH V2] rxe: Fix a sleep-in-atomic bug in post_one_send
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 :: CC: Doug Ledford --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
RE: [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. > ... > 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
[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 --- 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