| Message ID | 4d26a4cfd96bfcb3fe0362a37d595608b09ddd91.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:37AM >Cc: linux-rdma@vger.kernel.org, "Stefan Metzmacher" <metze@samba.org> >Subject: [EXTERNAL] [PATCH 04/31] rdma/siw: let siw_accept() deferr >RDMA_MODE until EVENT_ESTABLISHED > >If we receive an TCP FIN before sending the MPA response >we'll get an error and posted IW_CM_EVENT_CLOSE. >But the IWCM layer didn't receive IW_CM_EVENT_ESTABLISHED >yet and doesn't expect IW_CM_EVENT_CLOSE. Instead >it expects IW_CM_EVENT_CONNECT_REPLY. > >If we stay in SIW_EPSTATE_RECVD_MPAREQ until we posted >IW_CM_EVENT_ESTABLISHED __siw_cep_terminate_upcall() >will use IW_CM_EVENT_CONNECT_REPLY. > >This can be triggered by the following change on the >client: > > --- a/drivers/infiniband/sw/siw/siw_cm.c > +++ b/drivers/infiniband/sw/siw/siw_cm.c > @@ -1507,6 +1507,9 @@ int siw_connect(struct iw_cm_id *id, struct >iw_cm_conn_param *params) > if (rv >= 0) { > rv = siw_cm_queue_work(cep, >SIW_CM_WORK_MPATIMEOUT); > if (!rv) { > + rv = -ECONNRESET; > + msleep_interruptible(100); > + goto error; > siw_dbg_cep(cep, "[QP %u]: exit\n", >qp_id(qp)); > siw_cep_set_free(cep); > return 0; > >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 | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > >diff --git a/drivers/infiniband/sw/siw/siw_cm.c >b/drivers/infiniband/sw/siw/siw_cm.c >index da84686a21fd..145ab6e4e0ed 100644 >--- a/drivers/infiniband/sw/siw/siw_cm.c >+++ b/drivers/infiniband/sw/siw/siw_cm.c >@@ -129,8 +129,10 @@ static void siw_rtr_data_ready(struct sock *sk) > * Failed data processing would have already scheduled > * connection drop. > */ >- if (!qp->rx_stream.rx_suspend) >+ if (!qp->rx_stream.rx_suspend) { > siw_cm_upcall(cep, IW_CM_EVENT_ESTABLISHED, 0); >+ cep->state = SIW_EPSTATE_RDMA_MODE; >+ } > out: > read_unlock(&sk->sk_callback_lock); > if (qp) >@@ -1656,8 +1658,6 @@ int siw_accept(struct iw_cm_id *id, struct >iw_cm_conn_param *params) > /* siw_qp_get(qp) already done by QP lookup */ > cep->qp = qp; > >- cep->state = SIW_EPSTATE_RDMA_MODE; >- > /* Move socket RX/TX under QP control */ > rv = siw_qp_modify(qp, &qp_attrs, > SIW_QP_ATTR_STATE | SIW_QP_ATTR_LLP_HANDLE | >@@ -1683,6 +1683,7 @@ int siw_accept(struct iw_cm_id *id, struct >iw_cm_conn_param *params) > rv = siw_cm_upcall(cep, IW_CM_EVENT_ESTABLISHED, 0); > if (rv) > goto error; >+ cep->state = SIW_EPSTATE_RDMA_MODE; > } > siw_cep_set_free(cep); > >-- >2.25.1 > > Thanks! Reviewed-by: Bernard Metzler <bmt@zurich.ibm.com>
diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c index da84686a21fd..145ab6e4e0ed 100644 --- a/drivers/infiniband/sw/siw/siw_cm.c +++ b/drivers/infiniband/sw/siw/siw_cm.c @@ -129,8 +129,10 @@ static void siw_rtr_data_ready(struct sock *sk) * Failed data processing would have already scheduled * connection drop. */ - if (!qp->rx_stream.rx_suspend) + if (!qp->rx_stream.rx_suspend) { siw_cm_upcall(cep, IW_CM_EVENT_ESTABLISHED, 0); + cep->state = SIW_EPSTATE_RDMA_MODE; + } out: read_unlock(&sk->sk_callback_lock); if (qp) @@ -1656,8 +1658,6 @@ int siw_accept(struct iw_cm_id *id, struct iw_cm_conn_param *params) /* siw_qp_get(qp) already done by QP lookup */ cep->qp = qp; - cep->state = SIW_EPSTATE_RDMA_MODE; - /* Move socket RX/TX under QP control */ rv = siw_qp_modify(qp, &qp_attrs, SIW_QP_ATTR_STATE | SIW_QP_ATTR_LLP_HANDLE | @@ -1683,6 +1683,7 @@ int siw_accept(struct iw_cm_id *id, struct iw_cm_conn_param *params) rv = siw_cm_upcall(cep, IW_CM_EVENT_ESTABLISHED, 0); if (rv) goto error; + cep->state = SIW_EPSTATE_RDMA_MODE; } siw_cep_set_free(cep);
If we receive an TCP FIN before sending the MPA response we'll get an error and posted IW_CM_EVENT_CLOSE. But the IWCM layer didn't receive IW_CM_EVENT_ESTABLISHED yet and doesn't expect IW_CM_EVENT_CLOSE. Instead it expects IW_CM_EVENT_CONNECT_REPLY. If we stay in SIW_EPSTATE_RECVD_MPAREQ until we posted IW_CM_EVENT_ESTABLISHED __siw_cep_terminate_upcall() will use IW_CM_EVENT_CONNECT_REPLY. This can be triggered by the following change on the client: --- a/drivers/infiniband/sw/siw/siw_cm.c +++ b/drivers/infiniband/sw/siw/siw_cm.c @@ -1507,6 +1507,9 @@ int siw_connect(struct iw_cm_id *id, struct iw_cm_conn_param *params) if (rv >= 0) { rv = siw_cm_queue_work(cep, SIW_CM_WORK_MPATIMEOUT); if (!rv) { + rv = -ECONNRESET; + msleep_interruptible(100); + goto error; siw_dbg_cep(cep, "[QP %u]: exit\n", qp_id(qp)); siw_cep_set_free(cep); return 0; 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 | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)