RDMA/siw: Fix SQ/RQ drain logic

To: [email protected]
From: "Dan Carpenter" <[email protected]>
Date: 10/24/2019 10:39PM
Cc: [email protected]
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?

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

Quoted 33 lines

To: [email protected]
From: "Dan Carpenter" <[email protected]>
Date: 10/24/2019 10:39PM
Cc: [email protected]
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");

Quoted 4 lines

  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?