| Message ID | 05e4a83a1b65d0cf47f4d0501f6dd081bce75602.1620343860.git.metze@samba.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | rdma/siw: fix a lot of deadlocks and use after free bugs | expand |
-----"Stefan Metzmacher" <metze@samba.org> wrote: ----- >To: "Bernard Metzler" <bmt@zurich.ibm.com> >From: "Stefan Metzmacher" <metze@samba.org> >Date: 05/07/2021 01:39AM >Cc: linux-rdma@vger.kernel.org, "Stefan Metzmacher" <metze@samba.org> >Subject: [EXTERNAL] [PATCH 24/31] rdma/siw: do the full >disassociation of cep and qp in siw_qp_llp_close() > >It's much clearer to drop the references on both sides and reset the >cross referencing pointers in one place. I makes the caller much >saner >and understandable. I think it is cleaner if the qp code does not alter cep private pointers as it is in the current code. > >Fixes: 6c52fdc244b5 ("rdma/siw: connection management") >Signed-off-by: Stefan Metzmacher <metze@samba.org> >Cc: Bernard Metzler <bmt@zurich.ibm.com> >Cc: linux-rdma@vger.kernel.org >--- > drivers/infiniband/sw/siw/siw_cm.c | 2 -- > drivers/infiniband/sw/siw/siw_qp.c | 3 +++ > 2 files changed, 3 insertions(+), 2 deletions(-) > >diff --git a/drivers/infiniband/sw/siw/siw_cm.c >b/drivers/infiniband/sw/siw/siw_cm.c >index 7fd67499f1d3..31135d877d41 100644 >--- a/drivers/infiniband/sw/siw/siw_cm.c >+++ b/drivers/infiniband/sw/siw/siw_cm.c >@@ -1240,10 +1240,8 @@ static void siw_cm_work_handler(struct >work_struct *w) > siw_cep_set_free(cep); > > siw_qp_llp_close(qp); >- siw_qp_put(qp); > > siw_cep_set_inuse(cep); >- cep->qp = NULL; > siw_qp_put(qp); > } > if (cep->sock) { >diff --git a/drivers/infiniband/sw/siw/siw_qp.c >b/drivers/infiniband/sw/siw/siw_qp.c >index ddb2e66f9f13..badb065eb9b1 100644 >--- a/drivers/infiniband/sw/siw/siw_qp.c >+++ b/drivers/infiniband/sw/siw/siw_qp.c >@@ -166,8 +166,11 @@ void siw_qp_llp_close(struct siw_qp *qp) > * Dereference closing CEP > */ > if (qp->cep) { >+ BUG_ON(qp->cep->qp != qp); Don't introduce BUG() >+ qp->cep->qp = NULL; Only the CM code should change that pointer. > siw_cep_put(qp->cep); > qp->cep = NULL; >+ siw_qp_put(qp); > } > > up_write(&qp->state_lock); >-- >2.25.1 > >
diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c index 7fd67499f1d3..31135d877d41 100644 --- a/drivers/infiniband/sw/siw/siw_cm.c +++ b/drivers/infiniband/sw/siw/siw_cm.c @@ -1240,10 +1240,8 @@ static void siw_cm_work_handler(struct work_struct *w) siw_cep_set_free(cep); siw_qp_llp_close(qp); - siw_qp_put(qp); siw_cep_set_inuse(cep); - cep->qp = NULL; siw_qp_put(qp); } if (cep->sock) { diff --git a/drivers/infiniband/sw/siw/siw_qp.c b/drivers/infiniband/sw/siw/siw_qp.c index ddb2e66f9f13..badb065eb9b1 100644 --- a/drivers/infiniband/sw/siw/siw_qp.c +++ b/drivers/infiniband/sw/siw/siw_qp.c @@ -166,8 +166,11 @@ void siw_qp_llp_close(struct siw_qp *qp) * Dereference closing CEP */ if (qp->cep) { + BUG_ON(qp->cep->qp != qp); + qp->cep->qp = NULL; siw_cep_put(qp->cep); qp->cep = NULL; + siw_qp_put(qp); } up_write(&qp->state_lock);
It's much clearer to drop the references on both sides and reset the cross referencing pointers in one place. I makes the caller much saner and understandable. Fixes: 6c52fdc244b5 ("rdma/siw: connection management") Signed-off-by: Stefan Metzmacher <metze@samba.org> Cc: Bernard Metzler <bmt@zurich.ibm.com> Cc: linux-rdma@vger.kernel.org --- drivers/infiniband/sw/siw/siw_cm.c | 2 -- drivers/infiniband/sw/siw/siw_qp.c | 3 +++ 2 files changed, 3 insertions(+), 2 deletions(-)