Skip to content

Commit

Permalink
RDMA/siw: Fix SQ/RQ drain logic
Browse files Browse the repository at this point in the history 
Storage ULPs (e.g. iSER & NVMeOF) use ib_drain_qp() to drain
QP/CQ. Current SIW's own drain routines do not properly wait until all
SQ/RQ elements are completed and reaped from the CQ. This may cause touch
after free issues.  New logic relies on generic
__ib_drain_sq()/__ib_drain_rq() posting a final work request, which SIW
immediately flushes to CQ.

Fixes: 303ae1c ("rdma/siw: application interface")
Link: https://lore.kernel.org/r/20191004125356.20673-1-bmt@zurich.ibm.com
Signed-off-by: Krishnamraju Eraparaju <krishna2@chelsio.com>
Signed-off-by: Bernard Metzler <bmt@zurich.ibm.com>
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
  • Loading branch information
Bernard Metzler authored and Jason Gunthorpe committed Oct 22, 2019
1 parent 5a0d523 commit cf049bb
Show file tree
Hide file tree
Showing 2 changed files with 122 additions and 42 deletions.
20 changes: 0 additions & 20 deletions drivers/infiniband/sw/siw/siw_main.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,24 +249,6 @@ static struct ib_qp *siw_get_base_qp(struct ib_device *base_dev, int id)
return NULL;
}

static void siw_verbs_sq_flush(struct ib_qp *base_qp)
{
struct siw_qp *qp = to_siw_qp(base_qp);

down_write(&qp->state_lock);
siw_sq_flush(qp);
up_write(&qp->state_lock);
}

static void siw_verbs_rq_flush(struct ib_qp *base_qp)
{
struct siw_qp *qp = to_siw_qp(base_qp);

down_write(&qp->state_lock);
siw_rq_flush(qp);
up_write(&qp->state_lock);
}

