Re: [PATCH rdma-next 1/2] RDMA/qedr: Fix synchronization methods and memory leaks in qedr

[Jason Gunthorpe <jgg@ziepe.ca>]

On Thu, Oct 03, 2019 at 03:03:41PM +0300, Michal Kalderon wrote:

> diff --git a/drivers/infiniband/hw/qedr/qedr_iw_cm.c b/drivers/infiniband/hw/qedr/qedr_iw_cm.c
> index 22881d4442b9..ebc6bc25a0e2 100644
> +++ b/drivers/infiniband/hw/qedr/qedr_iw_cm.c
> @@ -79,6 +79,28 @@ qedr_fill_sockaddr6(const struct qed_iwarp_cm_info *cm_info,
>  	}
>  }
>  
> +static void qedr_iw_free_qp(struct kref *ref)
> +{
> +	struct qedr_qp *qp = container_of(ref, struct qedr_qp, refcnt);
> +
> +	xa_erase_irq(&qp->dev->qps, qp->qp_id);

why is it _irq? Where are we in an irq when using the xa_lock on this xarray?


> +	kfree(qp);

[..]

> @@ -516,8 +548,10 @@ int qedr_iw_connect(struct iw_cm_id *cm_id, struct iw_cm_conn_param *conn_param)
>  		return -ENOMEM;
>  
>  	ep->dev = dev;
> +	kref_init(&ep->refcnt);
> +
> +	kref_get(&qp->refcnt);

Here 'qp' comes out of an xa_load, but the QP is still visible in the
xarray with a 0 refcount, so this is invalid.

Also, the xa_load doesn't have any locking around it, so the entire
thing looks wrong to me.

Most probably you want to hold the xa_lock during xa_load and then use
a kref_get_not_zero - failure of the get also means the qp is not in
the xarray

Or rework things so the qp is removed from the xarray earlier, I'm not
sure.

Jason