Re: [[PATCH v2 for-next]] RDMA/siw: Fix SQ/RQ drain logic

[Leon Romanovsky <leon@kernel.org>]

On Wed, Oct 02, 2019 at 04:38:58PM +0200, Bernard Metzler wrote:
> 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: 303ae1cdfdf7 ("rdma/siw: application interface")
> Signed-off-by: Krishnamraju Eraparaju <krishna2@chelsio.com>
> Signed-off-by: Bernard Metzler <bmt@zurich.ibm.com>
> ---
> v1 -> v2:
> - Accept SQ and RQ work requests, if QP is in ERROR
>   state. In that case, immediately flush WR's to CQ.
>   This already provides needed functionality to
>   support ib_drain_sq()/ib_drain_rq() without extra
>   state checking in the fast path.
>
>  drivers/infiniband/sw/siw/siw_main.c  |  20 -----
>  drivers/infiniband/sw/siw/siw_verbs.c | 103 +++++++++++++++++++++-----
>  2 files changed, 86 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/infiniband/sw/siw/siw_main.c b/drivers/infiniband/sw/siw/siw_main.c
> index 05a92f997f60..fb01407a310f 100644
> --- a/drivers/infiniband/sw/siw/siw_main.c
> +++ b/drivers/infiniband/sw/siw/siw_main.c
> @@ -248,24 +248,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,
> @@ -284,8 +266,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,
> diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c
> index 869e02b69a01..40e68e7a4f39 100644
> --- a/drivers/infiniband/sw/siw/siw_verbs.c
> +++ b/drivers/infiniband/sw/siw/siw_verbs.c
> @@ -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()
>   *
> @@ -705,6 +746,12 @@ 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) {

It is not related to this specific patch, but all siw "kernel_verbs"
should go, we have standard way to distinguish between kernel and user
verbs.

Thanks