static const struct ib_device_ops siw_device_ops = {
.owner = THIS_MODULE,
.uverbs_abi_ver = SIW_ABI_VERSION,
Expand All @@ -285,8 +267,6 @@ static const struct ib_device_ops siw_device_ops = {
.destroy_cq = siw_destroy_cq,
.destroy_qp = siw_destroy_qp,
.destroy_srq = siw_destroy_srq,
.drain_rq = siw_verbs_rq_flush,
.drain_sq = siw_verbs_sq_flush,
.get_dma_mr = siw_get_dma_mr,
.get_port_immutable = siw_get_port_immutable,
.iw_accept = siw_accept,
Expand Down
144 changes: 122 additions & 22 deletions drivers/infiniband/sw/siw/siw_verbs.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -687,6 +687,47 @@ static int siw_copy_inline_sgl(const struct ib_send_wr *core_wr,
return bytes;
}

/* Complete SQ WR's without processing */
static int siw_sq_flush_wr(struct siw_qp *qp, const struct ib_send_wr *wr,
const struct ib_send_wr **bad_wr)
{
struct siw_sqe sqe = {};
int rv = 0;

while (wr) {
sqe.id = wr->wr_id;
sqe.opcode = wr->opcode;
rv = siw_sqe_complete(qp, &sqe, 0, SIW_WC_WR_FLUSH_ERR);
if (rv) {
if (bad_wr)
*bad_wr = wr;
break;
}
wr = wr->next;
}
return rv;
}

/* Complete RQ WR's without processing */
static int siw_rq_flush_wr(struct siw_qp *qp, const struct ib_recv_wr *wr,
const struct ib_recv_wr **bad_wr)
{
struct siw_rqe rqe = {};
int rv = 0;

while (wr) {
rqe.id = wr->wr_id;
rv = siw_rqe_complete(qp, &rqe, 0, 0, SIW_WC_WR_FLUSH_ERR);
if (rv) {
if (bad_wr)
*bad_wr = wr;
break;
}
wr = wr->next;
}
return rv;
}

/*
* siw_post_send()
*
Expand All @@ -705,26 +746,54 @@ int siw_post_send(struct ib_qp *base_qp, const struct ib_send_wr *wr,
unsigned long flags;
int rv = 0;

if (wr && !qp->kernel_verbs) {
siw_dbg_qp(qp, "wr must be empty for user mapped sq\n");
*bad_wr = wr;
return -EINVAL;
}

/*
* Try to acquire QP state lock. Must be non-blocking
* to accommodate kernel clients needs.
*/
if (!down_read_trylock(&qp->state_lock)) {
*bad_wr = wr;
siw_dbg_qp(qp, "QP locked, state %d\n", qp->attrs.state);
return -ENOTCONN;
if (qp->attrs.state == SIW_QP_STATE_ERROR) {
/*
* ERROR state is final, so we can be sure
* this state will not change as long as the QP
* exists.
*
* This handles an ib_drain_sq() call with
* a concurrent request to set the QP state
* to ERROR.
*/
rv = siw_sq_flush_wr(qp, wr, bad_wr);
} else {
siw_dbg_qp(qp, "QP locked, state %d\n",
qp->attrs.state);
*bad_wr = wr;
rv = -ENOTCONN;
}
return rv;
}
if (unlikely(qp->attrs.state != SIW_QP_STATE_RTS)) {
if (qp->attrs.state == SIW_QP_STATE_ERROR) {
/*
* Immediately flush this WR to CQ, if QP
* is in ERROR state. SQ is guaranteed to
* be empty, so WR complets in-order.
*
* Typically triggered by ib_drain_sq().
*/
rv = siw_sq_flush_wr(qp, wr, bad_wr);
} else {
siw_dbg_qp(qp, "QP out of state %d\n",
qp->attrs.state);
*bad_wr = wr;
rv = -ENOTCONN;
}
up_read(&qp->state_lock);
*bad_wr = wr;
siw_dbg_qp(qp, "QP out of state %d\n", qp->attrs.state);
return -ENOTCONN;
}
if (wr && !qp->kernel_verbs) {
siw_dbg_qp(qp, "wr must be empty for user mapped sq\n");
up_read(&qp->state_lock);
*bad_wr = wr;
return -EINVAL;
return rv;
}
spin_lock_irqsave(&qp->sq_lock, flags);

Expand Down Expand Up @@ -919,24 +988,55 @@ int siw_post_receive(struct ib_qp *base_qp, const struct ib_recv_wr *wr,
*bad_wr = wr;
return -EOPNOTSUPP; /* what else from errno.h? */
}
/*
* Try to acquire QP state lock. Must be non-blocking
* to accommodate kernel clients needs.
*/
if (!down_read_trylock(&qp->state_lock)) {
*bad_wr = wr;
return -ENOTCONN;
}
if (!qp->kernel_verbs) {
siw_dbg_qp(qp, "no kernel post_recv for user mapped sq\n");
up_read(&qp->state_lock);
*bad_wr = wr;
return -EINVAL;
}

/*
* Try to acquire QP state lock. Must be non-blocking
* to accommodate kernel clients needs.
*/
if (!down_read_trylock(&qp->state_lock)) {
if (qp->attrs.state == SIW_QP_STATE_ERROR) {
/*
* ERROR state is final, so we can be sure
* this state will not change as long as the QP
* exists.
*
* This handles an ib_drain_rq() call with
* a concurrent request to set the QP state
* to ERROR.
*/
rv = siw_rq_flush_wr(qp, wr, bad_wr);
} else {
siw_dbg_qp(qp, "QP locked, state %d\n",
qp->attrs.state);
*bad_wr = wr;
rv = -ENOTCONN;
}
return rv;
}
if (qp->attrs.state > SIW_QP_STATE_RTS) {
if (qp->attrs.state == SIW_QP_STATE_ERROR) {
/*
* Immediately flush this WR to CQ, if QP
* is in ERROR state. RQ is guaranteed to
* be empty, so WR complets in-order.
*
* Typically triggered by ib_drain_rq().
*/
rv = siw_rq_flush_wr(qp, wr, bad_wr);
} else {
siw_dbg_qp(qp, "QP out of state %d\n",
qp->attrs.state);
*bad_wr = wr;
rv = -ENOTCONN;
}
up_read(&qp->state_lock);
*bad_wr = wr;
return -EINVAL;
return rv;
}
/*
* Serialize potentially multiple producers.
Expand Down

0 comments on commit cf049bb

Please sign in to comment.