> 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.
>
> And this code above is total garbage:
>
> if (qp->ep) {
> int wait_count = 1;
>
> while (qp->ep->during_connect) {
> DP_DEBUG(dev, QEDR_MSG_QP,
> "Still in during connect/accept\n");
>
> msleep(100);
> if (wait_count++ > 200) {
> DP_NOTICE(dev,
> "during connect timeout\n");
> break;
> }
> }
> }
>
> That is just not an acceptable way to manage concurrency, it is not allowed to
> just read unlocked some random variable.
>
> As is the unlocked use of qp->ep.
>
Thanks for pointing this out. Note taken and we will revisit this code.
This code however is not part of the SRQ patch and has already been reviewed and accepted.
This is to handle the case between qedr_destroy_qp being called before the
asynchronous completion of qedr_iw_connect has completed (related to
the rdma-cm iWARP connection flow ).
> I think you need to send some kind of series to fix the races with this idr
> before going ahead with anything else.
We'll send v5 of the SRQ patch series after handling Mark's latest
comments.
Please re-consider applying the SRQ series and not connecting it to
the QP iWARP connection flow.
Thanks,
Michal
>
> 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
--
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
