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

[Michal Kalderon <mkalderon@marvell.com>]

> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Thursday, October 3, 2019 7:17 PM
> 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?
We could be under a spin lock when called from several locations in core/iwcm.c
> 
> 
> > +	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.
The core/iwcm takes a refcnt of the QP before calling connect, so it can't be with
refcnt zero

> 
> Also, the xa_load doesn't have any locking around it, so the entire thing looks
> wrong to me.
Since the functions calling it from core/iwcm ( connect / accept ) take a qp
Ref-cnt before the calling there's no risk of the entry being deleted while
xa_load is called

> 
> 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.
This would make qedr more robust, but I think it not needed given the existing
Core/iwcm implementation. 

Thanks,
Michal
> 
> Jason