[PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs

[PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs

[Stefan Metzmacher]

Hi Bernard,

while testing with my smbdirect driver I hit a lot of
bugs in the siw.ko driver. They all cause problems where
the siw driver was not able to unload anymore and I had to
reboot the machine.

I implemented:
- a non blocking connect
- fixed a lot of bugs where siw_cep_put() was called too often.
- fixed bugs where siw_cm_upcall() confused the core IWCM logic

I have some more changes to follow, but I wanted to send them
finally out after having them one and a half year sitting in some
private branch...

Stefan Metzmacher (31):
  rdma/siw: fix warning in siw_proc_send()
  rdma/siw: call smp_mb() after mem->stag_valid = 0 in
    siw_invalidate_stag() too
  rdma/siw: remove superfluous siw_cep_put() from siw_connect() error
    path
  rdma/siw: let siw_accept() deferr RDMA_MODE until EVENT_ESTABLISHED
  rdma/siw: make use of kernel_{bind,connect,listen}()
  rdma/siw: make siw_cm_upcall() a noop without valid 'id'
  rdma/siw: split out a __siw_cep_terminate_upcall() function
  rdma/siw: use __siw_cep_terminate_upcall() for indirect
    SIW_CM_WORK_CLOSE_LLP
  rdma/siw: use __siw_cep_terminate_upcall() for SIW_CM_WORK_PEER_CLOSE
  rdma/siw: use __siw_cep_terminate_upcall() for SIW_CM_WORK_MPATIMEOUT
  rdma/siw: introduce SIW_EPSTATE_ACCEPTING/REJECTING for
    rdma_accept/rdma_reject
  rdma/siw: add some debugging of state and sk_state to the teardown
    process
  rdma/siw: handle SIW_EPSTATE_CONNECTING in
    __siw_cep_terminate_upcall()
  rdma/siw: let siw_connect() set AWAIT_MPAREP before
    siw_send_mpareqrep()
  rdma/siw: create a temporary copy of private data
  rdma/siw: use error and out logic at the end of siw_connect()
  rdma/siw: start mpa timer before calling siw_send_mpareqrep()
  rdma/siw: call the blocking kernel_bindconnect() just before
    siw_send_mpareqrep()
  rdma/siw: split out a __siw_cep_close() function
  rdma/siw: implement non-blocking connect.
  rdma/siw: let siw_listen_address() call siw_cep_alloc() first
  rdma/siw: let siw_listen_address() call siw_cep_set_inuse() early
  rdma/siw: make use of __siw_cep_close() in siw_accept()
  rdma/siw: do the full disassociation of cep and qp in
    siw_qp_llp_close()
  rdma/siw: fix double siw_cep_put() in siw_cm_work_handler()
  rdma/siw: make use of __siw_cep_close() in siw_cm_work_handler()
  rdma/siw: fix the "close" logic in siw_qp_cm_drop()
  rdma/siw: make use of __siw_cep_close() in siw_qp_cm_drop()
  rdma/siw: make use of __siw_cep_close() in siw_reject()
  rdma/siw: make use of __siw_cep_close() in siw_listen_address()
  rdma/siw: make use of __siw_cep_close() in siw_drop_listeners()

 drivers/infiniband/sw/siw/siw_cm.c    | 537 +++++++++++++++-----------
 drivers/infiniband/sw/siw/siw_cm.h    |   3 +
 drivers/infiniband/sw/siw/siw_mem.c   |   2 +
 drivers/infiniband/sw/siw/siw_qp.c    |   3 +
 drivers/infiniband/sw/siw/siw_qp_rx.c |   2 +-
 5 files changed, 316 insertions(+), 231 deletions(-)

-- 
2.25.1

[PATCH 01/31] rdma/siw: fix warning in siw_proc_send()

[Stefan Metzmacher]

  CC [M]  drivers/infiniband/sw/siw/siw_qp_rx.o
In file included from ./include/linux/wait.h:9:0,
                 from ./include/linux/net.h:19,
                 from drivers/infiniband/sw/siw/siw_qp_rx.c:8:
drivers/infiniband/sw/siw/siw_qp_rx.c: In function ‘siw_proc_send’:
./include/linux/spinlock.h:288:3: warning: ‘flags’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   _raw_spin_unlock_irqrestore(lock, flags); \
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/infiniband/sw/siw/siw_qp_rx.c:335:16: note: ‘flags’ was declared here
  unsigned long flags;

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_qp_rx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/sw/siw/siw_qp_rx.c b/drivers/infiniband/sw/siw/siw_qp_rx.c
index 60116f20653c..0170c05d2cc3 100644
--- a/drivers/infiniband/sw/siw/siw_qp_rx.c
+++ b/drivers/infiniband/sw/siw/siw_qp_rx.c
@@ -333,7 +333,7 @@ static struct siw_wqe *siw_rqe_get(struct siw_qp *qp)
 	struct siw_srq *srq;
 	struct siw_wqe *wqe = NULL;
 	bool srq_event = false;
-	unsigned long flags;
+	unsigned long flags = 0;
 
 	srq = qp->srq;
 	if (srq) {
-- 
2.25.1

[PATCH 02/31] rdma/siw: call smp_mb() after mem->stag_valid = 0 in siw_invalidate_stag() too

[Stefan Metzmacher]

We already do the same in siw_mr_drop_mem().

Fixes: 2251334dcac9 ("rdma/siw: application buffer 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_mem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/infiniband/sw/siw/siw_mem.c b/drivers/infiniband/sw/siw/siw_mem.c
index 61c17db70d65..8596ce1ef5a3 100644
--- a/drivers/infiniband/sw/siw/siw_mem.c
+++ b/drivers/infiniband/sw/siw/siw_mem.c
@@ -309,6 +309,8 @@ int siw_invalidate_stag(struct ib_pd *pd, u32 stag)
 	 * state if invalidation is requested. So no state check here.
 	 */
 	mem->stag_valid = 0;
+	/* make STag invalid visible asap */
+	smp_mb();
 
 	siw_dbg_pd(pd, "STag 0x%08x now invalid\n", stag);
 out:
-- 
2.25.1

[PATCH 03/31] rdma/siw: remove superfluous siw_cep_put() from siw_connect() error path

[Stefan Metzmacher]

The following change demonstrate the bug:

    --- 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;

That change triggers the WARN_ON() in siw_cep_put().

As there's no siw_cep_get() arround id->add_ref()
I removed the siw_cep_put() following id->rem_ref().

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 | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 7a5ed86ffc9f..da84686a21fd 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -1494,7 +1494,6 @@ int siw_connect(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 
 		cep->cm_id = NULL;
 		id->rem_ref(id);
-		siw_cep_put(cep);
 
 		qp->cep = NULL;
 		siw_cep_put(cep);
-- 
2.25.1

[PATCH 04/31] rdma/siw: let siw_accept() deferr RDMA_MODE until EVENT_ESTABLISHED

[Stefan Metzmacher]

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

[PATCH 05/31] rdma/siw: make use of kernel_{bind,connect,listen}()

[Stefan Metzmacher]

That's nicer than dereferencing socket structures.

This prepares making rdma_connect()/siw_connect() non-blocking
in order to avoid deadlocks in the callers.

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 | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 145ab6e4e0ed..e21cde84306e 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -1319,11 +1319,11 @@ static int kernel_bindconnect(struct socket *s, struct sockaddr *laddr,
 			return rv;
 	}
 
-	rv = s->ops->bind(s, laddr, size);
+	rv = kernel_bind(s, laddr, size);
 	if (rv < 0)
 		return rv;
 
-	rv = s->ops->connect(s, raddr, size, flags);
+	rv = kernel_connect(s, raddr, size, flags);
 
 	return rv < 0 ? rv : 0;
 }
@@ -1787,8 +1787,8 @@ int siw_create_listen(struct iw_cm_id *id, int backlog)
 		if (ipv4_is_zeronet(laddr->sin_addr.s_addr))
 			s->sk->sk_bound_dev_if = sdev->netdev->ifindex;
 
-		rv = s->ops->bind(s, (struct sockaddr *)laddr,
-				  sizeof(struct sockaddr_in));
+		rv = kernel_bind(s, (struct sockaddr *)laddr,
+				 sizeof(struct sockaddr_in));
 	} else {
 		struct sockaddr_in6 *laddr = &to_sockaddr_in6(id->local_addr);
 
@@ -1805,8 +1805,8 @@ int siw_create_listen(struct iw_cm_id *id, int backlog)
 		if (ipv6_addr_any(&laddr->sin6_addr))
 			s->sk->sk_bound_dev_if = sdev->netdev->ifindex;
 
-		rv = s->ops->bind(s, (struct sockaddr *)laddr,
-				  sizeof(struct sockaddr_in6));
+		rv = kernel_bind(s, (struct sockaddr *)laddr,
+				 sizeof(struct sockaddr_in6));
 	}
 	if (rv) {
 		siw_dbg(id->device, "socket bind error: %d\n", rv);
@@ -1826,7 +1826,7 @@ int siw_create_listen(struct iw_cm_id *id, int backlog)
 			rv, backlog);
 		goto error;
 	}
-	rv = s->ops->listen(s, backlog);
+	rv = kernel_listen(s, backlog);
 	if (rv) {
 		siw_dbg(id->device, "listen error %d\n", rv);
 		goto error;
-- 
2.25.1

[PATCH 06/31] rdma/siw: make siw_cm_upcall() a noop without valid 'id'

[Stefan Metzmacher]

This will simplify the callers.

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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index e21cde84306e..2cc2863bd427 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -324,6 +324,9 @@ static int siw_cm_upcall(struct siw_cep *cep, enum iw_cm_event_type reason,
 	} else {
 		id = cep->cm_id;
 	}
+	if (id == NULL)
+		return status;
+
 	/* Signal IRD and ORD */
 	if (reason == IW_CM_EVENT_ESTABLISHED ||
 	    reason == IW_CM_EVENT_CONNECT_REPLY) {
-- 
2.25.1

[PATCH 07/31] rdma/siw: split out a __siw_cep_terminate_upcall() function

[Stefan Metzmacher]

There are multiple places where should have the same logic.
Having one helper function to be used in all places
makes it easier to extended the logic.

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 | 53 ++++++++++++++++++------------
 1 file changed, 32 insertions(+), 21 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 2cc2863bd427..c91a74271b9b 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -103,6 +103,37 @@ static void siw_socket_disassoc(struct socket *s)
 	}
 }
 
+/*
+ * The caller needs to deal with siw_cep_set_inuse()
+ * and siw_cep_set_free()
+ */
+static void __siw_cep_terminate_upcall(struct siw_cep *cep,
+				       int reply_status)
+{
+	if (cep->qp && cep->qp->term_info.valid)
+		siw_send_terminate(cep->qp);
+
+	switch (cep->state) {
+	case SIW_EPSTATE_AWAIT_MPAREP:
+		siw_cm_upcall(cep, IW_CM_EVENT_CONNECT_REPLY,
+			      reply_status);
+		break;
+
+	case SIW_EPSTATE_RDMA_MODE:
+		siw_cm_upcall(cep, IW_CM_EVENT_CLOSE, 0);
+		break;
+
+	case SIW_EPSTATE_IDLE:
+	case SIW_EPSTATE_LISTENING:
+	case SIW_EPSTATE_CONNECTING:
+	case SIW_EPSTATE_AWAIT_MPAREQ:
+	case SIW_EPSTATE_RECVD_MPAREQ:
+	case SIW_EPSTATE_CLOSED:
+	default:
+		break;
+	}
+}
+
 static void siw_rtr_data_ready(struct sock *sk)
 {
 	struct siw_cep *cep;
@@ -395,29 +426,9 @@ void siw_qp_cm_drop(struct siw_qp *qp, int schedule)
 		}
 		siw_dbg_cep(cep, "immediate close, state %d\n", cep->state);
 
-		if (qp->term_info.valid)
-			siw_send_terminate(qp);
+		__siw_cep_terminate_upcall(cep, -EINVAL);
 
 		if (cep->cm_id) {
-			switch (cep->state) {
-			case SIW_EPSTATE_AWAIT_MPAREP:
-				siw_cm_upcall(cep, IW_CM_EVENT_CONNECT_REPLY,
-					      -EINVAL);
-				break;
-
-			case SIW_EPSTATE_RDMA_MODE:
-				siw_cm_upcall(cep, IW_CM_EVENT_CLOSE, 0);
-				break;
-
-			case SIW_EPSTATE_IDLE:
-			case SIW_EPSTATE_LISTENING:
-			case SIW_EPSTATE_CONNECTING:
-			case SIW_EPSTATE_AWAIT_MPAREQ:
-			case SIW_EPSTATE_RECVD_MPAREQ:
-			case SIW_EPSTATE_CLOSED:
-			default:
-				break;
-			}
 			cep->cm_id->rem_ref(cep->cm_id);
 			cep->cm_id = NULL;
 			siw_cep_put(cep);
-- 
2.25.1

[PATCH 08/31] rdma/siw: use __siw_cep_terminate_upcall() for indirect SIW_CM_WORK_CLOSE_LLP

[Stefan Metzmacher]

Both code paths from siw_qp_cm_drop() should use the same logic.

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 | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index c91a74271b9b..b7e7f637bd03 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -1071,11 +1071,7 @@ static void siw_cm_work_handler(struct work_struct *w)
 		/*
 		 * QP scheduled LLP close
 		 */
-		if (cep->qp && cep->qp->term_info.valid)
-			siw_send_terminate(cep->qp);
-
-		if (cep->cm_id)
-			siw_cm_upcall(cep, IW_CM_EVENT_CLOSE, 0);
+		__siw_cep_terminate_upcall(cep, -EINVAL);
 
 		release_cep = 1;
 		break;
-- 
2.25.1

[PATCH 09/31] rdma/siw: use __siw_cep_terminate_upcall() for SIW_CM_WORK_PEER_CLOSE

[Stefan Metzmacher]

It's easier to have generic logic in just one place.

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 | 89 +++++++++++++++++-------------
 1 file changed, 50 insertions(+), 39 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index b7e7f637bd03..5338be450285 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -110,26 +110,67 @@ static void siw_socket_disassoc(struct socket *s)
 static void __siw_cep_terminate_upcall(struct siw_cep *cep,
 				       int reply_status)
 {
-	if (cep->qp && cep->qp->term_info.valid)
-		siw_send_terminate(cep->qp);
+	bool suspended = false;
+
+	if (cep->qp) {
+		struct siw_qp *qp = cep->qp;
+
+		if (qp->term_info.valid)
+			siw_send_terminate(qp);
+
+		if (qp->rx_stream.rx_suspend || qp->tx_ctx.tx_suspend)
+			suspended = true;
+	} else {
+		suspended = true;
+	}
 
 	switch (cep->state) {
 	case SIW_EPSTATE_AWAIT_MPAREP:
+		/*
+		 * MPA reply not received, but connection drop,
+		 * or timeout.
+		 */
 		siw_cm_upcall(cep, IW_CM_EVENT_CONNECT_REPLY,
 			      reply_status);
 		break;
 
 	case SIW_EPSTATE_RDMA_MODE:
+		/*
+		 * NOTE: IW_CM_EVENT_DISCONNECT is given just
+		 *       to transition IWCM into CLOSING.
+		 */
+		WARN(!suspended, "SIW_EPSTATE_RDMA_MODE called without suspended\n");
+		siw_cm_upcall(cep, IW_CM_EVENT_DISCONNECT, 0);
 		siw_cm_upcall(cep, IW_CM_EVENT_CLOSE, 0);
 		break;
 
+	case SIW_EPSTATE_RECVD_MPAREQ:
+		/*
+		 * Wait for the ulp/CM to call accept/reject
+		 */
+		siw_dbg_cep(cep, "mpa req recvd, wait for ULP\n");
+		WARN(!suspended, "SIW_EPSTATE_RECVD_MPAREQ called without suspended\n");
+		break;
+
+	case SIW_EPSTATE_AWAIT_MPAREQ:
+		/*
+		 * Socket close before MPA request received.
+		 */
+		siw_dbg_cep(cep, "no mpareq: drop listener\n");
+		if (cep->listen_cep)
+			siw_cep_put(cep->listen_cep);
+		cep->listen_cep = NULL;
+		break;
+
 	case SIW_EPSTATE_IDLE:
 	case SIW_EPSTATE_LISTENING:
 	case SIW_EPSTATE_CONNECTING:
-	case SIW_EPSTATE_AWAIT_MPAREQ:
-	case SIW_EPSTATE_RECVD_MPAREQ:
 	case SIW_EPSTATE_CLOSED:
 	default:
+		/*
+		 * for other states there is no connection
+		 * known to the IWCM.
+		 */
 		break;
 	}
 }
@@ -1077,41 +1118,11 @@ static void siw_cm_work_handler(struct work_struct *w)
 		break;
 
 	case SIW_CM_WORK_PEER_CLOSE:
-		if (cep->cm_id) {
-			if (cep->state == SIW_EPSTATE_AWAIT_MPAREP) {
-				/*
-				 * MPA reply not received, but connection drop
-				 */
-				siw_cm_upcall(cep, IW_CM_EVENT_CONNECT_REPLY,
-					      -ECONNRESET);
-			} else if (cep->state == SIW_EPSTATE_RDMA_MODE) {
-				/*
-				 * NOTE: IW_CM_EVENT_DISCONNECT is given just
-				 *       to transition IWCM into CLOSING.
-				 */
-				siw_cm_upcall(cep, IW_CM_EVENT_DISCONNECT, 0);
-				siw_cm_upcall(cep, IW_CM_EVENT_CLOSE, 0);
-			}
-			/*
-			 * for other states there is no connection
-			 * known to the IWCM.
-			 */
-		} else {
-			if (cep->state == SIW_EPSTATE_RECVD_MPAREQ) {
-				/*
-				 * Wait for the ulp/CM to call accept/reject
-				 */
-				siw_dbg_cep(cep,
-					    "mpa req recvd, wait for ULP\n");
-			} else if (cep->state == SIW_EPSTATE_AWAIT_MPAREQ) {
-				/*
-				 * Socket close before MPA request received.
-				 */
-				siw_dbg_cep(cep, "no mpareq: drop listener\n");
-				siw_cep_put(cep->listen_cep);
-				cep->listen_cep = NULL;
-			}
-		}
+		/*
+		 * Peer closed the connection: TCP_CLOSE*
+		 */
+		__siw_cep_terminate_upcall(cep, -ECONNRESET);
+
 		release_cep = 1;
 		break;
 
-- 
2.25.1

[PATCH 10/31] rdma/siw: use __siw_cep_terminate_upcall() for SIW_CM_WORK_MPATIMEOUT

[Stefan Metzmacher]

It's easier to have generic logic in just one place.

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 | 31 ++++++++----------------------
 1 file changed, 8 insertions(+), 23 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 5338be450285..d03c7a66c6d1 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -1127,31 +1127,16 @@ static void siw_cm_work_handler(struct work_struct *w)
 		break;
 
 	case SIW_CM_WORK_MPATIMEOUT:
+		/*
+		 * MPA request timed out:
+		 * Hide any partially received private data and signal
+		 * timeout
+		 */
 		cep->mpa_timer = NULL;
+		cep->mpa.hdr.params.pd_len = 0;
+		__siw_cep_terminate_upcall(cep, -ETIMEDOUT);
 
-		if (cep->state == SIW_EPSTATE_AWAIT_MPAREP) {
-			/*
-			 * MPA request timed out:
-			 * Hide any partially received private data and signal
-			 * timeout
-			 */
-			cep->mpa.hdr.params.pd_len = 0;
-
-			if (cep->cm_id)
-				siw_cm_upcall(cep, IW_CM_EVENT_CONNECT_REPLY,
-					      -ETIMEDOUT);
-			release_cep = 1;
-
-		} else if (cep->state == SIW_EPSTATE_AWAIT_MPAREQ) {
-			/*
-			 * No MPA request received after peer TCP stream setup.
-			 */
-			if (cep->listen_cep) {
-				siw_cep_put(cep->listen_cep);
-				cep->listen_cep = NULL;
-			}
-			release_cep = 1;
-		}
+		release_cep = 1;
 		break;
 
 	default:
-- 
2.25.1

[PATCH 11/31] rdma/siw: introduce SIW_EPSTATE_ACCEPTING/REJECTING for rdma_accept/rdma_reject

[Stefan Metzmacher]

When we received the MPA Request, we set SIW_EPSTATE_RECVD_MPAREQ
and port IW_CM_EVENT_CONNECT_REQUEST to the IWCM layer.

In that state we expect the caller to reacted with rdma_accept() or
rdma_reject(), which will turn the connection into SIW_EPSTATE_RDMA_MODE
or SIW_EPSTATE_CLOSED finally.

I think it much saner that rdma_accept and rdma_reject change the state
instead of keeping it as SIW_EPSTATE_RECVD_MPAREQ in order to make
the logic more understandable and allow more useful debug messages.

In all cases we need to inform the IWCM layer about that error!
As it only allows IW_CM_EVENT_ESTABLISHED to be posted after
IW_CM_EVENT_CONNECT_REQUEST was posted, we need to go through
IW_CM_EVENT_ESTABLISHED via IW_CM_EVENT_DISCONNECT to
IW_CM_EVENT_CLOSE.

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 | 31 ++++++++++++++++++++++++++++--
 drivers/infiniband/sw/siw/siw_cm.h |  2 ++
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index d03c7a66c6d1..3cc1d22fe232 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -146,10 +146,35 @@ static void __siw_cep_terminate_upcall(struct siw_cep *cep,
 
 	case SIW_EPSTATE_RECVD_MPAREQ:
 		/*
-		 * Wait for the ulp/CM to call accept/reject
+		 * Waited for the ulp/CM to call accept/reject
 		 */
-		siw_dbg_cep(cep, "mpa req recvd, wait for ULP\n");
 		WARN(!suspended, "SIW_EPSTATE_RECVD_MPAREQ called without suspended\n");
+		siw_dbg_cep(cep, "mpa req recvd, post established/disconnect/close\n");
+		siw_cm_upcall(cep, IW_CM_EVENT_ESTABLISHED, 0);
+		siw_cm_upcall(cep, IW_CM_EVENT_DISCONNECT, 0);
+		siw_cm_upcall(cep, IW_CM_EVENT_CLOSE, 0);
+		break;
+
+	case SIW_EPSTATE_ACCEPTING:
+		/*
+		 * We failed during the rdma_accept/siw_accept handling
+		 */
+		WARN(!suspended, "SIW_EPSTATE_ACCEPTING called without suspended\n");
+		siw_dbg_cep(cep, "accepting, post established/disconnect/close\n");
+		siw_cm_upcall(cep, IW_CM_EVENT_ESTABLISHED, 0);
+		siw_cm_upcall(cep, IW_CM_EVENT_DISCONNECT, 0);
+		siw_cm_upcall(cep, IW_CM_EVENT_CLOSE, 0);
+		break;
+
+	case SIW_EPSTATE_REJECTING:
+		/*
+		 * We failed during the rdma_reject/siw_reject handling
+		 */
+		WARN(!suspended, "SIW_EPSTATE_REJECTING called without suspended\n");
+		siw_dbg_cep(cep, "rejecting, post established/disconnect/close\n");
+		siw_cm_upcall(cep, IW_CM_EVENT_ESTABLISHED, 0);
+		siw_cm_upcall(cep, IW_CM_EVENT_DISCONNECT, 0);
+		siw_cm_upcall(cep, IW_CM_EVENT_CLOSE, 0);
 		break;
 
 	case SIW_EPSTATE_AWAIT_MPAREQ:
@@ -1563,6 +1588,7 @@ int siw_accept(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 
 		return -ECONNRESET;
 	}
+	cep->state = SIW_EPSTATE_ACCEPTING;
 	qp = siw_qp_id2obj(sdev, params->qpn);
 	if (!qp) {
 		WARN(1, "[QP %d] does not exist\n", params->qpn);
@@ -1743,6 +1769,7 @@ int siw_reject(struct iw_cm_id *id, const void *pdata, u8 pd_len)
 	}
 	siw_dbg_cep(cep, "cep->state %d, pd_len %d\n", cep->state,
 		    pd_len);
+	cep->state = SIW_EPSTATE_REJECTING;
 
 	if (__mpa_rr_revision(cep->mpa.hdr.params.bits) >= MPA_REVISION_1) {
 		cep->mpa.hdr.params.bits |= MPA_RR_FLAG_REJECT; /* reject */
diff --git a/drivers/infiniband/sw/siw/siw_cm.h b/drivers/infiniband/sw/siw/siw_cm.h
index 8c59cb3e2868..4f6219bd746b 100644
--- a/drivers/infiniband/sw/siw/siw_cm.h
+++ b/drivers/infiniband/sw/siw/siw_cm.h
@@ -20,6 +20,8 @@ enum siw_cep_state {
 	SIW_EPSTATE_AWAIT_MPAREQ,
 	SIW_EPSTATE_RECVD_MPAREQ,
 	SIW_EPSTATE_AWAIT_MPAREP,
+	SIW_EPSTATE_ACCEPTING,
+	SIW_EPSTATE_REJECTING,
 	SIW_EPSTATE_RDMA_MODE,
 	SIW_EPSTATE_CLOSED
 };
-- 
2.25.1

[PATCH 12/31] rdma/siw: add some debugging of state and sk_state to the teardown process

[Stefan Metzmacher]

That makes it easier to understanf where possible problems come from.

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 | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 3cc1d22fe232..ed33533ff9e6 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -111,6 +111,14 @@ static void __siw_cep_terminate_upcall(struct siw_cep *cep,
 				       int reply_status)
 {
 	bool suspended = false;
+	int sk_state_val = UINT_MAX;
+
+	if (cep->sock && cep->sock->sk)
+		sk_state_val = cep->sock->sk->sk_state;
+
+	siw_dbg_cep(cep, "[QP %u]: state: %d sk_state: %d\n",
+		    cep->qp ? qp_id(cep->qp) : UINT_MAX,
+		    cep->state, sk_state_val);
 
 	if (cep->qp) {
 		struct siw_qp *qp = cep->qp;
@@ -118,9 +126,14 @@ static void __siw_cep_terminate_upcall(struct siw_cep *cep,
 		if (qp->term_info.valid)
 			siw_send_terminate(qp);
 
+		siw_dbg_cep(cep,
+			    "with qp rx_suspend=%d tx_suspend=%d\n",
+			    qp->rx_stream.rx_suspend,
+			    qp->tx_ctx.tx_suspend);
 		if (qp->rx_stream.rx_suspend || qp->tx_ctx.tx_suspend)
 			suspended = true;
 	} else {
+		siw_dbg_cep(cep, "without qp\n");
 		suspended = true;
 	}
 
@@ -1307,7 +1320,7 @@ static void siw_cm_llp_state_change(struct sock *sk)
 	}
 	orig_state_change = cep->sk_state_change;
 
-	siw_dbg_cep(cep, "state: %d\n", cep->state);
+	siw_dbg_cep(cep, "state: %d sk_state: %d\n", cep->state, sk->sk_state);
 
 	switch (sk->sk_state) {
 	case TCP_ESTABLISHED:
-- 
2.25.1

[PATCH 13/31] rdma/siw: handle SIW_EPSTATE_CONNECTING in __siw_cep_terminate_upcall()

[Stefan Metzmacher]

The final patch will implement a non-blocking connect,
which means SIW_CM_WORK_MPATIMEOUT can also happen
during SIW_EPSTATE_CONNECTING.

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 | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index ed33533ff9e6..8e9f5ce5ce29 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -138,6 +138,14 @@ static void __siw_cep_terminate_upcall(struct siw_cep *cep,
 	}
 
 	switch (cep->state) {
+	case SIW_EPSTATE_CONNECTING:
+		/*
+		 * The TCP connect got rejected or timed out.
+		 */
+		siw_cm_upcall(cep, IW_CM_EVENT_CONNECT_REPLY,
+			      reply_status);
+		break;
+
 	case SIW_EPSTATE_AWAIT_MPAREP:
 		/*
 		 * MPA reply not received, but connection drop,
@@ -202,7 +210,6 @@ static void __siw_cep_terminate_upcall(struct siw_cep *cep,
 
 	case SIW_EPSTATE_IDLE:
 	case SIW_EPSTATE_LISTENING:
-	case SIW_EPSTATE_CONNECTING:
 	case SIW_EPSTATE_CLOSED:
 	default:
 		/*
-- 
2.25.1

[PATCH 14/31] rdma/siw: let siw_connect() set AWAIT_MPAREP before siw_send_mpareqrep()

[Stefan Metzmacher]

There's no real change made in this commit, but it makes the follwing
commits easier to review.

The idea is that we stay in SIW_EPSTATE_CONNECTING as long as
we only deal with tcp and not started the MPA negotiation.

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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 8e9f5ce5ce29..027bc18cb801 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -1477,8 +1477,6 @@ int siw_connect(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 	 */
 	siw_cep_socket_assoc(cep, s);
 
-	cep->state = SIW_EPSTATE_AWAIT_MPAREP;
-
 	/*
 	 * Set MPA Request bits: CRC if required, no MPA Markers,
 	 * MPA Rev. according to module parameter 'mpa_version', Key 'Request'.
@@ -1521,6 +1519,8 @@ int siw_connect(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 	}
 	memcpy(cep->mpa.hdr.key, MPA_KEY_REQ, 16);
 
+	cep->state = SIW_EPSTATE_AWAIT_MPAREP;
+
 	rv = siw_send_mpareqrep(cep, params->private_data, pd_len);
 	/*
 	 * Reset private data.
-- 
2.25.1

[PATCH 15/31] rdma/siw: create a temporary copy of private data

[Stefan Metzmacher]

The final patch will implement a non-blocking connect,
which means that siw_connect() will be split into
siw_connect() and siw_connected().

kernel_bindconnect() will be the last action
in siw_connect(), while the MPA negotiation
is deferred to siw_connected().

We should not rely on the callers private data
pointers to be still valid when siw_connected()
is called, so we better create a copy.

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 | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 027bc18cb801..41d3436985a6 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -1519,13 +1519,25 @@ int siw_connect(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 	}
 	memcpy(cep->mpa.hdr.key, MPA_KEY_REQ, 16);
 
+	cep->mpa.pdata = kmemdup(params->private_data, pd_len, GFP_KERNEL);
+	if (IS_ERR_OR_NULL(cep->mpa.pdata)) {
+		rv = -ENOMEM;
+		goto error;
+	}
+	cep->mpa.hdr.params.pd_len = pd_len;
+
 	cep->state = SIW_EPSTATE_AWAIT_MPAREP;
 
-	rv = siw_send_mpareqrep(cep, params->private_data, pd_len);
+	rv = siw_send_mpareqrep(cep, cep->mpa.pdata,
+				cep->mpa.hdr.params.pd_len);
 	/*
 	 * Reset private data.
 	 */
-	cep->mpa.hdr.params.pd_len = 0;
+	if (cep->mpa.hdr.params.pd_len) {
+		cep->mpa.hdr.params.pd_len = 0;
+		kfree(cep->mpa.pdata);
+		cep->mpa.pdata = NULL;
+	}
 
 	if (rv >= 0) {
 		rv = siw_cm_queue_work(cep, SIW_CM_WORK_MPATIMEOUT);
-- 
2.25.1

[PATCH 16/31] rdma/siw: use error and out logic at the end of siw_connect()

[Stefan Metzmacher]

This will make the following changes easier.

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 | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 41d3436985a6..ec6d5c26fe22 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -1539,14 +1539,19 @@ int siw_connect(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 		cep->mpa.pdata = NULL;
 	}
 
-	if (rv >= 0) {
-		rv = siw_cm_queue_work(cep, SIW_CM_WORK_MPATIMEOUT);
-		if (!rv) {
-			siw_dbg_cep(cep, "[QP %u]: exit\n", qp_id(qp));
-			siw_cep_set_free(cep);
-			return 0;
-		}
+	if (rv < 0) {
+		goto error;
+	}
+
+	rv = siw_cm_queue_work(cep, SIW_CM_WORK_MPATIMEOUT);
+	if (rv != 0) {
+		goto error;
 	}
+
+	siw_dbg_cep(cep, "[QP %u]: exit\n", qp_id(qp));
+	siw_cep_set_free(cep);
+	return 0;
+
 error:
 	siw_dbg(id->device, "failed: %d\n", rv);
 
-- 
2.25.1

[PATCH 17/31] rdma/siw: start mpa timer before calling siw_send_mpareqrep()

[Stefan Metzmacher]

The mpa timer will also span the non-blocking connect
in the final patch.

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 | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index ec6d5c26fe22..853b80fcb8b0 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -1526,6 +1526,11 @@ int siw_connect(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 	}
 	cep->mpa.hdr.params.pd_len = pd_len;
 
+	rv = siw_cm_queue_work(cep, SIW_CM_WORK_MPATIMEOUT);
+	if (rv != 0) {
+		goto error;
+	}
+
 	cep->state = SIW_EPSTATE_AWAIT_MPAREP;
 
 	rv = siw_send_mpareqrep(cep, cep->mpa.pdata,
@@ -1543,11 +1548,6 @@ int siw_connect(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 		goto error;
 	}
 
-	rv = siw_cm_queue_work(cep, SIW_CM_WORK_MPATIMEOUT);
-	if (rv != 0) {
-		goto error;
-	}
-
 	siw_dbg_cep(cep, "[QP %u]: exit\n", qp_id(qp));
 	siw_cep_set_free(cep);
 	return 0;
@@ -1556,6 +1556,8 @@ int siw_connect(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 	siw_dbg(id->device, "failed: %d\n", rv);
 
 	if (cep) {
+		siw_cancel_mpatimer(cep);
+
 		siw_socket_disassoc(s);
 		sock_release(s);
 		cep->sock = NULL;
-- 
2.25.1

[PATCH 18/31] rdma/siw: call the blocking kernel_bindconnect() just before siw_send_mpareqrep()

[Stefan Metzmacher]

We should build all state before calling kernel_bindconnect().
This will allow us to go async in the final patch.

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 | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 853b80fcb8b0..009a0afe6669 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -1425,18 +1425,6 @@ int siw_connect(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 	if (rv < 0)
 		goto error;
 
-	/*
-	 * NOTE: For simplification, connect() is called in blocking
-	 * mode. Might be reconsidered for async connection setup at
-	 * TCP level.
-	 */
-	rv = kernel_bindconnect(s, laddr, raddr, id->afonly);
-	if (rv != 0) {
-		siw_dbg_qp(qp, "kernel_bindconnect: error %d\n", rv);
-		goto error;
-	}
-	if (siw_tcp_nagle == false)
-		tcp_sock_set_nodelay(s->sk);
 	cep = siw_cep_alloc(sdev);
 	if (!cep) {
 		rv = -ENOMEM;
@@ -1531,6 +1519,19 @@ int siw_connect(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 		goto error;
 	}
 
+	/*
+	 * NOTE: For simplification, connect() is called in blocking
+	 * mode. Might be reconsidered for async connection setup at
+	 * TCP level.
+	 */
+	rv = kernel_bindconnect(s, laddr, raddr, id->afonly);
+	if (rv != 0) {
+		siw_dbg_qp(qp, "kernel_bindconnect: error %d\n", rv);
+		goto error;
+	}
+	if (siw_tcp_nagle == false)
+		tcp_sock_set_nodelay(s->sk);
+
 	cep->state = SIW_EPSTATE_AWAIT_MPAREP;
 
 	rv = siw_send_mpareqrep(cep, cep->mpa.pdata,
-- 
2.25.1

[PATCH 19/31] rdma/siw: split out a __siw_cep_close() function

[Stefan Metzmacher]

This can be used in a lot of other places too.
And can be the code path that we can easily adjust
without forgetting other places.

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 | 48 ++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 15 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 009a0afe6669..cf0f881c6793 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -220,6 +220,34 @@ static void __siw_cep_terminate_upcall(struct siw_cep *cep,
 	}
 }
 
+/*
+ * The caller needs to deal with siw_cep_set_inuse()
+ * and siw_cep_set_free()
+ */
+static void __siw_cep_close(struct siw_cep *cep)
+{
+	cep->state = SIW_EPSTATE_CLOSED;
+
+	if (cep->sock) {
+		siw_socket_disassoc(cep->sock);
+		sock_release(cep->sock);
+		cep->sock = NULL;
+	}
+
+	if (cep->cm_id) {
+		cep->cm_id->rem_ref(cep->cm_id);
+		cep->cm_id = NULL;
+	}
+
+	if (cep->qp) {
+		BUG_ON(cep->qp->cep != cep);
+		cep->qp->cep = NULL;
+		siw_qp_put(cep->qp);
+		cep->qp = NULL;
+		siw_cep_put(cep);
+	}
+}
+
 static void siw_rtr_data_ready(struct sock *sk)
 {
 	struct siw_cep *cep;
@@ -1559,27 +1587,17 @@ int siw_connect(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 	if (cep) {
 		siw_cancel_mpatimer(cep);
 
-		siw_socket_disassoc(s);
-		sock_release(s);
-		cep->sock = NULL;
-
-		cep->qp = NULL;
+		s = NULL;
+		qp = NULL;
 
-		cep->cm_id = NULL;
-		id->rem_ref(id);
-
-		qp->cep = NULL;
-		siw_cep_put(cep);
-
-		cep->state = SIW_EPSTATE_CLOSED;
+		__siw_cep_close(cep);
 
 		siw_cep_set_free(cep);
 
 		siw_cep_put(cep);
-
-	} else if (s) {
-		sock_release(s);
 	}
+	if (s)
+		sock_release(s);
 	if (qp)
 		siw_qp_put(qp);
 
-- 
2.25.1

[PATCH 20/31] rdma/siw: implement non-blocking connect.

[Stefan Metzmacher]

This is very important in order to prevent deadlocks.

The RDMA application layer expects rdma_connect() to be non-blocking
as the completion is handled via RDMA_CM_EVENT_ESTABLISHED and
other async events. It's not unlikely to hold a lock during
the rdma_connect() call.

Without out this a connection attempt to a non-existing/reachable
server block until the very long tcp timeout hits.
The application layer had no chance to have its own timeout handler
as that would just deadlock with the already blocking rdma_connect().

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 | 114 +++++++++++++++++++++--------
 drivers/infiniband/sw/siw/siw_cm.h |   1 +
 2 files changed, 85 insertions(+), 30 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index cf0f881c6793..9a550f040678 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -37,6 +37,7 @@ static void siw_cm_llp_write_space(struct sock *s);
 static void siw_cm_llp_error_report(struct sock *s);
 static int siw_cm_upcall(struct siw_cep *cep, enum iw_cm_event_type reason,
 			 int status);
+static void siw_connected(struct siw_cep *cep);
 
 static void siw_sk_assign_cm_upcalls(struct sock *sk)
 {
@@ -1141,6 +1142,10 @@ static void siw_cm_work_handler(struct work_struct *w)
 		siw_accept_newconn(cep);
 		break;
 
+	case SIW_CM_WORK_CONNECTED:
+		siw_connected(cep);
+		break;
+
 	case SIW_CM_WORK_READ_MPAHDR:
 		if (cep->state == SIW_EPSTATE_AWAIT_MPAREQ) {
 			if (cep->listen_cep) {
@@ -1306,6 +1311,7 @@ static void siw_cm_llp_data_ready(struct sock *sk)
 	switch (cep->state) {
 	case SIW_EPSTATE_RDMA_MODE:
 	case SIW_EPSTATE_LISTENING:
+	case SIW_EPSTATE_CONNECTING:
 		break;
 
 	case SIW_EPSTATE_AWAIT_MPAREQ:
@@ -1359,12 +1365,26 @@ static void siw_cm_llp_state_change(struct sock *sk)
 
 	switch (sk->sk_state) {
 	case TCP_ESTABLISHED:
-		/*
-		 * handle accepting socket as special case where only
-		 * new connection is possible
-		 */
-		siw_cm_queue_work(cep, SIW_CM_WORK_ACCEPT);
-		break;
+		if (cep->state == SIW_EPSTATE_CONNECTING) {
+			/*
+			 * handle accepting socket as special case where only
+			 * new connection is possible
+			 */
+			siw_cm_queue_work(cep, SIW_CM_WORK_CONNECTED);
+			break;
+
+		} else if (cep->state == SIW_EPSTATE_LISTENING) {
+			/*
+			 * handle accepting socket as special case where only
+			 * new connection is possible
+			 */
+			siw_cm_queue_work(cep, SIW_CM_WORK_ACCEPT);
+			break;
+		}
+		siw_dbg_cep(cep,
+			    "unexpected socket state %d with cep state %d\n",
+			    sk->sk_state, cep->state);
+		/* fall through */
 
 	case TCP_CLOSE:
 	case TCP_CLOSE_WAIT:
@@ -1383,7 +1403,7 @@ static void siw_cm_llp_state_change(struct sock *sk)
 static int kernel_bindconnect(struct socket *s, struct sockaddr *laddr,
 			      struct sockaddr *raddr, bool afonly)
 {
-	int rv, flags = 0;
+	int rv;
 	size_t size = laddr->sa_family == AF_INET ?
 		sizeof(struct sockaddr_in) : sizeof(struct sockaddr_in6);
 
@@ -1402,7 +1422,7 @@ static int kernel_bindconnect(struct socket *s, struct sockaddr *laddr,
 	if (rv < 0)
 		return rv;
 
-	rv = kernel_connect(s, raddr, size, flags);
+	rv = kernel_connect(s, raddr, size, O_NONBLOCK);
 
 	return rv < 0 ? rv : 0;
 }
@@ -1547,36 +1567,27 @@ int siw_connect(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 		goto error;
 	}
 
-	/*
-	 * NOTE: For simplification, connect() is called in blocking
-	 * mode. Might be reconsidered for async connection setup at
-	 * TCP level.
-	 */
 	rv = kernel_bindconnect(s, laddr, raddr, id->afonly);
+	if (rv == -EINPROGRESS) {
+		siw_dbg_qp(qp, "kernel_bindconnect: EINPROGRESS\n");
+		rv = 0;
+	}
 	if (rv != 0) {
 		siw_dbg_qp(qp, "kernel_bindconnect: error %d\n", rv);
 		goto error;
 	}
-	if (siw_tcp_nagle == false)
-		tcp_sock_set_nodelay(s->sk);
-
-	cep->state = SIW_EPSTATE_AWAIT_MPAREP;
 
-	rv = siw_send_mpareqrep(cep, cep->mpa.pdata,
-				cep->mpa.hdr.params.pd_len);
 	/*
-	 * Reset private data.
+	 * The rest will be done by siw_connected()
+	 *
+	 * siw_cm_llp_state_change() will detect
+	 * TCP_ESTABLISHED and schedules SIW_CM_WORK_CONNECTED,
+	 * which will finally call siw_connected().
+	 *
+	 * As siw_cm_llp_state_change() handles everything
+	 * siw_cm_llp_data_ready() can be a noop for
+	 * SIW_EPSTATE_CONNECTING.
 	 */
-	if (cep->mpa.hdr.params.pd_len) {
-		cep->mpa.hdr.params.pd_len = 0;
-		kfree(cep->mpa.pdata);
-		cep->mpa.pdata = NULL;
-	}
-
-	if (rv < 0) {
-		goto error;
-	}
-
 	siw_dbg_cep(cep, "[QP %u]: exit\n", qp_id(qp));
 	siw_cep_set_free(cep);
 	return 0;
@@ -1604,6 +1615,49 @@ int siw_connect(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 	return rv;
 }
 
+static void siw_connected(struct siw_cep *cep)
+{
+	struct siw_qp *qp = cep->qp;
+	struct socket *s = cep->sock;
+	int rv = -ECONNABORTED;
+
+	/*
+	 * already called with
+	 * siw_cep_set_inuse(cep);
+	 */
+
+	if (cep->state != SIW_EPSTATE_CONNECTING)
+		goto error;
+
+	if (siw_tcp_nagle == false)
+		tcp_sock_set_nodelay(s->sk);
+
+	cep->state = SIW_EPSTATE_AWAIT_MPAREP;
+
+	rv = siw_send_mpareqrep(cep, cep->mpa.pdata,
+				cep->mpa.hdr.params.pd_len);
+	/*
+	 * Reset private data.
+	 */
+	if (cep->mpa.hdr.params.pd_len) {
+		cep->mpa.hdr.params.pd_len = 0;
+		kfree(cep->mpa.pdata);
+		cep->mpa.pdata = NULL;
+	}
+
+	if (rv < 0) {
+		goto error;
+	}
+
+	siw_dbg_cep(cep, "[QP %u]: exit\n", qp_id(qp));
+	return;
+
+error:
+	siw_dbg_cep(cep, "[QP %u]: exit, error %d\n", qp_id(qp), rv);
+	siw_qp_cm_drop(qp, 1);
+	return;
+}
+
 /*
  * siw_accept - Let SoftiWARP accept an RDMA connection request
  *
diff --git a/drivers/infiniband/sw/siw/siw_cm.h b/drivers/infiniband/sw/siw/siw_cm.h
index 4f6219bd746b..62c9947999ac 100644
--- a/drivers/infiniband/sw/siw/siw_cm.h
+++ b/drivers/infiniband/sw/siw/siw_cm.h
@@ -78,6 +78,7 @@ struct siw_cep {
 
 enum siw_work_type {
 	SIW_CM_WORK_ACCEPT = 1,
+	SIW_CM_WORK_CONNECTED,
 	SIW_CM_WORK_READ_MPAHDR,
 	SIW_CM_WORK_CLOSE_LLP, /* close socket */
 	SIW_CM_WORK_PEER_CLOSE, /* socket indicated peer close */
-- 
2.25.1

[PATCH 21/31] rdma/siw: let siw_listen_address() call siw_cep_alloc() first

[Stefan Metzmacher]

This simplifies the cleanup path and makes the following
changes easier to review.

Check with "git show -w".

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 | 39 +++++++++++++++---------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 9a550f040678..fe6f7bb4d615 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -1916,9 +1916,16 @@ int siw_create_listen(struct iw_cm_id *id, int backlog)
 	if (addr_family != AF_INET && addr_family != AF_INET6)
 		return -EAFNOSUPPORT;
 
+	cep = siw_cep_alloc(sdev);
+	if (!cep)
+		return -ENOMEM;
+
 	rv = sock_create(addr_family, SOCK_STREAM, IPPROTO_TCP, &s);
-	if (rv < 0)
-		return rv;
+	if (rv < 0) {
+		siw_dbg(id->device, "sock_create error: %d\n", rv);
+		goto error;
+	}
+	siw_cep_socket_assoc(cep, s);
 
 	/*
 	 * Allow binding local port when still in TIME_WAIT from last close.
@@ -1957,12 +1964,6 @@ int siw_create_listen(struct iw_cm_id *id, int backlog)
 		siw_dbg(id->device, "socket bind error: %d\n", rv);
 		goto error;
 	}
-	cep = siw_cep_alloc(sdev);
-	if (!cep) {
-		rv = -ENOMEM;
-		goto error;
-	}
-	siw_cep_socket_assoc(cep, s);
 
 	rv = siw_cm_alloc_work(cep, backlog);
 	if (rv) {
@@ -2018,20 +2019,18 @@ int siw_create_listen(struct iw_cm_id *id, int backlog)
 error:
 	siw_dbg(id->device, "failed: %d\n", rv);
 
-	if (cep) {
-		siw_cep_set_inuse(cep);
-
-		if (cep->cm_id) {
-			cep->cm_id->rem_ref(cep->cm_id);
-			cep->cm_id = NULL;
-		}
-		cep->sock = NULL;
-		siw_socket_disassoc(s);
-		cep->state = SIW_EPSTATE_CLOSED;
+	siw_cep_set_inuse(cep);
 
-		siw_cep_set_free(cep);
-		siw_cep_put(cep);
+	if (cep->cm_id) {
+		cep->cm_id->rem_ref(cep->cm_id);
+		cep->cm_id = NULL;
 	}
+	cep->sock = NULL;
+	siw_socket_disassoc(s);
+	cep->state = SIW_EPSTATE_CLOSED;
+
+	siw_cep_set_free(cep);
+	siw_cep_put(cep);
 	sock_release(s);
 
 	return rv;
-- 
2.25.1

[PATCH 22/31] rdma/siw: let siw_listen_address() call siw_cep_set_inuse() early

[Stefan Metzmacher]

We should protect the whole section after siw_cep_alloc().

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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index fe6f7bb4d615..09ae7f7ca82a 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -1920,6 +1920,8 @@ int siw_create_listen(struct iw_cm_id *id, int backlog)
 	if (!cep)
 		return -ENOMEM;
 
+	siw_cep_set_inuse(cep);
+
 	rv = sock_create(addr_family, SOCK_STREAM, IPPROTO_TCP, &s);
 	if (rv < 0) {
 		siw_dbg(id->device, "sock_create error: %d\n", rv);
@@ -2014,13 +2016,12 @@ int siw_create_listen(struct iw_cm_id *id, int backlog)
 
 	siw_dbg(id->device, "Listen at laddr %pISp\n", &id->local_addr);
 
+	siw_cep_set_free(cep);
 	return 0;
 
 error:
 	siw_dbg(id->device, "failed: %d\n", rv);
 
-	siw_cep_set_inuse(cep);
-
 	if (cep->cm_id) {
 		cep->cm_id->rem_ref(cep->cm_id);
 		cep->cm_id = NULL;
-- 
2.25.1

[PATCH 23/31] rdma/siw: make use of __siw_cep_close() in siw_accept()

[Stefan Metzmacher]

This is basically the same just that the code in
__siw_cep_close() common, it skips elements which
are still NULL. Before it was really hard to prove
that we don't deference NULL pointers.

While developing my smbdirect driver, I hit so much
crashes and deadlocks, so we better have code that's
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 | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 09ae7f7ca82a..7fd67499f1d3 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -1833,23 +1833,7 @@ int siw_accept(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 
 	return 0;
 error:
-	siw_socket_disassoc(cep->sock);
-	sock_release(cep->sock);
-	cep->sock = NULL;
-
-	cep->state = SIW_EPSTATE_CLOSED;
-
-	if (cep->cm_id) {
-		cep->cm_id->rem_ref(id);
-		cep->cm_id = NULL;
-	}
-	if (qp->cep) {
-		siw_cep_put(cep);
-		qp->cep = NULL;
-	}
-	cep->qp = NULL;
-	siw_qp_put(qp);
-
+	__siw_cep_close(cep);
 	siw_cep_set_free(cep);
 	siw_cep_put(cep);
 
-- 
2.25.1

[PATCH 24/31] rdma/siw: do the full disassociation of cep and qp in siw_qp_llp_close()

[Stefan Metzmacher]

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(-)

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);
-- 
2.25.1

[PATCH 25/31] rdma/siw: fix double siw_cep_put() in siw_cm_work_handler()

[Stefan Metzmacher]

We never do an additional siw_cep_get(cep) when calling id->add_ref(id),
there's no reason to call siw_cep_put(cep) when calling
cep->cm_id->rem_ref(cep->cm_id)!

I saw this happening quite often while testing my smbdirect driver
and the peer already reseted the tcp connection.

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 | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 31135d877d41..a2a5a36370af 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -1252,7 +1252,6 @@ static void siw_cm_work_handler(struct work_struct *w)
 		if (cep->cm_id) {
 			cep->cm_id->rem_ref(cep->cm_id);
 			cep->cm_id = NULL;
-			siw_cep_put(cep);
 		}
 	}
 	siw_cep_set_free(cep);
-- 
2.25.1

[PATCH 26/31] rdma/siw: make use of __siw_cep_close() in siw_cm_work_handler()

[Stefan Metzmacher]

We should always go to that common function in order to
avoid potential problems in future.

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 | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index a2a5a36370af..3dc80c21ac60 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -1244,15 +1244,7 @@ static void siw_cm_work_handler(struct work_struct *w)
 			siw_cep_set_inuse(cep);
 			siw_qp_put(qp);
 		}
-		if (cep->sock) {
-			siw_socket_disassoc(cep->sock);
-			sock_release(cep->sock);
-			cep->sock = NULL;
-		}
-		if (cep->cm_id) {
-			cep->cm_id->rem_ref(cep->cm_id);
-			cep->cm_id = NULL;
-		}
+		__siw_cep_close(cep);
 	}
 	siw_cep_set_free(cep);
 	siw_put_work(work);
-- 
2.25.1

[PATCH 27/31] rdma/siw: fix the "close" logic in siw_qp_cm_drop()

[Stefan Metzmacher]

cep->cm_id->rem_ref(cep->cm_id) is no reason to call
siw_cep_put(cep), we never call siw_cep_get(cep) when
calling id->add_ref(id).

But the cep->qp cleanup needs to drop both references!

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 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 3dc80c21ac60..9f9750237e75 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -546,7 +546,6 @@ void siw_qp_cm_drop(struct siw_qp *qp, int schedule)
 		if (cep->cm_id) {
 			cep->cm_id->rem_ref(cep->cm_id);
 			cep->cm_id = NULL;
-			siw_cep_put(cep);
 		}
 		cep->state = SIW_EPSTATE_CLOSED;
 
@@ -559,8 +558,11 @@ void siw_qp_cm_drop(struct siw_qp *qp, int schedule)
 			cep->sock = NULL;
 		}
 		if (cep->qp) {
+			BUG_ON(cep->qp->cep != cep);
+			cep->qp->cep = NULL;
+			siw_qp_put(cep->qp);
 			cep->qp = NULL;
-			siw_qp_put(qp);
+			siw_cep_put(cep);
 		}
 out:
 		siw_cep_set_free(cep);
-- 
2.25.1

[PATCH 28/31] rdma/siw: make use of __siw_cep_close() in siw_qp_cm_drop()

[Stefan Metzmacher]

Now that the logic is identical we better use the common function
in order to avoid future problems.

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 | 23 +----------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 9f9750237e75..d2b1c62177ea 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -542,28 +542,7 @@ void siw_qp_cm_drop(struct siw_qp *qp, int schedule)
 		siw_dbg_cep(cep, "immediate close, state %d\n", cep->state);
 
 		__siw_cep_terminate_upcall(cep, -EINVAL);
-
-		if (cep->cm_id) {
-			cep->cm_id->rem_ref(cep->cm_id);
-			cep->cm_id = NULL;
-		}
-		cep->state = SIW_EPSTATE_CLOSED;
-
-		if (cep->sock) {
-			siw_socket_disassoc(cep->sock);
-			/*
-			 * Immediately close socket
-			 */
-			sock_release(cep->sock);
-			cep->sock = NULL;
-		}
-		if (cep->qp) {
-			BUG_ON(cep->qp->cep != cep);
-			cep->qp->cep = NULL;
-			siw_qp_put(cep->qp);
-			cep->qp = NULL;
-			siw_cep_put(cep);
-		}
+		__siw_cep_close(cep);
 out:
 		siw_cep_set_free(cep);
 	}
-- 
2.25.1

[PATCH 29/31] rdma/siw: make use of __siw_cep_close() in siw_reject()

[Stefan Metzmacher]

This is more or less a consistency change, we should always
go via __siw_cep_close() to release the socket in order to
avoid problems in future.

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 | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index d2b1c62177ea..4b789379f676 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -1841,12 +1841,8 @@ int siw_reject(struct iw_cm_id *id, const void *pdata, u8 pd_len)
 		cep->mpa.hdr.params.bits |= MPA_RR_FLAG_REJECT; /* reject */
 		siw_send_mpareqrep(cep, pdata, pd_len);
 	}
-	siw_socket_disassoc(cep->sock);
-	sock_release(cep->sock);
-	cep->sock = NULL;
-
-	cep->state = SIW_EPSTATE_CLOSED;
 
+	__siw_cep_close(cep);
 	siw_cep_set_free(cep);
 	siw_cep_put(cep);
 
-- 
2.25.1

[PATCH 30/31] rdma/siw: make use of __siw_cep_close() in siw_listen_address()

[Stefan Metzmacher]

This is more or less a consistency change, we should always
go via __siw_cep_close() to release the socket in order to
avoid problems in future.

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 | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 4b789379f676..49d01264682a 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -1972,17 +1972,9 @@ int siw_create_listen(struct iw_cm_id *id, int backlog)
 error:
 	siw_dbg(id->device, "failed: %d\n", rv);
 
-	if (cep->cm_id) {
-		cep->cm_id->rem_ref(cep->cm_id);
-		cep->cm_id = NULL;
-	}
-	cep->sock = NULL;
-	siw_socket_disassoc(s);
-	cep->state = SIW_EPSTATE_CLOSED;
-
+	__siw_cep_close(cep);
 	siw_cep_set_free(cep);
 	siw_cep_put(cep);
-	sock_release(s);
 
 	return rv;
 }
-- 
2.25.1

[PATCH 31/31] rdma/siw: make use of __siw_cep_close() in siw_drop_listeners()

[Stefan Metzmacher]

This is more or less a consistency change, we should always
go via __siw_cep_close() to release the socket in order to
avoid problems in future.

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 | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 49d01264682a..58e965970f3e 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -1995,17 +1995,7 @@ static void siw_drop_listeners(struct iw_cm_id *id)
 		siw_dbg_cep(cep, "drop cep, state %d\n", cep->state);
 
 		siw_cep_set_inuse(cep);
-
-		if (cep->cm_id) {
-			cep->cm_id->rem_ref(cep->cm_id);
-			cep->cm_id = NULL;
-		}
-		if (cep->sock) {
-			siw_socket_disassoc(cep->sock);
-			sock_release(cep->sock);
-			cep->sock = NULL;
-		}
-		cep->state = SIW_EPSTATE_CLOSED;
+		__siw_cep_close(cep);
 		siw_cep_set_free(cep);
 		siw_cep_put(cep);
 	}
-- 
2.25.1

Re: [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs

[Bernard Metzler]

-----"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 00/31] rdma/siw: fix a lot of deadlocks
>and use after free bugs
>
>Hi Bernard,
>
>while testing with my smbdirect driver I hit a lot of
>bugs in the siw.ko driver. They all cause problems where
>the siw driver was not able to unload anymore and I had to
>reboot the machine.
>
Hi Stefan,

Much appreciated!
These are quite some patches, and I will need some time
to go through. Would bee nice if those would be broken
down into smaller bundles (introduce non-blocking connect,
_siw_cep_close() subroutine, fixing cep reference counting,
smp_mb() after STag invalidation, ..). Anyway, many
thanks for the effort, it will improve the driver!

First comments:

A non blocking connect does really makes sense as you
are pointing out. I hope it doesn't complicate the CM
code even further.

I think we agreed upon not using BUG() and BUG_ON(),
so please don't introduce it.

'I hit a lot of bugs' is not very helpful, but just
a statement.

Thanks very much!
Bernard.


>I implemented:
>- a non blocking connect
>- fixed a lot of bugs where siw_cep_put() was called too often.
>- fixed bugs where siw_cm_upcall() confused the core IWCM logic
>
>I have some more changes to follow, but I wanted to send them
>finally out after having them one and a half year sitting in some
>private branch...
>
>Stefan Metzmacher (31):
>  rdma/siw: fix warning in siw_proc_send()
>  rdma/siw: call smp_mb() after mem->stag_valid = 0 in
>    siw_invalidate_stag() too
>  rdma/siw: remove superfluous siw_cep_put() from siw_connect() error
>    path
>  rdma/siw: let siw_accept() deferr RDMA_MODE until EVENT_ESTABLISHED
>  rdma/siw: make use of kernel_{bind,connect,listen}()
>  rdma/siw: make siw_cm_upcall() a noop without valid 'id'
>  rdma/siw: split out a __siw_cep_terminate_upcall() function
>  rdma/siw: use __siw_cep_terminate_upcall() for indirect
>    SIW_CM_WORK_CLOSE_LLP
>  rdma/siw: use __siw_cep_terminate_upcall() for
>SIW_CM_WORK_PEER_CLOSE
>  rdma/siw: use __siw_cep_terminate_upcall() for
>SIW_CM_WORK_MPATIMEOUT
>  rdma/siw: introduce SIW_EPSTATE_ACCEPTING/REJECTING for
>    rdma_accept/rdma_reject
>  rdma/siw: add some debugging of state and sk_state to the teardown
>    process
>  rdma/siw: handle SIW_EPSTATE_CONNECTING in
>    __siw_cep_terminate_upcall()
>  rdma/siw: let siw_connect() set AWAIT_MPAREP before
>    siw_send_mpareqrep()
>  rdma/siw: create a temporary copy of private data
>  rdma/siw: use error and out logic at the end of siw_connect()
>  rdma/siw: start mpa timer before calling siw_send_mpareqrep()
>  rdma/siw: call the blocking kernel_bindconnect() just before
>    siw_send_mpareqrep()
>  rdma/siw: split out a __siw_cep_close() function
>  rdma/siw: implement non-blocking connect.
>  rdma/siw: let siw_listen_address() call siw_cep_alloc() first
>  rdma/siw: let siw_listen_address() call siw_cep_set_inuse() early
>  rdma/siw: make use of __siw_cep_close() in siw_accept()
>  rdma/siw: do the full disassociation of cep and qp in
>    siw_qp_llp_close()
>  rdma/siw: fix double siw_cep_put() in siw_cm_work_handler()
>  rdma/siw: make use of __siw_cep_close() in siw_cm_work_handler()
>  rdma/siw: fix the "close" logic in siw_qp_cm_drop()
>  rdma/siw: make use of __siw_cep_close() in siw_qp_cm_drop()
>  rdma/siw: make use of __siw_cep_close() in siw_reject()
>  rdma/siw: make use of __siw_cep_close() in siw_listen_address()
>  rdma/siw: make use of __siw_cep_close() in siw_drop_listeners()
>
> drivers/infiniband/sw/siw/siw_cm.c    | 537
>+++++++++++++++-----------
> drivers/infiniband/sw/siw/siw_cm.h    |   3 +
> drivers/infiniband/sw/siw/siw_mem.c   |   2 +
> drivers/infiniband/sw/siw/siw_qp.c    |   3 +
> drivers/infiniband/sw/siw/siw_qp_rx.c |   2 +-
> 5 files changed, 316 insertions(+), 231 deletions(-)
>
>-- 
>2.25.1
>
>

Re: [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs

[Stefan Metzmacher]

Hi Bernard,

> Much appreciated!
> These are quite some patches, and I will need some time
> to go through. Would bee nice if those would be broken
> down into smaller bundles (introduce non-blocking connect,
> _siw_cep_close() subroutine, fixing cep reference counting,
> smp_mb() after STag invalidation, ..). 

They mostly fall out naturally getting one step further
with each commit. So most of them depend on each other.
I'll see if I can reorder some of them, but I'm not sure it's really
worth the effort.

> Anyway, many thanks for the effort,

Thanks a lot for the review.

> it will improve the driver!

Yes!

On top I have some code to support MPA rev1 in peer_to_peer mode
in order to interoperate with a Chelcio T404-BT card running
under Windows.

In preparation I've code that moves the currently hardcoded values
(which where module params before) into a per device structure,
some like 'sdev->options.crc_strict'. With that we only need to
find a good way to pass these parameters from userspace to the
device. I guess that should be done somehow via the 'rdma link add'
command, or via files similar to /proc/sys/net/ipv4/conf/*.

Here's my branch with all (partly incomplete) siw changes:
https://git.samba.org/?p=metze/linux/wip.git;a=shortlog;h=refs/heads/rdma-next-siw

> First comments:
> 
> A non blocking connect does really makes sense as you
> are pointing out. I hope it doesn't complicate the CM
> code even further.
> 
> I think we agreed upon not using BUG() and BUG_ON(),
> so please don't introduce it.

Ok.

> 'I hit a lot of bugs' is not very helpful, but just
> a statement.

More details are in the individual commit messages,
should I double them in the cover letter next time?

Currently I'm quite busy with other stuff...
I hope to find some time in the next weeks to
comment more detailed and post a new revision.

metze

Re: Re: [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs

[Bernard Metzler]

-----"Stefan Metzmacher" <metze@samba.org> wrote: -----

>To: "Bernard Metzler" <BMT@zurich.ibm.com>
>From: "Stefan Metzmacher" <metze@samba.org>
>Date: 05/26/2021 05:44PM
>Cc: "linux-rdma" <linux-rdma@vger.kernel.org>
>Subject: [EXTERNAL] Re: [PATCH 00/31] rdma/siw: fix a lot of
>deadlocks and use after free bugs
>
>Hi Bernard,
>
>> Much appreciated!
>> These are quite some patches, and I will need some time
>> to go through. Would bee nice if those would be broken
>> down into smaller bundles (introduce non-blocking connect,
>> _siw_cep_close() subroutine, fixing cep reference counting,
>> smp_mb() after STag invalidation, ..). 
>
>They mostly fall out naturally getting one step further
>with each commit. So most of them depend on each other.
>I'll see if I can reorder some of them, but I'm not sure it's really
>worth the effort.

Why not having just a few patches - one fixing the obvious
object management bug(s), one on a more concise error handling,
one on using exported kernel functions instead of calling
socket method directly, one introducing asynchronous connect..?

I understand you collected problems over time and fixed those,
but it would be much easier to digest if separated logically.

>
>> Anyway, many thanks for the effort,
>
>Thanks a lot for the review.
>
>> it will improve the driver!
>
>Yes!
>
>On top I have some code to support MPA rev1 in peer_to_peer mode
>in order to interoperate with a Chelcio T404-BT card running
>under Windows.
>
>In preparation I've code that moves the currently hardcoded values
>(which where module params before) into a per device structure,
>some like 'sdev->options.crc_strict'. With that we only need to
>find a good way to pass these parameters from userspace to the
>device. I guess that should be done somehow via the 'rdma link add'
>command, or via files similar to /proc/sys/net/ipv4/conf/*.
>
yes with dropping the module parameters we lost that flexibility...

I'd prefer protocol specific extensions to the rdma tools.

In fact, we could allow different CRC and MPA settings per
QP (which would make sense if we have connections from a
local siw device to multiple remote devices with different
capabilities etc.). But we do not have endpoint/QP object
specific settings in rdma netlink currently. 
Having link specific settings might be sufficient though.


>Here's my branch with all (partly incomplete) siw changes:
>INVALID URI REMOVED
>Fp-3Dmetze_linux_wip.git-3Ba-3Dshortlog-3Bh-3Drefs_heads_rdma-2Dnext-
>2Dsiw&d=DwIDaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=2TaYXQ0T-r8ZO1PP1alNwU_QJcR
>RLfmYTAgd3QCvqSc&m=bcCk65hNAmUVFgsBxIE9Y6S1cnxdE1otmHllxAlO-Ko&s=Rgu7
>GuEAeI9MyUx7m03KEMLH2qA7Y3065X8LCBo3EBY&e= 
>

Thanks, I'll have a look.

>> First comments:
>> 
>> A non blocking connect does really makes sense as you
>> are pointing out. I hope it doesn't complicate the CM
>> code even further.
>> 
>> I think we agreed upon not using BUG() and BUG_ON(),
>> so please don't introduce it.
>
>Ok.
>
>> 'I hit a lot of bugs' is not very helpful, but just
>> a statement.
>
>More details are in the individual commit messages,
>should I double them in the cover letter next time?
>
>Currently I'm quite busy with other stuff...
>I hope to find some time in the next weeks to
>comment more detailed and post a new revision.
>
>metze
>
Thanks again!
Bernard.

Re: [PATCH 01/31] rdma/siw: fix warning in siw_proc_send()

[Bernard Metzler]

-----"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 01/31] rdma/siw: fix warning in
>siw_proc_send()
>
>  CC [M]  drivers/infiniband/sw/siw/siw_qp_rx.o
>In file included from ./include/linux/wait.h:9:0,
>                 from ./include/linux/net.h:19,
>                 from drivers/infiniband/sw/siw/siw_qp_rx.c:8:
>drivers/infiniband/sw/siw/siw_qp_rx.c: In function ‘siw_proc_send’:
>./include/linux/spinlock.h:288:3: warning: ‘flags’ may be used
>uninitialized in this function [-Wmaybe-uninitialized]
>   _raw_spin_unlock_irqrestore(lock, flags); \
>   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>drivers/infiniband/sw/siw/siw_qp_rx.c:335:16: note: ‘flags’ was
>declared here
>  unsigned long flags;
>
>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_qp_rx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_qp_rx.c
>b/drivers/infiniband/sw/siw/siw_qp_rx.c
>index 60116f20653c..0170c05d2cc3 100644
>--- a/drivers/infiniband/sw/siw/siw_qp_rx.c
>+++ b/drivers/infiniband/sw/siw/siw_qp_rx.c
>@@ -333,7 +333,7 @@ static struct siw_wqe *siw_rqe_get(struct siw_qp
>*qp)
> 	struct siw_srq *srq;
> 	struct siw_wqe *wqe = NULL;
> 	bool srq_event = false;
>-	unsigned long flags;
>+	unsigned long flags = 0;
> 
This is not needed. flags are only used if 'srq'
is valid. 'srq' dosen't get reassigned after first check.
Somehow the compiler warning is a false negative.


> 	srq = qp->srq;
> 	if (srq) {
>-- 
>2.25.1
>
>

Re: [PATCH 03/31] rdma/siw: remove superfluous siw_cep_put() from siw_connect() error path

[Bernard Metzler]

-----"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 03/31] rdma/siw: remove superfluous
>siw_cep_put() from siw_connect() error path
>
>The following change demonstrate the bug:
>
>    --- 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;
>
>That change triggers the WARN_ON() in siw_cep_put().
>
>As there's no siw_cep_get() arround id->add_ref()
>I removed the siw_cep_put() following id->rem_ref().
>
>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 | 1 -
> 1 file changed, 1 deletion(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_cm.c
>b/drivers/infiniband/sw/siw/siw_cm.c
>index 7a5ed86ffc9f..da84686a21fd 100644
>--- a/drivers/infiniband/sw/siw/siw_cm.c
>+++ b/drivers/infiniband/sw/siw/siw_cm.c
>@@ -1494,7 +1494,6 @@ int siw_connect(struct iw_cm_id *id, struct
>iw_cm_conn_param *params)
> 
> 		cep->cm_id = NULL;
> 		id->rem_ref(id);
>-		siw_cep_put(cep);
> 
> 		qp->cep = NULL;
> 		siw_cep_put(cep);
>-- 
>2.25.1
>
>

Thanks, good catch!

Reviewed-by: Bernard Metzler <bmt@zurich.ibm.com>

Re: [PATCH 02/31] rdma/siw: call smp_mb() after mem->stag_valid = 0 in siw_invalidate_stag() too

[Bernard Metzler]

-----"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 02/31] rdma/siw: call smp_mb() after
>mem->stag_valid = 0 in siw_invalidate_stag() too
>
>We already do the same in siw_mr_drop_mem().
>
>Fixes: 2251334dcac9 ("rdma/siw: application buffer 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_mem.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/drivers/infiniband/sw/siw/siw_mem.c
>b/drivers/infiniband/sw/siw/siw_mem.c
>index 61c17db70d65..8596ce1ef5a3 100644
>--- a/drivers/infiniband/sw/siw/siw_mem.c
>+++ b/drivers/infiniband/sw/siw/siw_mem.c
>@@ -309,6 +309,8 @@ int siw_invalidate_stag(struct ib_pd *pd, u32
>stag)
> 	 * state if invalidation is requested. So no state check here.
> 	 */
> 	mem->stag_valid = 0;
>+	/* make STag invalid visible asap */
>+	smp_mb();
> 
> 	siw_dbg_pd(pd, "STag 0x%08x now invalid\n", stag);
> out:
>-- 
>2.25.1
>
>
makes sense, thanks.

Reviewed-by: Bernard Metzler <bmt@zurich.ibm.com>

Re: [PATCH 05/31] rdma/siw: make use of kernel_{bind,connect,listen}()

[Bernard Metzler]

---
Bernard Metzler, PhD
Tech. Leader High Performance I/O, Principal Research Staff
IBM Zurich Research Laboratory
Saeumerstrasse 4
CH-8803 Rueschlikon, Switzerland
+41 44 724 8605
 

-----"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 05/31] rdma/siw: make use of
>kernel_{bind,connect,listen}()
>
>That's nicer than dereferencing socket structures.
>
>This prepares making rdma_connect()/siw_connect() non-blocking
>in order to avoid deadlocks in the callers.
>
>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 | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_cm.c
>b/drivers/infiniband/sw/siw/siw_cm.c
>index 145ab6e4e0ed..e21cde84306e 100644
>--- a/drivers/infiniband/sw/siw/siw_cm.c
>+++ b/drivers/infiniband/sw/siw/siw_cm.c
>@@ -1319,11 +1319,11 @@ static int kernel_bindconnect(struct socket
>*s, struct sockaddr *laddr,
> 			return rv;
> 	}
> 
>-	rv = s->ops->bind(s, laddr, size);
>+	rv = kernel_bind(s, laddr, size);
> 	if (rv < 0)
> 		return rv;
> 
>-	rv = s->ops->connect(s, raddr, size, flags);
>+	rv = kernel_connect(s, raddr, size, flags);
> 
> 	return rv < 0 ? rv : 0;
> }
>@@ -1787,8 +1787,8 @@ int siw_create_listen(struct iw_cm_id *id, int
>backlog)
> 		if (ipv4_is_zeronet(laddr->sin_addr.s_addr))
> 			s->sk->sk_bound_dev_if = sdev->netdev->ifindex;
> 
>-		rv = s->ops->bind(s, (struct sockaddr *)laddr,
>-				  sizeof(struct sockaddr_in));
>+		rv = kernel_bind(s, (struct sockaddr *)laddr,
>+				 sizeof(struct sockaddr_in));
> 	} else {
> 		struct sockaddr_in6 *laddr = &to_sockaddr_in6(id->local_addr);
> 
>@@ -1805,8 +1805,8 @@ int siw_create_listen(struct iw_cm_id *id, int
>backlog)
> 		if (ipv6_addr_any(&laddr->sin6_addr))
> 			s->sk->sk_bound_dev_if = sdev->netdev->ifindex;
> 
>-		rv = s->ops->bind(s, (struct sockaddr *)laddr,
>-				  sizeof(struct sockaddr_in6));
>+		rv = kernel_bind(s, (struct sockaddr *)laddr,
>+				 sizeof(struct sockaddr_in6));
> 	}
> 	if (rv) {
> 		siw_dbg(id->device, "socket bind error: %d\n", rv);
>@@ -1826,7 +1826,7 @@ int siw_create_listen(struct iw_cm_id *id, int
>backlog)
> 			rv, backlog);
> 		goto error;
> 	}
>-	rv = s->ops->listen(s, backlog);
>+	rv = kernel_listen(s, backlog);
> 	if (rv) {
> 		siw_dbg(id->device, "listen error %d\n", rv);
> 		goto error;
>-- 
>2.25.1
>
>
Yes, thanks.

Reviewed-by: Bernard Metzler <bmt@zurich.ibm.com>

Re: [PATCH 04/31] rdma/siw: let siw_accept() deferr RDMA_MODE until EVENT_ESTABLISHED

[Bernard Metzler]

-----"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>

Re: [PATCH 06/31] rdma/siw: make siw_cm_upcall() a noop without valid 'id'

[Bernard Metzler]

-----"Stefan Metzmacher" <metze@samba.org> wrote: -----

>To: "Bernard Metzler" <bmt@zurich.ibm.com>
>From: "Stefan Metzmacher" <metze@samba.org>
>Date: 05/07/2021 01:38AM
>Cc: linux-rdma@vger.kernel.org, "Stefan Metzmacher" <metze@samba.org>
>Subject: [EXTERNAL] [PATCH 06/31] rdma/siw: make siw_cm_upcall() a
>noop without valid 'id'
>
>This will simplify the callers.
>
>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 | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/drivers/infiniband/sw/siw/siw_cm.c
>b/drivers/infiniband/sw/siw/siw_cm.c
>index e21cde84306e..2cc2863bd427 100644
>--- a/drivers/infiniband/sw/siw/siw_cm.c
>+++ b/drivers/infiniband/sw/siw/siw_cm.c
>@@ -324,6 +324,9 @@ static int siw_cm_upcall(struct siw_cep *cep,
>enum iw_cm_event_type reason,
> 	} else {
> 		id = cep->cm_id;
> 	}
>+	if (id == NULL)
How can this happen?

>+		return status;

better return 0 ?
>+
> 	/* Signal IRD and ORD */
> 	if (reason == IW_CM_EVENT_ESTABLISHED ||
> 	    reason == IW_CM_EVENT_CONNECT_REPLY) {
>-- 
>2.25.1
>
>

Re: [PATCH 07/31] rdma/siw: split out a __siw_cep_terminate_upcall() function

[Bernard Metzler]

-----"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 07/31] rdma/siw: split out a
>__siw_cep_terminate_upcall() function
>
>There are multiple places where should have the same logic.
>Having one helper function to be used in all places
>makes it easier to extended the logic.
>
>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 | 53
>++++++++++++++++++------------
> 1 file changed, 32 insertions(+), 21 deletions(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_cm.c
>b/drivers/infiniband/sw/siw/siw_cm.c
>index 2cc2863bd427..c91a74271b9b 100644
>--- a/drivers/infiniband/sw/siw/siw_cm.c
>+++ b/drivers/infiniband/sw/siw/siw_cm.c
>@@ -103,6 +103,37 @@ static void siw_socket_disassoc(struct socket
>*s)
> 	}
> }
> 
>+/*
>+ * The caller needs to deal with siw_cep_set_inuse()
>+ * and siw_cep_set_free()
>+ */
>+static void __siw_cep_terminate_upcall(struct siw_cep *cep,
>+				       int reply_status)
>+{
>+	if (cep->qp && cep->qp->term_info.valid)
>+		siw_send_terminate(cep->qp);
>+

While I see some merits of having consolidated the 
updates to the IWCM state machine on close, I don't think we
should mix this with sending TERMINATE messages on the wire.
Just keep that where it currently is.

Call that new thing different - 'siw_upcall_llp_close' 
or some such?

>+	switch (cep->state) {
>+	case SIW_EPSTATE_AWAIT_MPAREP:
>+		siw_cm_upcall(cep, IW_CM_EVENT_CONNECT_REPLY,
>+			      reply_status);
>+		break;
>+
>+	case SIW_EPSTATE_RDMA_MODE:
>+		siw_cm_upcall(cep, IW_CM_EVENT_CLOSE, 0);
>+		break;
>+
>+	case SIW_EPSTATE_IDLE:
>+	case SIW_EPSTATE_LISTENING:
>+	case SIW_EPSTATE_CONNECTING:
>+	case SIW_EPSTATE_AWAIT_MPAREQ:
>+	case SIW_EPSTATE_RECVD_MPAREQ:
>+	case SIW_EPSTATE_CLOSED:
>+	default:
>+		break;
>+	}
>+}
>+
> static void siw_rtr_data_ready(struct sock *sk)
> {
> 	struct siw_cep *cep;
>@@ -395,29 +426,9 @@ void siw_qp_cm_drop(struct siw_qp *qp, int
>schedule)
> 		}
> 		siw_dbg_cep(cep, "immediate close, state %d\n", cep->state);
> 
>-		if (qp->term_info.valid)
>-			siw_send_terminate(qp);
>+		__siw_cep_terminate_upcall(cep, -EINVAL);
> 
> 		if (cep->cm_id) {
>-			switch (cep->state) {
>-			case SIW_EPSTATE_AWAIT_MPAREP:
>-				siw_cm_upcall(cep, IW_CM_EVENT_CONNECT_REPLY,
>-					      -EINVAL);
>-				break;
>-
>-			case SIW_EPSTATE_RDMA_MODE:
>-				siw_cm_upcall(cep, IW_CM_EVENT_CLOSE, 0);
>-				break;
>-
>-			case SIW_EPSTATE_IDLE:
>-			case SIW_EPSTATE_LISTENING:
>-			case SIW_EPSTATE_CONNECTING:
>-			case SIW_EPSTATE_AWAIT_MPAREQ:
>-			case SIW_EPSTATE_RECVD_MPAREQ:
>-			case SIW_EPSTATE_CLOSED:
>-			default:
>-				break;
>-			}
> 			cep->cm_id->rem_ref(cep->cm_id);
> 			cep->cm_id = NULL;
> 			siw_cep_put(cep);
>-- 
>2.25.1
>
>

Re: [PATCH 08/31] rdma/siw: use __siw_cep_terminate_upcall() for indirect SIW_CM_WORK_CLOSE_LLP

[Bernard Metzler]

-----"Stefan Metzmacher" <metze@samba.org> wrote: -----

>To: "Bernard Metzler" <bmt@zurich.ibm.com>
>From: "Stefan Metzmacher" <metze@samba.org>
>Date: 05/07/2021 01:38AM
>Cc: linux-rdma@vger.kernel.org, "Stefan Metzmacher" <metze@samba.org>
>Subject: [EXTERNAL] [PATCH 08/31] rdma/siw: use
>__siw_cep_terminate_upcall() for indirect SIW_CM_WORK_CLOSE_LLP
>
>Both code paths from siw_qp_cm_drop() should use the same logic.
>
>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 | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_cm.c
>b/drivers/infiniband/sw/siw/siw_cm.c
>index c91a74271b9b..b7e7f637bd03 100644
>--- a/drivers/infiniband/sw/siw/siw_cm.c
>+++ b/drivers/infiniband/sw/siw/siw_cm.c
>@@ -1071,11 +1071,7 @@ static void siw_cm_work_handler(struct
>work_struct *w)
> 		/*
> 		 * QP scheduled LLP close
> 		 */
>-		if (cep->qp && cep->qp->term_info.valid)
>-			siw_send_terminate(cep->qp);
>-
leave that there.

>-		if (cep->cm_id)
>-			siw_cm_upcall(cep, IW_CM_EVENT_CLOSE, 0);
>+		__siw_cep_terminate_upcall(cep, -EINVAL);
> 
> 		release_cep = 1;
> 		break;
>-- 
>2.25.1
>
>

Re: [PATCH 09/31] rdma/siw: use __siw_cep_terminate_upcall() for SIW_CM_WORK_PEER_CLOSE

[Bernard Metzler]

-----"Stefan Metzmacher" <metze@samba.org> wrote: -----

>To: "Bernard Metzler" <bmt@zurich.ibm.com>
>From: "Stefan Metzmacher" <metze@samba.org>
>Date: 05/07/2021 01:38AM
>Cc: linux-rdma@vger.kernel.org, "Stefan Metzmacher" <metze@samba.org>
>Subject: [EXTERNAL] [PATCH 09/31] rdma/siw: use
>__siw_cep_terminate_upcall() for SIW_CM_WORK_PEER_CLOSE
>
>It's easier to have generic logic in just one place.
>
>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 | 89
>+++++++++++++++++-------------
> 1 file changed, 50 insertions(+), 39 deletions(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_cm.c
>b/drivers/infiniband/sw/siw/siw_cm.c
>index b7e7f637bd03..5338be450285 100644
>--- a/drivers/infiniband/sw/siw/siw_cm.c
>+++ b/drivers/infiniband/sw/siw/siw_cm.c
>@@ -110,26 +110,67 @@ static void siw_socket_disassoc(struct socket
>*s)
> static void __siw_cep_terminate_upcall(struct siw_cep *cep,
> 				       int reply_status)
> {
>-	if (cep->qp && cep->qp->term_info.valid)
>-		siw_send_terminate(cep->qp);
>+	bool suspended = false;

this is development debugging only. please remove it.

>+
>+	if (cep->qp) {
>+		struct siw_qp *qp = cep->qp;
>+
>+		if (qp->term_info.valid)
>+			siw_send_terminate(qp);
>+
>+		if (qp->rx_stream.rx_suspend || qp->tx_ctx.tx_suspend)
>+			suspended = true;
>+	} else {
>+		suspended = true;
>+	}

This block is not needed.
> 
> 	switch (cep->state) {
> 	case SIW_EPSTATE_AWAIT_MPAREP:
>+		/*
>+		 * MPA reply not received, but connection drop,
>+		 * or timeout.
>+		 */
> 		siw_cm_upcall(cep, IW_CM_EVENT_CONNECT_REPLY,
> 			      reply_status);
> 		break;
> 
> 	case SIW_EPSTATE_RDMA_MODE:
>+		/*
>+		 * NOTE: IW_CM_EVENT_DISCONNECT is given just
>+		 *       to transition IWCM into CLOSING.
>+		 */
>+		WARN(!suspended, "SIW_EPSTATE_RDMA_MODE called without
>suspended\n");

Please remove this development debugging. also below.

>+		siw_cm_upcall(cep, IW_CM_EVENT_DISCONNECT, 0);
> 		siw_cm_upcall(cep, IW_CM_EVENT_CLOSE, 0);
> 		break;
> 
>+	case SIW_EPSTATE_RECVD_MPAREQ:
>+		/*
>+		 * Wait for the ulp/CM to call accept/reject
>+		 */
>+		siw_dbg_cep(cep, "mpa req recvd, wait for ULP\n");
>+		WARN(!suspended, "SIW_EPSTATE_RECVD_MPAREQ called without
>suspended\n");
>+		break;
>+
>+	case SIW_EPSTATE_AWAIT_MPAREQ:
>+		/*
>+		 * Socket close before MPA request received.
>+		 */
>+		siw_dbg_cep(cep, "no mpareq: drop listener\n");
>+		if (cep->listen_cep)
>+			siw_cep_put(cep->listen_cep);
>+		cep->listen_cep = NULL;
>+		break;
>+
> 	case SIW_EPSTATE_IDLE:
> 	case SIW_EPSTATE_LISTENING:
> 	case SIW_EPSTATE_CONNECTING:
>-	case SIW_EPSTATE_AWAIT_MPAREQ:
>-	case SIW_EPSTATE_RECVD_MPAREQ:
> 	case SIW_EPSTATE_CLOSED:
> 	default:
>+		/*
>+		 * for other states there is no connection
>+		 * known to the IWCM.
>+		 */
> 		break;
> 	}
> }
>@@ -1077,41 +1118,11 @@ static void siw_cm_work_handler(struct
>work_struct *w)
> 		break;
> 
> 	case SIW_CM_WORK_PEER_CLOSE:
>-		if (cep->cm_id) {
>-			if (cep->state == SIW_EPSTATE_AWAIT_MPAREP) {
>-				/*
>-				 * MPA reply not received, but connection drop
>-				 */
>-				siw_cm_upcall(cep, IW_CM_EVENT_CONNECT_REPLY,
>-					      -ECONNRESET);
>-			} else if (cep->state == SIW_EPSTATE_RDMA_MODE) {
>-				/*
>-				 * NOTE: IW_CM_EVENT_DISCONNECT is given just
>-				 *       to transition IWCM into CLOSING.
>-				 */
>-				siw_cm_upcall(cep, IW_CM_EVENT_DISCONNECT, 0);
>-				siw_cm_upcall(cep, IW_CM_EVENT_CLOSE, 0);
>-			}
>-			/*
>-			 * for other states there is no connection
>-			 * known to the IWCM.
>-			 */
>-		} else {
>-			if (cep->state == SIW_EPSTATE_RECVD_MPAREQ) {
>-				/*
>-				 * Wait for the ulp/CM to call accept/reject
>-				 */
>-				siw_dbg_cep(cep,
>-					    "mpa req recvd, wait for ULP\n");
>-			} else if (cep->state == SIW_EPSTATE_AWAIT_MPAREQ) {
>-				/*
>-				 * Socket close before MPA request received.
>-				 */
>-				siw_dbg_cep(cep, "no mpareq: drop listener\n");
>-				siw_cep_put(cep->listen_cep);
>-				cep->listen_cep = NULL;
>-			}
>-		}
>+		/*
>+		 * Peer closed the connection: TCP_CLOSE*
>+		 */
>+		__siw_cep_terminate_upcall(cep, -ECONNRESET);
>+
> 		release_cep = 1;
> 		break;
> 
>-- 
>2.25.1
>
>

Re: [PATCH 10/31] rdma/siw: use __siw_cep_terminate_upcall() for SIW_CM_WORK_MPATIMEOUT

[Bernard Metzler]

-----"Stefan Metzmacher" <metze@samba.org> wrote: -----

>To: "Bernard Metzler" <bmt@zurich.ibm.com>
>From: "Stefan Metzmacher" <metze@samba.org>
>Date: 05/07/2021 01:38AM
>Cc: linux-rdma@vger.kernel.org, "Stefan Metzmacher" <metze@samba.org>
>Subject: [EXTERNAL] [PATCH 10/31] rdma/siw: use
>__siw_cep_terminate_upcall() for SIW_CM_WORK_MPATIMEOUT
>
>It's easier to have generic logic in just one place.
>
>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 | 31
>++++++++----------------------
> 1 file changed, 8 insertions(+), 23 deletions(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_cm.c
>b/drivers/infiniband/sw/siw/siw_cm.c
>index 5338be450285..d03c7a66c6d1 100644
>--- a/drivers/infiniband/sw/siw/siw_cm.c
>+++ b/drivers/infiniband/sw/siw/siw_cm.c
>@@ -1127,31 +1127,16 @@ static void siw_cm_work_handler(struct
>work_struct *w)
> 		break;
> 
> 	case SIW_CM_WORK_MPATIMEOUT:
>+		/*
>+		 * MPA request timed out:
>+		 * Hide any partially received private data and signal
>+		 * timeout
>+		 */
> 		cep->mpa_timer = NULL;
>+		cep->mpa.hdr.params.pd_len = 0;
>+		__siw_cep_terminate_upcall(cep, -ETIMEDOUT);
> 
>-		if (cep->state == SIW_EPSTATE_AWAIT_MPAREP) {
>-			/*
>-			 * MPA request timed out:
>-			 * Hide any partially received private data and signal
>-			 * timeout
>-			 */
>-			cep->mpa.hdr.params.pd_len = 0;
>-
>-			if (cep->cm_id)
>-				siw_cm_upcall(cep, IW_CM_EVENT_CONNECT_REPLY,
>-					      -ETIMEDOUT);
>-			release_cep = 1;
>-
>-		} else if (cep->state == SIW_EPSTATE_AWAIT_MPAREQ) {
>-			/*
>-			 * No MPA request received after peer TCP stream setup.
>-			 */
>-			if (cep->listen_cep) {
>-				siw_cep_put(cep->listen_cep);
>-				cep->listen_cep = NULL;
>-			}
>-			release_cep = 1;
>-		}
>+		release_cep = 1;
> 		break;
> 
> 	default:
>-- 
>2.25.1
>
>

Okay.

Re: [PATCH 11/31] rdma/siw: introduce SIW_EPSTATE_ACCEPTING/REJECTING for rdma_accept/rdma_reject

[Bernard Metzler]

-----"Stefan Metzmacher" <metze@samba.org> wrote: -----

>To: "Bernard Metzler" <bmt@zurich.ibm.com>
>From: "Stefan Metzmacher" <metze@samba.org>
>Date: 05/07/2021 01:38AM
>Cc: linux-rdma@vger.kernel.org, "Stefan Metzmacher" <metze@samba.org>
>Subject: [EXTERNAL] [PATCH 11/31] rdma/siw: introduce
>SIW_EPSTATE_ACCEPTING/REJECTING for rdma_accept/rdma_reject
>
>When we received the MPA Request, we set SIW_EPSTATE_RECVD_MPAREQ
>and port IW_CM_EVENT_CONNECT_REQUEST to the IWCM layer.
>
>In that state we expect the caller to reacted with rdma_accept() or
>rdma_reject(), which will turn the connection into
>SIW_EPSTATE_RDMA_MODE
>or SIW_EPSTATE_CLOSED finally.
>
>I think it much saner that rdma_accept and rdma_reject change the
>state
>instead of keeping it as SIW_EPSTATE_RECVD_MPAREQ in order to make
>the logic more understandable and allow more useful debug messages.
>
>In all cases we need to inform the IWCM layer about that error!
>As it only allows IW_CM_EVENT_ESTABLISHED to be posted after
>IW_CM_EVENT_CONNECT_REQUEST was posted, we need to go through
>IW_CM_EVENT_ESTABLISHED via IW_CM_EVENT_DISCONNECT to
>IW_CM_EVENT_CLOSE.
>
>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 | 31
>++++++++++++++++++++++++++++--
> drivers/infiniband/sw/siw/siw_cm.h |  2 ++
> 2 files changed, 31 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_cm.c
>b/drivers/infiniband/sw/siw/siw_cm.c
>index d03c7a66c6d1..3cc1d22fe232 100644
>--- a/drivers/infiniband/sw/siw/siw_cm.c
>+++ b/drivers/infiniband/sw/siw/siw_cm.c
>@@ -146,10 +146,35 @@ static void __siw_cep_terminate_upcall(struct
>siw_cep *cep,
> 
> 	case SIW_EPSTATE_RECVD_MPAREQ:
> 		/*
>-		 * Wait for the ulp/CM to call accept/reject
>+		 * Waited for the ulp/CM to call accept/reject
> 		 */
>-		siw_dbg_cep(cep, "mpa req recvd, wait for ULP\n");
> 		WARN(!suspended, "SIW_EPSTATE_RECVD_MPAREQ called without
>suspended\n");
Please remove all that WARN(!suspended, ..)

>+		siw_dbg_cep(cep, "mpa req recvd, post
>established/disconnect/close\n");
>+		siw_cm_upcall(cep, IW_CM_EVENT_ESTABLISHED, 0);
>+		siw_cm_upcall(cep, IW_CM_EVENT_DISCONNECT, 0);
>+		siw_cm_upcall(cep, IW_CM_EVENT_CLOSE, 0);
>+		break;
>+
>+	case SIW_EPSTATE_ACCEPTING:
>+		/*
>+		 * We failed during the rdma_accept/siw_accept handling
>+		 */
>+		WARN(!suspended, "SIW_EPSTATE_ACCEPTING called without
>suspended\n");
>+		siw_dbg_cep(cep, "accepting, post
>established/disconnect/close\n");
>+		siw_cm_upcall(cep, IW_CM_EVENT_ESTABLISHED, 0);
>+		siw_cm_upcall(cep, IW_CM_EVENT_DISCONNECT, 0);
>+		siw_cm_upcall(cep, IW_CM_EVENT_CLOSE, 0);
>+		break;
>+
>+	case SIW_EPSTATE_REJECTING:
>+		/*
>+		 * We failed during the rdma_reject/siw_reject handling
>+		 */
>+		WARN(!suspended, "SIW_EPSTATE_REJECTING called without
>suspended\n");
>+		siw_dbg_cep(cep, "rejecting, post
>established/disconnect/close\n");
>+		siw_cm_upcall(cep, IW_CM_EVENT_ESTABLISHED, 0);
>+		siw_cm_upcall(cep, IW_CM_EVENT_DISCONNECT, 0);
>+		siw_cm_upcall(cep, IW_CM_EVENT_CLOSE, 0);
> 		break;
> 
> 	case SIW_EPSTATE_AWAIT_MPAREQ:
>@@ -1563,6 +1588,7 @@ int siw_accept(struct iw_cm_id *id, struct
>iw_cm_conn_param *params)
> 
> 		return -ECONNRESET;
> 	}
>+	cep->state = SIW_EPSTATE_ACCEPTING;
> 	qp = siw_qp_id2obj(sdev, params->qpn);
> 	if (!qp) {
> 		WARN(1, "[QP %d] does not exist\n", params->qpn);
>@@ -1743,6 +1769,7 @@ int siw_reject(struct iw_cm_id *id, const void
>*pdata, u8 pd_len)
> 	}
> 	siw_dbg_cep(cep, "cep->state %d, pd_len %d\n", cep->state,
> 		    pd_len);
>+	cep->state = SIW_EPSTATE_REJECTING;
> 
> 	if (__mpa_rr_revision(cep->mpa.hdr.params.bits) >= MPA_REVISION_1)
>{
> 		cep->mpa.hdr.params.bits |= MPA_RR_FLAG_REJECT; /* reject */
>diff --git a/drivers/infiniband/sw/siw/siw_cm.h
>b/drivers/infiniband/sw/siw/siw_cm.h
>index 8c59cb3e2868..4f6219bd746b 100644
>--- a/drivers/infiniband/sw/siw/siw_cm.h
>+++ b/drivers/infiniband/sw/siw/siw_cm.h
>@@ -20,6 +20,8 @@ enum siw_cep_state {
> 	SIW_EPSTATE_AWAIT_MPAREQ,
> 	SIW_EPSTATE_RECVD_MPAREQ,
> 	SIW_EPSTATE_AWAIT_MPAREP,
>+	SIW_EPSTATE_ACCEPTING,
>+	SIW_EPSTATE_REJECTING,
> 	SIW_EPSTATE_RDMA_MODE,
> 	SIW_EPSTATE_CLOSED
> };
>-- 
>2.25.1
>
>

Re: [PATCH 12/31] rdma/siw: add some debugging of state and sk_state to the teardown process

[Bernard Metzler]

-----"Stefan Metzmacher" <metze@samba.org> wrote: -----

>To: "Bernard Metzler" <bmt@zurich.ibm.com>
>From: "Stefan Metzmacher" <metze@samba.org>
>Date: 05/07/2021 01:38AM
>Cc: linux-rdma@vger.kernel.org, "Stefan Metzmacher" <metze@samba.org>
>Subject: [EXTERNAL] [PATCH 12/31] rdma/siw: add some debugging of
>state and sk_state to the teardown process
>
>That makes it easier to understanf where possible problems come from.
>
>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 | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_cm.c
>b/drivers/infiniband/sw/siw/siw_cm.c
>index 3cc1d22fe232..ed33533ff9e6 100644
>--- a/drivers/infiniband/sw/siw/siw_cm.c
>+++ b/drivers/infiniband/sw/siw/siw_cm.c
>@@ -111,6 +111,14 @@ static void __siw_cep_terminate_upcall(struct
>siw_cep *cep,
> 				       int reply_status)
> {
> 	bool suspended = false;
>+	int sk_state_val = UINT_MAX;
>+
>+	if (cep->sock && cep->sock->sk)
>+		sk_state_val = cep->sock->sk->sk_state;
>+

Please put this 'sk_state_val' logic into the single debug
statement, if we need that information at all.

>+	siw_dbg_cep(cep, "[QP %u]: state: %d sk_state: %d\n",
>+		    cep->qp ? qp_id(cep->qp) : UINT_MAX,
>+		    cep->state, sk_state_val);
> 
> 	if (cep->qp) {
> 		struct siw_qp *qp = cep->qp;
>@@ -118,9 +126,14 @@ static void __siw_cep_terminate_upcall(struct
>siw_cep *cep,
> 		if (qp->term_info.valid)
> 			siw_send_terminate(qp);
> 
>+		siw_dbg_cep(cep,
>+			    "with qp rx_suspend=%d tx_suspend=%d\n",
>+			    qp->rx_stream.rx_suspend,
>+			    qp->tx_ctx.tx_suspend);
> 		if (qp->rx_stream.rx_suspend || qp->tx_ctx.tx_suspend)
> 			suspended = true;
> 	} else {
>+		siw_dbg_cep(cep, "without qp\n");
> 		suspended = true;
> 	}
> 
>@@ -1307,7 +1320,7 @@ static void siw_cm_llp_state_change(struct sock
>*sk)
> 	}
> 	orig_state_change = cep->sk_state_change;
> 
>-	siw_dbg_cep(cep, "state: %d\n", cep->state);
>+	siw_dbg_cep(cep, "state: %d sk_state: %d\n", cep->state,
>sk->sk_state);
> 
> 	switch (sk->sk_state) {
> 	case TCP_ESTABLISHED:
>-- 
>2.25.1
>
>

Re: [PATCH 15/31] rdma/siw: create a temporary copy of private data

[Bernard Metzler]

-----"Stefan Metzmacher" <metze@samba.org> wrote: -----

>To: "Bernard Metzler" <bmt@zurich.ibm.com>
>From: "Stefan Metzmacher" <metze@samba.org>
>Date: 05/07/2021 01:38AM
>Cc: linux-rdma@vger.kernel.org, "Stefan Metzmacher" <metze@samba.org>
>Subject: [EXTERNAL] [PATCH 15/31] rdma/siw: create a temporary copy
>of private data
>
>The final patch will implement a non-blocking connect,
>which means that siw_connect() will be split into
>siw_connect() and siw_connected().
>
>kernel_bindconnect() will be the last action
>in siw_connect(), while the MPA negotiation
>is deferred to siw_connected().
>

While it adds complexity, I really like the non-blocking
connect(). It would be much easier to review if a single
patch would introduce that on top of the previous work.

>We should not rely on the callers private data
>pointers to be still valid when siw_connected()
>is called, so we better create a copy.
>
>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 | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_cm.c
>b/drivers/infiniband/sw/siw/siw_cm.c
>index 027bc18cb801..41d3436985a6 100644
>--- a/drivers/infiniband/sw/siw/siw_cm.c
>+++ b/drivers/infiniband/sw/siw/siw_cm.c
>@@ -1519,13 +1519,25 @@ int siw_connect(struct iw_cm_id *id, struct
>iw_cm_conn_param *params)
> 	}
> 	memcpy(cep->mpa.hdr.key, MPA_KEY_REQ, 16);
> 
>+	cep->mpa.pdata = kmemdup(params->private_data, pd_len, GFP_KERNEL);
>+	if (IS_ERR_OR_NULL(cep->mpa.pdata)) {
>+		rv = -ENOMEM;
>+		goto error;
>+	}
>+	cep->mpa.hdr.params.pd_len = pd_len;
>+
> 	cep->state = SIW_EPSTATE_AWAIT_MPAREP;
> 
>-	rv = siw_send_mpareqrep(cep, params->private_data, pd_len);
>+	rv = siw_send_mpareqrep(cep, cep->mpa.pdata,
>+				cep->mpa.hdr.params.pd_len);
> 	/*
> 	 * Reset private data.
> 	 */
>-	cep->mpa.hdr.params.pd_len = 0;
>+	if (cep->mpa.hdr.params.pd_len) {
>+		cep->mpa.hdr.params.pd_len = 0;
>+		kfree(cep->mpa.pdata);
>+		cep->mpa.pdata = NULL;
>+	}
> 
> 	if (rv >= 0) {
> 		rv = siw_cm_queue_work(cep, SIW_CM_WORK_MPATIMEOUT);
>-- 
>2.25.1
>
>

Re: [PATCH 16/31] rdma/siw: use error and out logic at the end of siw_connect()

[Bernard Metzler]

-----"Stefan Metzmacher" <metze@samba.org> wrote: -----

>To: "Bernard Metzler" <bmt@zurich.ibm.com>
>From: "Stefan Metzmacher" <metze@samba.org>
>Date: 05/07/2021 01:38AM
>Cc: linux-rdma@vger.kernel.org, "Stefan Metzmacher" <metze@samba.org>
>Subject: [EXTERNAL] [PATCH 16/31] rdma/siw: use error and out logic
>at the end of siw_connect()
>
>This will make the following changes easier.
>
>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 | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_cm.c
>b/drivers/infiniband/sw/siw/siw_cm.c
>index 41d3436985a6..ec6d5c26fe22 100644
>--- a/drivers/infiniband/sw/siw/siw_cm.c
>+++ b/drivers/infiniband/sw/siw/siw_cm.c
>@@ -1539,14 +1539,19 @@ int siw_connect(struct iw_cm_id *id, struct
>iw_cm_conn_param *params)
> 		cep->mpa.pdata = NULL;
> 	}
> 
>-	if (rv >= 0) {
>-		rv = siw_cm_queue_work(cep, SIW_CM_WORK_MPATIMEOUT);
>-		if (!rv) {
>-			siw_dbg_cep(cep, "[QP %u]: exit\n", qp_id(qp));
>-			siw_cep_set_free(cep);
>-			return 0;
>-		}
>+	if (rv < 0) {
>+		goto error;
>+	}
>+
>+	rv = siw_cm_queue_work(cep, SIW_CM_WORK_MPATIMEOUT);
>+	if (rv != 0) {
>+		goto error;
> 	}
>+
>+	siw_dbg_cep(cep, "[QP %u]: exit\n", qp_id(qp));
>+	siw_cep_set_free(cep);
>+	return 0;
>+
> error:
> 	siw_dbg(id->device, "failed: %d\n", rv);
> 
>-- 
>2.25.1
>
>
Okay

Reviewed-by: Bernard Metzler <bmt@zurich.ibm.com>

Re: [PATCH 17/31] rdma/siw: start mpa timer before calling siw_send_mpareqrep()

[Bernard Metzler]

-----"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 17/31] rdma/siw: start mpa timer before
>calling siw_send_mpareqrep()
>
>The mpa timer will also span the non-blocking connect
>in the final patch.
>
>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 | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_cm.c
>b/drivers/infiniband/sw/siw/siw_cm.c
>index ec6d5c26fe22..853b80fcb8b0 100644
>--- a/drivers/infiniband/sw/siw/siw_cm.c
>+++ b/drivers/infiniband/sw/siw/siw_cm.c
>@@ -1526,6 +1526,11 @@ int siw_connect(struct iw_cm_id *id, struct
>iw_cm_conn_param *params)
> 	}
> 	cep->mpa.hdr.params.pd_len = pd_len;
> 
>+	rv = siw_cm_queue_work(cep, SIW_CM_WORK_MPATIMEOUT);
>+	if (rv != 0) {
>+		goto error;
>+	}
>+
> 	cep->state = SIW_EPSTATE_AWAIT_MPAREP;
> 
> 	rv = siw_send_mpareqrep(cep, cep->mpa.pdata,
>@@ -1543,11 +1548,6 @@ int siw_connect(struct iw_cm_id *id, struct
>iw_cm_conn_param *params)
> 		goto error;
> 	}
> 
>-	rv = siw_cm_queue_work(cep, SIW_CM_WORK_MPATIMEOUT);
>-	if (rv != 0) {
>-		goto error;
>-	}
>-
> 	siw_dbg_cep(cep, "[QP %u]: exit\n", qp_id(qp));
> 	siw_cep_set_free(cep);
> 	return 0;
>@@ -1556,6 +1556,8 @@ int siw_connect(struct iw_cm_id *id, struct
>iw_cm_conn_param *params)
> 	siw_dbg(id->device, "failed: %d\n", rv);
> 
> 	if (cep) {
>+		siw_cancel_mpatimer(cep);
>+
> 		siw_socket_disassoc(s);
> 		sock_release(s);
> 		cep->sock = NULL;
>-- 
>2.25.1
>
>
Makes sense if we have a non-blocking connect()

Reviewed-by: Bernard Metzler <bmt@zurich.ibm.com>

Re: [PATCH 19/31] rdma/siw: split out a __siw_cep_close() function

[Bernard Metzler]

-----"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 19/31] rdma/siw: split out a
>__siw_cep_close() function
>
>This can be used in a lot of other places too.
>And can be the code path that we can easily adjust
>without forgetting other places.
>
>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 | 48
>++++++++++++++++++++----------
> 1 file changed, 33 insertions(+), 15 deletions(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_cm.c
>b/drivers/infiniband/sw/siw/siw_cm.c
>index 009a0afe6669..cf0f881c6793 100644
>--- a/drivers/infiniband/sw/siw/siw_cm.c
>+++ b/drivers/infiniband/sw/siw/siw_cm.c
>@@ -220,6 +220,34 @@ static void __siw_cep_terminate_upcall(struct
>siw_cep *cep,
> 	}
> }
> 
>+/*
>+ * The caller needs to deal with siw_cep_set_inuse()
>+ * and siw_cep_set_free()
>+ */
>+static void __siw_cep_close(struct siw_cep *cep)
>+{
>+	cep->state = SIW_EPSTATE_CLOSED;
>+
>+	if (cep->sock) {
>+		siw_socket_disassoc(cep->sock);
>+		sock_release(cep->sock);
>+		cep->sock = NULL;
>+	}
>+
>+	if (cep->cm_id) {
>+		cep->cm_id->rem_ref(cep->cm_id);
>+		cep->cm_id = NULL;
>+	}
>+
>+	if (cep->qp) {
>+		BUG_ON(cep->qp->cep != cep);

Don't (re)introduce BUG()into the driver.


>+		cep->qp->cep = NULL;
>+		siw_qp_put(cep->qp);
>+		cep->qp = NULL;
>+		siw_cep_put(cep);
>+	}
>+}
>+
> static void siw_rtr_data_ready(struct sock *sk)
> {
> 	struct siw_cep *cep;
>@@ -1559,27 +1587,17 @@ int siw_connect(struct iw_cm_id *id, struct
>iw_cm_conn_param *params)
> 	if (cep) {
> 		siw_cancel_mpatimer(cep);
> 
>-		siw_socket_disassoc(s);
>-		sock_release(s);
>-		cep->sock = NULL;
>-
>-		cep->qp = NULL;
>+		s = NULL;
>+		qp = NULL;
> 
>-		cep->cm_id = NULL;
>-		id->rem_ref(id);
>-
>-		qp->cep = NULL;
>-		siw_cep_put(cep);
>-
>-		cep->state = SIW_EPSTATE_CLOSED;
>+		__siw_cep_close(cep);
> 
> 		siw_cep_set_free(cep);
> 
> 		siw_cep_put(cep);
>-
>-	} else if (s) {
>-		sock_release(s);
> 	}
>+	if (s)
>+		sock_release(s);
> 	if (qp)
> 		siw_qp_put(qp);
> 
>-- 
>2.25.1
>
>

Re: [PATCH 20/31] rdma/siw: implement non-blocking connect.

[Bernard Metzler]

-----"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 20/31] rdma/siw: implement non-blocking
>connect.
>
>This is very important in order to prevent deadlocks.
>
>The RDMA application layer expects rdma_connect() to be non-blocking
>as the completion is handled via RDMA_CM_EVENT_ESTABLISHED and
>other async events. It's not unlikely to hold a lock during
>the rdma_connect() call.
>
>Without out this a connection attempt to a non-existing/reachable
>server block until the very long tcp timeout hits.
>The application layer had no chance to have its own timeout handler
>as that would just deadlock with the already blocking rdma_connect().
>
>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 | 114
>+++++++++++++++++++++--------
> drivers/infiniband/sw/siw/siw_cm.h |   1 +
> 2 files changed, 85 insertions(+), 30 deletions(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_cm.c
>b/drivers/infiniband/sw/siw/siw_cm.c
>index cf0f881c6793..9a550f040678 100644
>--- a/drivers/infiniband/sw/siw/siw_cm.c
>+++ b/drivers/infiniband/sw/siw/siw_cm.c
>@@ -37,6 +37,7 @@ static void siw_cm_llp_write_space(struct sock *s);
> static void siw_cm_llp_error_report(struct sock *s);
> static int siw_cm_upcall(struct siw_cep *cep, enum iw_cm_event_type
>reason,
> 			 int status);
>+static void siw_connected(struct siw_cep *cep);
> 
> static void siw_sk_assign_cm_upcalls(struct sock *sk)
> {
>@@ -1141,6 +1142,10 @@ static void siw_cm_work_handler(struct
>work_struct *w)
> 		siw_accept_newconn(cep);
> 		break;
> 
>+	case SIW_CM_WORK_CONNECTED:
>+		siw_connected(cep);
>+		break;
>+
> 	case SIW_CM_WORK_READ_MPAHDR:
> 		if (cep->state == SIW_EPSTATE_AWAIT_MPAREQ) {
> 			if (cep->listen_cep) {
>@@ -1306,6 +1311,7 @@ static void siw_cm_llp_data_ready(struct sock
>*sk)
> 	switch (cep->state) {
> 	case SIW_EPSTATE_RDMA_MODE:
> 	case SIW_EPSTATE_LISTENING:
>+	case SIW_EPSTATE_CONNECTING:
> 		break;
> 
> 	case SIW_EPSTATE_AWAIT_MPAREQ:
>@@ -1359,12 +1365,26 @@ static void siw_cm_llp_state_change(struct
>sock *sk)
> 
> 	switch (sk->sk_state) {
> 	case TCP_ESTABLISHED:
>-		/*
>-		 * handle accepting socket as special case where only
>-		 * new connection is possible
>-		 */
>-		siw_cm_queue_work(cep, SIW_CM_WORK_ACCEPT);
>-		break;
>+		if (cep->state == SIW_EPSTATE_CONNECTING) {
>+			/*
>+			 * handle accepting socket as special case where only
>+			 * new connection is possible
>+			 */
>+			siw_cm_queue_work(cep, SIW_CM_WORK_CONNECTED);
>+			break;
>+
>+		} else if (cep->state == SIW_EPSTATE_LISTENING) {
>+			/*
>+			 * handle accepting socket as special case where only
>+			 * new connection is possible
>+			 */
>+			siw_cm_queue_work(cep, SIW_CM_WORK_ACCEPT);
>+			break;
>+		}
>+		siw_dbg_cep(cep,
>+			    "unexpected socket state %d with cep state %d\n",
>+			    sk->sk_state, cep->state);
>+		/* fall through */
> 
> 	case TCP_CLOSE:
> 	case TCP_CLOSE_WAIT:
>@@ -1383,7 +1403,7 @@ static void siw_cm_llp_state_change(struct sock
>*sk)
> static int kernel_bindconnect(struct socket *s, struct sockaddr
>*laddr,
> 			      struct sockaddr *raddr, bool afonly)
> {
>-	int rv, flags = 0;
>+	int rv;
> 	size_t size = laddr->sa_family == AF_INET ?
> 		sizeof(struct sockaddr_in) : sizeof(struct sockaddr_in6);
> 
>@@ -1402,7 +1422,7 @@ static int kernel_bindconnect(struct socket *s,
>struct sockaddr *laddr,
> 	if (rv < 0)
> 		return rv;
> 
>-	rv = kernel_connect(s, raddr, size, flags);
>+	rv = kernel_connect(s, raddr, size, O_NONBLOCK);
> 
> 	return rv < 0 ? rv : 0;
> }
>@@ -1547,36 +1567,27 @@ int siw_connect(struct iw_cm_id *id, struct
>iw_cm_conn_param *params)
> 		goto error;
> 	}
> 
>-	/*
>-	 * NOTE: For simplification, connect() is called in blocking
>-	 * mode. Might be reconsidered for async connection setup at
>-	 * TCP level.
>-	 */
> 	rv = kernel_bindconnect(s, laddr, raddr, id->afonly);
>+	if (rv == -EINPROGRESS) {
>+		siw_dbg_qp(qp, "kernel_bindconnect: EINPROGRESS\n");
>+		rv = 0;
>+	}
> 	if (rv != 0) {
> 		siw_dbg_qp(qp, "kernel_bindconnect: error %d\n", rv);
> 		goto error;
> 	}
>-	if (siw_tcp_nagle == false)
>-		tcp_sock_set_nodelay(s->sk);
>-
>-	cep->state = SIW_EPSTATE_AWAIT_MPAREP;
> 
>-	rv = siw_send_mpareqrep(cep, cep->mpa.pdata,
>-				cep->mpa.hdr.params.pd_len);
> 	/*
>-	 * Reset private data.
>+	 * The rest will be done by siw_connected()

Please use more concise language, giving some details.
The 'rest' and 'everything' refers to some state of code
understanding we cannot assume for every reader ;)


>+	 *
>+	 * siw_cm_llp_state_change() will detect
>+	 * TCP_ESTABLISHED and schedules SIW_CM_WORK_CONNECTED,
>+	 * which will finally call siw_connected().
>+	 *
>+	 * As siw_cm_llp_state_change() handles everything
>+	 * siw_cm_llp_data_ready() can be a noop for
>+	 * SIW_EPSTATE_CONNECTING.
> 	 */
>-	if (cep->mpa.hdr.params.pd_len) {
>-		cep->mpa.hdr.params.pd_len = 0;
>-		kfree(cep->mpa.pdata);
>-		cep->mpa.pdata = NULL;
>-	}
>-
>-	if (rv < 0) {
>-		goto error;
>-	}
>-
> 	siw_dbg_cep(cep, "[QP %u]: exit\n", qp_id(qp));
> 	siw_cep_set_free(cep);
> 	return 0;
>@@ -1604,6 +1615,49 @@ int siw_connect(struct iw_cm_id *id, struct
>iw_cm_conn_param *params)
> 	return rv;
> }
> 
>+static void siw_connected(struct siw_cep *cep)
>+{
>+	struct siw_qp *qp = cep->qp;
>+	struct socket *s = cep->sock;
>+	int rv = -ECONNABORTED;
>+
>+	/*
>+	 * already called with
>+	 * siw_cep_set_inuse(cep);
>+	 */
>+
>+	if (cep->state != SIW_EPSTATE_CONNECTING)
>+		goto error;
>+
>+	if (siw_tcp_nagle == false)
>+		tcp_sock_set_nodelay(s->sk);
>+
>+	cep->state = SIW_EPSTATE_AWAIT_MPAREP;
>+
>+	rv = siw_send_mpareqrep(cep, cep->mpa.pdata,
>+				cep->mpa.hdr.params.pd_len);
>+	/*
>+	 * Reset private data.
>+	 */
>+	if (cep->mpa.hdr.params.pd_len) {
>+		cep->mpa.hdr.params.pd_len = 0;
>+		kfree(cep->mpa.pdata);
>+		cep->mpa.pdata = NULL;
>+	}
>+
>+	if (rv < 0) {
>+		goto error;
>+	}
>+
>+	siw_dbg_cep(cep, "[QP %u]: exit\n", qp_id(qp));
>+	return;
>+
>+error:
>+	siw_dbg_cep(cep, "[QP %u]: exit, error %d\n", qp_id(qp), rv);
>+	siw_qp_cm_drop(qp, 1);
>+	return;
>+}
>+
> /*
>  * siw_accept - Let SoftiWARP accept an RDMA connection request
>  *
>diff --git a/drivers/infiniband/sw/siw/siw_cm.h
>b/drivers/infiniband/sw/siw/siw_cm.h
>index 4f6219bd746b..62c9947999ac 100644
>--- a/drivers/infiniband/sw/siw/siw_cm.h
>+++ b/drivers/infiniband/sw/siw/siw_cm.h
>@@ -78,6 +78,7 @@ struct siw_cep {
> 
> enum siw_work_type {
> 	SIW_CM_WORK_ACCEPT = 1,
>+	SIW_CM_WORK_CONNECTED,
> 	SIW_CM_WORK_READ_MPAHDR,
> 	SIW_CM_WORK_CLOSE_LLP, /* close socket */
> 	SIW_CM_WORK_PEER_CLOSE, /* socket indicated peer close */
>-- 
>2.25.1
>
>

Re: [PATCH 22/31] rdma/siw: let siw_listen_address() call siw_cep_set_inuse() early

[Bernard Metzler]

-----"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 22/31] rdma/siw: let siw_listen_address()
>call siw_cep_set_inuse() early
>
>We should protect the whole section after siw_cep_alloc().
>
>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 | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_cm.c
>b/drivers/infiniband/sw/siw/siw_cm.c
>index fe6f7bb4d615..09ae7f7ca82a 100644
>--- a/drivers/infiniband/sw/siw/siw_cm.c
>+++ b/drivers/infiniband/sw/siw/siw_cm.c
>@@ -1920,6 +1920,8 @@ int siw_create_listen(struct iw_cm_id *id, int
>backlog)
> 	if (!cep)
> 		return -ENOMEM;
> 
>+	siw_cep_set_inuse(cep);
>+
> 	rv = sock_create(addr_family, SOCK_STREAM, IPPROTO_TCP, &s);
> 	if (rv < 0) {
> 		siw_dbg(id->device, "sock_create error: %d\n", rv);
>@@ -2014,13 +2016,12 @@ int siw_create_listen(struct iw_cm_id *id,
>int backlog)
> 
> 	siw_dbg(id->device, "Listen at laddr %pISp\n", &id->local_addr);
> 
>+	siw_cep_set_free(cep);
> 	return 0;
> 
> error:
> 	siw_dbg(id->device, "failed: %d\n", rv);
> 
>-	siw_cep_set_inuse(cep);
>-
> 	if (cep->cm_id) {
> 		cep->cm_id->rem_ref(cep->cm_id);
> 		cep->cm_id = NULL;
>-- 
>2.25.1
>
>
Agreed, makes it easier to read.

Reviewed-by: Bernard Metzler <bmt@zurich.ibm.com>

Re: [PATCH 23/31] rdma/siw: make use of __siw_cep_close() in siw_accept()

[Bernard Metzler]

-----"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 23/31] rdma/siw: make use of
>__siw_cep_close() in siw_accept()
>
>This is basically the same just that the code in
>__siw_cep_close() common, it skips elements which
>are still NULL. Before it was really hard to prove
>that we don't deference NULL pointers.
>
>While developing my smbdirect driver, I hit so much
>crashes and deadlocks, so we better have code that's
>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 | 18 +-----------------
> 1 file changed, 1 insertion(+), 17 deletions(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_cm.c
>b/drivers/infiniband/sw/siw/siw_cm.c
>index 09ae7f7ca82a..7fd67499f1d3 100644
>--- a/drivers/infiniband/sw/siw/siw_cm.c
>+++ b/drivers/infiniband/sw/siw/siw_cm.c
>@@ -1833,23 +1833,7 @@ int siw_accept(struct iw_cm_id *id, struct
>iw_cm_conn_param *params)
> 
> 	return 0;
> error:
>-	siw_socket_disassoc(cep->sock);
>-	sock_release(cep->sock);
>-	cep->sock = NULL;
>-
>-	cep->state = SIW_EPSTATE_CLOSED;
>-
>-	if (cep->cm_id) {
>-		cep->cm_id->rem_ref(id);
>-		cep->cm_id = NULL;
>-	}
>-	if (qp->cep) {
>-		siw_cep_put(cep);
>-		qp->cep = NULL;
>-	}
>-	cep->qp = NULL;
>-	siw_qp_put(qp);
>-
>+	__siw_cep_close(cep);
> 	siw_cep_set_free(cep);
> 	siw_cep_put(cep);
> 
>-- 
>2.25.1
>
>
OK, makes life easier.

Reviewed-by: Bernard Metzler <bmt@zurich.ibm.com>

Re: [PATCH 24/31] rdma/siw: do the full disassociation of cep and qp in siw_qp_llp_close()

[Bernard Metzler]

-----"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
>
>

Re: [PATCH 25/31] rdma/siw: fix double siw_cep_put() in siw_cm_work_handler()

[Bernard Metzler]

-----"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 25/31] rdma/siw: fix double siw_cep_put()
>in siw_cm_work_handler()
>
>We never do an additional siw_cep_get(cep) when calling
>id->add_ref(id),
>there's no reason to call siw_cep_put(cep) when calling
>cep->cm_id->rem_ref(cep->cm_id)!
>
>I saw this happening quite often while testing my smbdirect driver
>and the peer already reseted the tcp connection.
>

Uhh...you got a WARN()? 
Thanks!


>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 | 1 -
> 1 file changed, 1 deletion(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_cm.c
>b/drivers/infiniband/sw/siw/siw_cm.c
>index 31135d877d41..a2a5a36370af 100644
>--- a/drivers/infiniband/sw/siw/siw_cm.c
>+++ b/drivers/infiniband/sw/siw/siw_cm.c
>@@ -1252,7 +1252,6 @@ static void siw_cm_work_handler(struct
>work_struct *w)
> 		if (cep->cm_id) {
> 			cep->cm_id->rem_ref(cep->cm_id);
> 			cep->cm_id = NULL;
>-			siw_cep_put(cep);
> 		}
> 	}
> 	siw_cep_set_free(cep);
>-- 
>2.25.1
>
>

Re: [PATCH 27/31] rdma/siw: fix the "close" logic in siw_qp_cm_drop()

[Bernard Metzler]

-----"Stefan Metzmacher" <metze@samba.org> wrote: -----

>To: "Bernard Metzler" <bmt@zurich.ibm.com>
>From: "Stefan Metzmacher" <metze@samba.org>
>Date: 05/07/2021 01:40AM
>Cc: linux-rdma@vger.kernel.org, "Stefan Metzmacher" <metze@samba.org>
>Subject: [EXTERNAL] [PATCH 27/31] rdma/siw: fix the "close" logic in
>siw_qp_cm_drop()
>
>cep->cm_id->rem_ref(cep->cm_id) is no reason to call
>siw_cep_put(cep), we never call siw_cep_get(cep) when
>calling id->add_ref(id).
>
>But the cep->qp cleanup needs to drop both references!
>
>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 | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_cm.c
>b/drivers/infiniband/sw/siw/siw_cm.c
>index 3dc80c21ac60..9f9750237e75 100644
>--- a/drivers/infiniband/sw/siw/siw_cm.c
>+++ b/drivers/infiniband/sw/siw/siw_cm.c
>@@ -546,7 +546,6 @@ void siw_qp_cm_drop(struct siw_qp *qp, int
>schedule)
> 		if (cep->cm_id) {
> 			cep->cm_id->rem_ref(cep->cm_id);
> 			cep->cm_id = NULL;
>-			siw_cep_put(cep);
> 		}
> 		cep->state = SIW_EPSTATE_CLOSED;
> 
>@@ -559,8 +558,11 @@ void siw_qp_cm_drop(struct siw_qp *qp, int
>schedule)
> 			cep->sock = NULL;
> 		}
> 		if (cep->qp) {
>+			BUG_ON(cep->qp->cep != cep);

Please no BUG() and friends

>+			cep->qp->cep = NULL;

That pointer should be handled by the qp code

>+			siw_qp_put(cep->qp);
> 			cep->qp = NULL;
>-			siw_qp_put(qp);
>+			siw_cep_put(cep);
> 		}
> out:
> 		siw_cep_set_free(cep);
>-- 
>2.25.1
>
>