* [bug report] RDMA/siw: Fix SQ/RQ drain logic
@ 2019-10-24 20:38 Dan Carpenter
2019-10-25 8:16 ` Bernard Metzler
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2019-10-24 20:38 UTC (permalink / raw)
To: bmt; +Cc: linux-rdma
Hello Bernard Metzler,
The patch cf049bb31f71: "RDMA/siw: Fix SQ/RQ drain logic" from Oct 4,
2019, leads to the following static checker warning:
drivers/infiniband/sw/siw/siw_verbs.c:1079 siw_post_receive()
error: locking inconsistency. We assume 'read_sem:&qp->state_lock' is both locked and unlocked at the start.
drivers/infiniband/sw/siw/siw_verbs.c
978 int siw_post_receive(struct ib_qp *base_qp, const struct ib_recv_wr *wr,
979 const struct ib_recv_wr **bad_wr)
980 {
981 struct siw_qp *qp = to_siw_qp(base_qp);
982 unsigned long flags;
983 int rv = 0;
984
985 if (qp->srq) {
986 *bad_wr = wr;
987 return -EOPNOTSUPP; /* what else from errno.h? */
988 }
989 if (!qp->kernel_verbs) {
990 siw_dbg_qp(qp, "no kernel post_recv for user mapped sq\n");
991 up_read(&qp->state_lock);
^^^^^^^^^^^^^^^^^^^^^^^^
The patch changes the locking so this isn't held here and should be
released. Should it be held, though?
992 *bad_wr = wr;
993 return -EINVAL;
994 }
995
996 /*
997 * Try to acquire QP state lock. Must be non-blocking
998 * to accommodate kernel clients needs.
999 */
1000 if (!down_read_trylock(&qp->state_lock)) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
1001 if (qp->attrs.state == SIW_QP_STATE_ERROR) {
1002 /*
1003 * ERROR state is final, so we can be sure
1004 * this state will not change as long as the QP
1005 * exists.
1006 *
1007 * This handles an ib_drain_rq() call with
1008 * a concurrent request to set the QP state
1009 * to ERROR.
1010 */
1011 rv = siw_rq_flush_wr(qp, wr, bad_wr);
1012 } else {
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] RDMA/siw: Fix SQ/RQ drain logic
2019-10-24 20:38 [bug report] RDMA/siw: Fix SQ/RQ drain logic Dan Carpenter
@ 2019-10-25 8:16 ` Bernard Metzler
2019-10-25 12:34 ` Jason Gunthorpe
0 siblings, 1 reply; 3+ messages in thread
From: Bernard Metzler @ 2019-10-25 8:16 UTC (permalink / raw)
To: Dan Carpenter, Doug Ledford; +Cc: linux-rdma
-----"Dan Carpenter" <dan.carpenter@oracle.com> wrote: -----
>To: bmt@zurich.ibm.com
>From: "Dan Carpenter" <dan.carpenter@oracle.com>
>Date: 10/24/2019 10:39PM
>Cc: linux-rdma@vger.kernel.org
>Subject: [EXTERNAL] [bug report] RDMA/siw: Fix SQ/RQ drain logic
>
>Hello Bernard Metzler,
>
>The patch cf049bb31f71: "RDMA/siw: Fix SQ/RQ drain logic" from Oct 4,
>2019, leads to the following static checker warning:
>
> drivers/infiniband/sw/siw/siw_verbs.c:1079 siw_post_receive()
> error: locking inconsistency. We assume 'read_sem:&qp->state_lock'
>is both locked and unlocked at the start.
>
>drivers/infiniband/sw/siw/siw_verbs.c
> 978 int siw_post_receive(struct ib_qp *base_qp, const struct
>ib_recv_wr *wr,
> 979 const struct ib_recv_wr **bad_wr)
> 980 {
> 981 struct siw_qp *qp = to_siw_qp(base_qp);
> 982 unsigned long flags;
> 983 int rv = 0;
> 984
> 985 if (qp->srq) {
> 986 *bad_wr = wr;
> 987 return -EOPNOTSUPP; /* what else from
>errno.h? */
> 988 }
> 989 if (!qp->kernel_verbs) {
> 990 siw_dbg_qp(qp, "no kernel post_recv for user
>mapped sq\n");
> 991 up_read(&qp->state_lock);
> ^^^^^^^^^^^^^^^^^^^^^^^^
>The patch changes the locking so this isn't held here and should be
>released. Should it be held, though?
Yes, this is a BUG. Thanks very much! There is no qp spinlock to
be held here. I moved that down to state checking. No need to
hold the qp lock before.
Shall I re-send, or can we just amend the patch,
removing that single 'up_read()' line?
Thanks very much,
Bernard
>
> 992 *bad_wr = wr;
> 993 return -EINVAL;
> 994 }
> 995
> 996 /*
> 997 * Try to acquire QP state lock. Must be non-blocking
> 998 * to accommodate kernel clients needs.
> 999 */
> 1000 if (!down_read_trylock(&qp->state_lock)) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> 1001 if (qp->attrs.state == SIW_QP_STATE_ERROR) {
> 1002 /*
> 1003 * ERROR state is final, so we can be
>sure
> 1004 * this state will not change as long
>as the QP
> 1005 * exists.
> 1006 *
> 1007 * This handles an ib_drain_rq() call
>with
> 1008 * a concurrent request to set the QP
>state
> 1009 * to ERROR.
> 1010 */
> 1011 rv = siw_rq_flush_wr(qp, wr, bad_wr);
> 1012 } else {
>
>regards,
>dan carpenter
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] RDMA/siw: Fix SQ/RQ drain logic
2019-10-25 8:16 ` Bernard Metzler
@ 2019-10-25 12:34 ` Jason Gunthorpe
0 siblings, 0 replies; 3+ messages in thread
From: Jason Gunthorpe @ 2019-10-25 12:34 UTC (permalink / raw)
To: Bernard Metzler; +Cc: Dan Carpenter, Doug Ledford, linux-rdma
On Fri, Oct 25, 2019 at 08:16:15AM +0000, Bernard Metzler wrote:
>
> >To: bmt@zurich.ibm.com
> >From: "Dan Carpenter" <dan.carpenter@oracle.com>
> >Date: 10/24/2019 10:39PM
> >Cc: linux-rdma@vger.kernel.org
> >Subject: [EXTERNAL] [bug report] RDMA/siw: Fix SQ/RQ drain logic
> >
> >Hello Bernard Metzler,
> >
> >The patch cf049bb31f71: "RDMA/siw: Fix SQ/RQ drain logic" from Oct 4,
> >2019, leads to the following static checker warning:
> >
> > drivers/infiniband/sw/siw/siw_verbs.c:1079 siw_post_receive()
> > error: locking inconsistency. We assume 'read_sem:&qp->state_lock'
> >is both locked and unlocked at the start.
> >
> >drivers/infiniband/sw/siw/siw_verbs.c
> > 978 int siw_post_receive(struct ib_qp *base_qp, const struct
> >ib_recv_wr *wr,
> > 979 const struct ib_recv_wr **bad_wr)
> > 980 {
> > 981 struct siw_qp *qp = to_siw_qp(base_qp);
> > 982 unsigned long flags;
> > 983 int rv = 0;
> > 984
> > 985 if (qp->srq) {
> > 986 *bad_wr = wr;
> > 987 return -EOPNOTSUPP; /* what else from
> >errno.h? */
> > 988 }
> > 989 if (!qp->kernel_verbs) {
> > 990 siw_dbg_qp(qp, "no kernel post_recv for user
> >mapped sq\n");
> > 991 up_read(&qp->state_lock);
> > ^^^^^^^^^^^^^^^^^^^^^^^^
> >The patch changes the locking so this isn't held here and should be
> >released. Should it be held, though?
>
> Yes, this is a BUG. Thanks very much! There is no qp spinlock to
> be held here. I moved that down to state checking. No need to
> hold the qp lock before.
>
> Shall I re-send, or can we just amend the patch,
> removing that single 'up_read()' line?
You need to send a new patch against for-next to fix this
Jason
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-10-25 12:34 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-10-24 20:38 [bug report] RDMA/siw: Fix SQ/RQ drain logic Dan Carpenter
2019-10-25 8:16 ` Bernard Metzler
2019-10-25 12:34 ` Jason Gunthorpe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox