On Wed, Aug 08, 2018 at 06:50:17PM +0000, Kalderon, Michal wrote:
> > From: Jason Gunthorpe <jgg@xxxxxxxx>
> > Sent: Tuesday, August 07, 2018 5:28 PM
> > On Tue, Aug 07, 2018 at 10:02:03AM +0000, Kalderon, Michal wrote:
> > > > From: linux-rdma-owner@xxxxxxxxxxxxxxx <linux-rdma-
> > > > owner@xxxxxxxxxxxxxxx> On Behalf Of Jason Gunthorpe
> > > >
> > > > On Sun, Jul 29, 2018 at 01:16:10PM +0300, Yuval Bason wrote:
> > > >
> > > > > diff --git a/drivers/infiniband/hw/qedr/qedr_iw_cm.c
> > > > > b/drivers/infiniband/hw/qedr/qedr_iw_cm.c
> > > > > index 26dc374..505fa36 100644
> > > > > +++ b/drivers/infiniband/hw/qedr/qedr_iw_cm.c
> > > > > @@ -491,7 +491,7 @@ int qedr_iw_connect(struct iw_cm_id *cm_id,
> > > > struct iw_cm_conn_param *conn_param)
> > > > > int rc = 0;
> > > > > int i;
> > > > >
> > > > > - qp = idr_find(&dev->qpidr, conn_param->qpn);
> > > > > + qp = idr_find(&dev->qpidr.idr, conn_param->qpn);
> > > > >
> > > > > laddr = (struct sockaddr_in *)&cm_id->m_local_addr;
> > > > > raddr = (struct sockaddr_in *)&cm_id->m_remote_addr; @@
> > > > > -679,7
> > > > > +679,7 @@ int qedr_iw_accept(struct iw_cm_id *cm_id, struct
> > > > > iw_cm_conn_param *conn_param)
> > > >
> > > > [..]
> > > >
> > > > > @@ -2279,8 +2275,9 @@ int qedr_destroy_qp(struct ib_qp *ibqp)
> > > > >
> > > > > qedr_free_qp_resources(dev, qp);
> > > > >
> > > > > - if (atomic_dec_and_test(&qp->refcnt)) {
> > > > > - qedr_idr_remove(dev, qp->qp_id);
> > > > > + if (atomic_dec_and_test(&qp->refcnt) &&
> > > > > + rdma_protocol_iwarp(&dev->ibdev, 1)) {
> > > > > + qedr_idr_remove(dev, &dev->qpidr, qp->qp_id);
> > > > > kfree(qp);
> > > >
> > > > So what prevents kfree from racing with the idr_find above? Looks
> > > > like noting.
> > >
> > > Hi Jason,
> >
> > > This is the QP idr, and the use case here is for iWARP rdma connection
> > > flow. The flows here are part of the rdma-cm for iWARP and the races
> > > here are dealt with by locking and ref-counting in the rdma-cm.
> >
> > How?
> Specifically for the kfree race with idr_find the description below
> explains how it can't occur. But for the race of the idr being
> modified by adding/removing a different QP and performing idr_find,
> I agree, we need an rcu_read_lock around the idr_find.
This needs more than rcu_read_lock.
> The idr_find above that you're referring to won't race with the
> kfree(qp) because qedr_idr_remove & kfree(qp) will be called only
> when the qp->refcnt reaches zero
This pattern is to be implemented using a 'kref', it needs to change,
and then it will become clearer what needs to be done..
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
