* [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs
@ 2021-05-06 23:36 Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 01/31] rdma/siw: fix warning in siw_proc_send() Stefan Metzmacher
` (53 more replies)
0 siblings, 54 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 01/31] rdma/siw: fix warning in siw_proc_send()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 02/31] rdma/siw: call smp_mb() after mem->stag_valid = 0 in siw_invalidate_stag() too Stefan Metzmacher
` (52 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 02/31] rdma/siw: call smp_mb() after mem->stag_valid = 0 in siw_invalidate_stag() too
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 01/31] rdma/siw: fix warning in siw_proc_send() Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 03/31] rdma/siw: remove superfluous siw_cep_put() from siw_connect() error path Stefan Metzmacher
` (51 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 03/31] rdma/siw: remove superfluous siw_cep_put() from siw_connect() error path
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 01/31] rdma/siw: fix warning in siw_proc_send() Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 02/31] rdma/siw: call smp_mb() after mem->stag_valid = 0 in siw_invalidate_stag() too Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 04/31] rdma/siw: let siw_accept() deferr RDMA_MODE until EVENT_ESTABLISHED Stefan Metzmacher
` (50 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 04/31] rdma/siw: let siw_accept() deferr RDMA_MODE until EVENT_ESTABLISHED
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (2 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 03/31] rdma/siw: remove superfluous siw_cep_put() from siw_connect() error path Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 05/31] rdma/siw: make use of kernel_{bind,connect,listen}() Stefan Metzmacher
` (49 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 05/31] rdma/siw: make use of kernel_{bind,connect,listen}()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (3 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 04/31] rdma/siw: let siw_accept() deferr RDMA_MODE until EVENT_ESTABLISHED Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 06/31] rdma/siw: make siw_cm_upcall() a noop without valid 'id' Stefan Metzmacher
` (48 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 06/31] rdma/siw: make siw_cm_upcall() a noop without valid 'id'
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (4 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 05/31] rdma/siw: make use of kernel_{bind,connect,listen}() Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 07/31] rdma/siw: split out a __siw_cep_terminate_upcall() function Stefan Metzmacher
` (47 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 07/31] rdma/siw: split out a __siw_cep_terminate_upcall() function
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (5 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 06/31] rdma/siw: make siw_cm_upcall() a noop without valid 'id' Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 08/31] rdma/siw: use __siw_cep_terminate_upcall() for indirect SIW_CM_WORK_CLOSE_LLP Stefan Metzmacher
` (46 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 08/31] rdma/siw: use __siw_cep_terminate_upcall() for indirect SIW_CM_WORK_CLOSE_LLP
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (6 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 07/31] rdma/siw: split out a __siw_cep_terminate_upcall() function Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 09/31] rdma/siw: use __siw_cep_terminate_upcall() for SIW_CM_WORK_PEER_CLOSE Stefan Metzmacher
` (45 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 09/31] rdma/siw: use __siw_cep_terminate_upcall() for SIW_CM_WORK_PEER_CLOSE
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (7 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 08/31] rdma/siw: use __siw_cep_terminate_upcall() for indirect SIW_CM_WORK_CLOSE_LLP Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 10/31] rdma/siw: use __siw_cep_terminate_upcall() for SIW_CM_WORK_MPATIMEOUT Stefan Metzmacher
` (44 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 10/31] rdma/siw: use __siw_cep_terminate_upcall() for SIW_CM_WORK_MPATIMEOUT
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (8 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 09/31] rdma/siw: use __siw_cep_terminate_upcall() for SIW_CM_WORK_PEER_CLOSE Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 11/31] rdma/siw: introduce SIW_EPSTATE_ACCEPTING/REJECTING for rdma_accept/rdma_reject Stefan Metzmacher
` (43 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 11/31] rdma/siw: introduce SIW_EPSTATE_ACCEPTING/REJECTING for rdma_accept/rdma_reject
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (9 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 10/31] rdma/siw: use __siw_cep_terminate_upcall() for SIW_CM_WORK_MPATIMEOUT Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 12/31] rdma/siw: add some debugging of state and sk_state to the teardown process Stefan Metzmacher
` (42 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 12/31] rdma/siw: add some debugging of state and sk_state to the teardown process
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (10 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 11/31] rdma/siw: introduce SIW_EPSTATE_ACCEPTING/REJECTING for rdma_accept/rdma_reject Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 13/31] rdma/siw: handle SIW_EPSTATE_CONNECTING in __siw_cep_terminate_upcall() Stefan Metzmacher
` (41 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 13/31] rdma/siw: handle SIW_EPSTATE_CONNECTING in __siw_cep_terminate_upcall()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (11 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 12/31] rdma/siw: add some debugging of state and sk_state to the teardown process Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 14/31] rdma/siw: let siw_connect() set AWAIT_MPAREP before siw_send_mpareqrep() Stefan Metzmacher
` (40 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 14/31] rdma/siw: let siw_connect() set AWAIT_MPAREP before siw_send_mpareqrep()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (12 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 13/31] rdma/siw: handle SIW_EPSTATE_CONNECTING in __siw_cep_terminate_upcall() Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 15/31] rdma/siw: create a temporary copy of private data Stefan Metzmacher
` (39 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 15/31] rdma/siw: create a temporary copy of private data
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (13 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 14/31] rdma/siw: let siw_connect() set AWAIT_MPAREP before siw_send_mpareqrep() Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 16/31] rdma/siw: use error and out logic at the end of siw_connect() Stefan Metzmacher
` (38 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 16/31] rdma/siw: use error and out logic at the end of siw_connect()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (14 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 15/31] rdma/siw: create a temporary copy of private data Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 17/31] rdma/siw: start mpa timer before calling siw_send_mpareqrep() Stefan Metzmacher
` (37 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 17/31] rdma/siw: start mpa timer before calling siw_send_mpareqrep()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (15 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 16/31] rdma/siw: use error and out logic at the end of siw_connect() Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 18/31] rdma/siw: call the blocking kernel_bindconnect() just before siw_send_mpareqrep() Stefan Metzmacher
` (36 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 18/31] rdma/siw: call the blocking kernel_bindconnect() just before siw_send_mpareqrep()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (16 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 17/31] rdma/siw: start mpa timer before calling siw_send_mpareqrep() Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 19/31] rdma/siw: split out a __siw_cep_close() function Stefan Metzmacher
` (35 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 19/31] rdma/siw: split out a __siw_cep_close() function
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (17 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 18/31] rdma/siw: call the blocking kernel_bindconnect() just before siw_send_mpareqrep() Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 20/31] rdma/siw: implement non-blocking connect Stefan Metzmacher
` (34 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 20/31] rdma/siw: implement non-blocking connect.
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (18 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 19/31] rdma/siw: split out a __siw_cep_close() function Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 21/31] rdma/siw: let siw_listen_address() call siw_cep_alloc() first Stefan Metzmacher
` (33 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 21/31] rdma/siw: let siw_listen_address() call siw_cep_alloc() first
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (19 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 20/31] rdma/siw: implement non-blocking connect Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 22/31] rdma/siw: let siw_listen_address() call siw_cep_set_inuse() early Stefan Metzmacher
` (32 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 22/31] rdma/siw: let siw_listen_address() call siw_cep_set_inuse() early
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (20 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 21/31] rdma/siw: let siw_listen_address() call siw_cep_alloc() first Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 23/31] rdma/siw: make use of __siw_cep_close() in siw_accept() Stefan Metzmacher
` (31 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 23/31] rdma/siw: make use of __siw_cep_close() in siw_accept()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (21 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 22/31] rdma/siw: let siw_listen_address() call siw_cep_set_inuse() early Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 24/31] rdma/siw: do the full disassociation of cep and qp in siw_qp_llp_close() Stefan Metzmacher
` (30 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 24/31] rdma/siw: do the full disassociation of cep and qp in siw_qp_llp_close()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (22 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 23/31] rdma/siw: make use of __siw_cep_close() in siw_accept() Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 25/31] rdma/siw: fix double siw_cep_put() in siw_cm_work_handler() Stefan Metzmacher
` (29 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 25/31] rdma/siw: fix double siw_cep_put() in siw_cm_work_handler()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (23 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 24/31] rdma/siw: do the full disassociation of cep and qp in siw_qp_llp_close() Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 26/31] rdma/siw: make use of __siw_cep_close() " Stefan Metzmacher
` (28 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 26/31] rdma/siw: make use of __siw_cep_close() in siw_cm_work_handler()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (24 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 25/31] rdma/siw: fix double siw_cep_put() in siw_cm_work_handler() Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 27/31] rdma/siw: fix the "close" logic in siw_qp_cm_drop() Stefan Metzmacher
` (27 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 27/31] rdma/siw: fix the "close" logic in siw_qp_cm_drop()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (25 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 26/31] rdma/siw: make use of __siw_cep_close() " Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 28/31] rdma/siw: make use of __siw_cep_close() " Stefan Metzmacher
` (26 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 28/31] rdma/siw: make use of __siw_cep_close() in siw_qp_cm_drop()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (26 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 27/31] rdma/siw: fix the "close" logic in siw_qp_cm_drop() Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 29/31] rdma/siw: make use of __siw_cep_close() in siw_reject() Stefan Metzmacher
` (25 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 29/31] rdma/siw: make use of __siw_cep_close() in siw_reject()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (27 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 28/31] rdma/siw: make use of __siw_cep_close() " Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 30/31] rdma/siw: make use of __siw_cep_close() in siw_listen_address() Stefan Metzmacher
` (24 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 30/31] rdma/siw: make use of __siw_cep_close() in siw_listen_address()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (28 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 29/31] rdma/siw: make use of __siw_cep_close() in siw_reject() Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 31/31] rdma/siw: make use of __siw_cep_close() in siw_drop_listeners() Stefan Metzmacher
` (23 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* [PATCH 31/31] rdma/siw: make use of __siw_cep_close() in siw_drop_listeners()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (29 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 30/31] rdma/siw: make use of __siw_cep_close() in siw_listen_address() Stefan Metzmacher
@ 2021-05-06 23:36 ` Stefan Metzmacher
2021-05-07 12:08 ` [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Bernard Metzler
` (22 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-06 23:36 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, 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
^ permalink raw reply related [flat|nested] 57+ messages in thread
* Re: [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (30 preceding siblings ...)
2021-05-06 23:36 ` [PATCH 31/31] rdma/siw: make use of __siw_cep_close() in siw_drop_listeners() Stefan Metzmacher
@ 2021-05-07 12:08 ` Bernard Metzler
2021-05-26 15:43 ` Stefan Metzmacher
2021-05-28 14:35 ` Bernard Metzler
2021-05-07 12:15 ` [PATCH 01/31] rdma/siw: fix warning in siw_proc_send() Bernard Metzler
` (21 subsequent siblings)
53 siblings, 2 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-07 12:08 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
-----"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
>
>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 01/31] rdma/siw: fix warning in siw_proc_send()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (31 preceding siblings ...)
2021-05-07 12:08 ` [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Bernard Metzler
@ 2021-05-07 12:15 ` Bernard Metzler
2021-05-07 13:52 ` [PATCH 03/31] rdma/siw: remove superfluous siw_cep_put() from siw_connect() error path Bernard Metzler
` (20 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-07 12:15 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
-----"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
>
>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 03/31] rdma/siw: remove superfluous siw_cep_put() from siw_connect() error path
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (32 preceding siblings ...)
2021-05-07 12:15 ` [PATCH 01/31] rdma/siw: fix warning in siw_proc_send() Bernard Metzler
@ 2021-05-07 13:52 ` Bernard Metzler
2021-05-11 11:31 ` [PATCH 02/31] rdma/siw: call smp_mb() after mem->stag_valid = 0 in siw_invalidate_stag() too Bernard Metzler
` (19 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-07 13:52 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
-----"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>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 02/31] rdma/siw: call smp_mb() after mem->stag_valid = 0 in siw_invalidate_stag() too
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (33 preceding siblings ...)
2021-05-07 13:52 ` [PATCH 03/31] rdma/siw: remove superfluous siw_cep_put() from siw_connect() error path Bernard Metzler
@ 2021-05-11 11:31 ` Bernard Metzler
2021-05-11 11:36 ` [PATCH 05/31] rdma/siw: make use of kernel_{bind,connect,listen}() Bernard Metzler
` (18 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-11 11:31 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
-----"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>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 05/31] rdma/siw: make use of kernel_{bind,connect,listen}()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (34 preceding siblings ...)
2021-05-11 11:31 ` [PATCH 02/31] rdma/siw: call smp_mb() after mem->stag_valid = 0 in siw_invalidate_stag() too Bernard Metzler
@ 2021-05-11 11:36 ` Bernard Metzler
2021-05-11 11:38 ` [PATCH 04/31] rdma/siw: let siw_accept() deferr RDMA_MODE until EVENT_ESTABLISHED Bernard Metzler
` (17 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-11 11:36 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
---
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>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 04/31] rdma/siw: let siw_accept() deferr RDMA_MODE until EVENT_ESTABLISHED
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (35 preceding siblings ...)
2021-05-11 11:36 ` [PATCH 05/31] rdma/siw: make use of kernel_{bind,connect,listen}() Bernard Metzler
@ 2021-05-11 11:38 ` Bernard Metzler
2021-05-11 11:42 ` [PATCH 06/31] rdma/siw: make siw_cm_upcall() a noop without valid 'id' Bernard Metzler
` (16 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-11 11:38 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
-----"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>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 06/31] rdma/siw: make siw_cm_upcall() a noop without valid 'id'
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (36 preceding siblings ...)
2021-05-11 11:38 ` [PATCH 04/31] rdma/siw: let siw_accept() deferr RDMA_MODE until EVENT_ESTABLISHED Bernard Metzler
@ 2021-05-11 11:42 ` Bernard Metzler
2021-05-11 11:55 ` [PATCH 07/31] rdma/siw: split out a __siw_cep_terminate_upcall() function Bernard Metzler
` (15 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-11 11:42 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
-----"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
>
>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 07/31] rdma/siw: split out a __siw_cep_terminate_upcall() function
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (37 preceding siblings ...)
2021-05-11 11:42 ` [PATCH 06/31] rdma/siw: make siw_cm_upcall() a noop without valid 'id' Bernard Metzler
@ 2021-05-11 11:55 ` Bernard Metzler
2021-05-11 11:56 ` [PATCH 08/31] rdma/siw: use __siw_cep_terminate_upcall() for indirect SIW_CM_WORK_CLOSE_LLP Bernard Metzler
` (14 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-11 11:55 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
-----"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
>
>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 08/31] rdma/siw: use __siw_cep_terminate_upcall() for indirect SIW_CM_WORK_CLOSE_LLP
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (38 preceding siblings ...)
2021-05-11 11:55 ` [PATCH 07/31] rdma/siw: split out a __siw_cep_terminate_upcall() function Bernard Metzler
@ 2021-05-11 11:56 ` Bernard Metzler
2021-05-11 12:00 ` [PATCH 09/31] rdma/siw: use __siw_cep_terminate_upcall() for SIW_CM_WORK_PEER_CLOSE Bernard Metzler
` (13 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-11 11:56 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
-----"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
>
>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 09/31] rdma/siw: use __siw_cep_terminate_upcall() for SIW_CM_WORK_PEER_CLOSE
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (39 preceding siblings ...)
2021-05-11 11:56 ` [PATCH 08/31] rdma/siw: use __siw_cep_terminate_upcall() for indirect SIW_CM_WORK_CLOSE_LLP Bernard Metzler
@ 2021-05-11 12:00 ` Bernard Metzler
2021-05-11 12:01 ` [PATCH 10/31] rdma/siw: use __siw_cep_terminate_upcall() for SIW_CM_WORK_MPATIMEOUT Bernard Metzler
` (12 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-11 12:00 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
-----"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
>
>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 10/31] rdma/siw: use __siw_cep_terminate_upcall() for SIW_CM_WORK_MPATIMEOUT
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (40 preceding siblings ...)
2021-05-11 12:00 ` [PATCH 09/31] rdma/siw: use __siw_cep_terminate_upcall() for SIW_CM_WORK_PEER_CLOSE Bernard Metzler
@ 2021-05-11 12:01 ` Bernard Metzler
2021-05-11 12:03 ` [PATCH 11/31] rdma/siw: introduce SIW_EPSTATE_ACCEPTING/REJECTING for rdma_accept/rdma_reject Bernard Metzler
` (11 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-11 12:01 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
-----"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.
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 11/31] rdma/siw: introduce SIW_EPSTATE_ACCEPTING/REJECTING for rdma_accept/rdma_reject
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (41 preceding siblings ...)
2021-05-11 12:01 ` [PATCH 10/31] rdma/siw: use __siw_cep_terminate_upcall() for SIW_CM_WORK_MPATIMEOUT Bernard Metzler
@ 2021-05-11 12:03 ` Bernard Metzler
2021-05-11 12:07 ` [PATCH 12/31] rdma/siw: add some debugging of state and sk_state to the teardown process Bernard Metzler
` (10 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-11 12:03 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
-----"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
>
>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 12/31] rdma/siw: add some debugging of state and sk_state to the teardown process
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (42 preceding siblings ...)
2021-05-11 12:03 ` [PATCH 11/31] rdma/siw: introduce SIW_EPSTATE_ACCEPTING/REJECTING for rdma_accept/rdma_reject Bernard Metzler
@ 2021-05-11 12:07 ` Bernard Metzler
2021-05-11 12:15 ` [PATCH 15/31] rdma/siw: create a temporary copy of private data Bernard Metzler
` (9 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-11 12:07 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
-----"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
>
>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 15/31] rdma/siw: create a temporary copy of private data
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (43 preceding siblings ...)
2021-05-11 12:07 ` [PATCH 12/31] rdma/siw: add some debugging of state and sk_state to the teardown process Bernard Metzler
@ 2021-05-11 12:15 ` Bernard Metzler
2021-05-11 12:18 ` [PATCH 16/31] rdma/siw: use error and out logic at the end of siw_connect() Bernard Metzler
` (8 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-11 12:15 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
-----"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
>
>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 16/31] rdma/siw: use error and out logic at the end of siw_connect()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (44 preceding siblings ...)
2021-05-11 12:15 ` [PATCH 15/31] rdma/siw: create a temporary copy of private data Bernard Metzler
@ 2021-05-11 12:18 ` Bernard Metzler
2021-05-11 12:20 ` [PATCH 17/31] rdma/siw: start mpa timer before calling siw_send_mpareqrep() Bernard Metzler
` (7 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-11 12:18 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
-----"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>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 17/31] rdma/siw: start mpa timer before calling siw_send_mpareqrep()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (45 preceding siblings ...)
2021-05-11 12:18 ` [PATCH 16/31] rdma/siw: use error and out logic at the end of siw_connect() Bernard Metzler
@ 2021-05-11 12:20 ` Bernard Metzler
2021-05-11 12:25 ` [PATCH 19/31] rdma/siw: split out a __siw_cep_close() function Bernard Metzler
` (6 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-11 12:20 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
-----"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>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 19/31] rdma/siw: split out a __siw_cep_close() function
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (46 preceding siblings ...)
2021-05-11 12:20 ` [PATCH 17/31] rdma/siw: start mpa timer before calling siw_send_mpareqrep() Bernard Metzler
@ 2021-05-11 12:25 ` Bernard Metzler
2021-05-11 12:31 ` [PATCH 20/31] rdma/siw: implement non-blocking connect Bernard Metzler
` (5 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-11 12:25 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
-----"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
>
>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 20/31] rdma/siw: implement non-blocking connect.
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (47 preceding siblings ...)
2021-05-11 12:25 ` [PATCH 19/31] rdma/siw: split out a __siw_cep_close() function Bernard Metzler
@ 2021-05-11 12:31 ` Bernard Metzler
2021-05-11 12:42 ` [PATCH 22/31] rdma/siw: let siw_listen_address() call siw_cep_set_inuse() early Bernard Metzler
` (4 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-11 12:31 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
-----"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
>
>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 22/31] rdma/siw: let siw_listen_address() call siw_cep_set_inuse() early
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (48 preceding siblings ...)
2021-05-11 12:31 ` [PATCH 20/31] rdma/siw: implement non-blocking connect Bernard Metzler
@ 2021-05-11 12:42 ` Bernard Metzler
2021-05-11 12:43 ` [PATCH 23/31] rdma/siw: make use of __siw_cep_close() in siw_accept() Bernard Metzler
` (3 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-11 12:42 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
-----"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>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 23/31] rdma/siw: make use of __siw_cep_close() in siw_accept()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (49 preceding siblings ...)
2021-05-11 12:42 ` [PATCH 22/31] rdma/siw: let siw_listen_address() call siw_cep_set_inuse() early Bernard Metzler
@ 2021-05-11 12:43 ` Bernard Metzler
2021-05-11 12:47 ` [PATCH 24/31] rdma/siw: do the full disassociation of cep and qp in siw_qp_llp_close() Bernard Metzler
` (2 subsequent siblings)
53 siblings, 0 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-11 12:43 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
-----"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>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 24/31] rdma/siw: do the full disassociation of cep and qp in siw_qp_llp_close()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (50 preceding siblings ...)
2021-05-11 12:43 ` [PATCH 23/31] rdma/siw: make use of __siw_cep_close() in siw_accept() Bernard Metzler
@ 2021-05-11 12:47 ` Bernard Metzler
2021-05-11 12:58 ` [PATCH 25/31] rdma/siw: fix double siw_cep_put() in siw_cm_work_handler() Bernard Metzler
2021-05-11 13:02 ` [PATCH 27/31] rdma/siw: fix the "close" logic in siw_qp_cm_drop() Bernard Metzler
53 siblings, 0 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-11 12:47 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
-----"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
>
>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 25/31] rdma/siw: fix double siw_cep_put() in siw_cm_work_handler()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (51 preceding siblings ...)
2021-05-11 12:47 ` [PATCH 24/31] rdma/siw: do the full disassociation of cep and qp in siw_qp_llp_close() Bernard Metzler
@ 2021-05-11 12:58 ` Bernard Metzler
2021-05-11 13:02 ` [PATCH 27/31] rdma/siw: fix the "close" logic in siw_qp_cm_drop() Bernard Metzler
53 siblings, 0 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-11 12:58 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
-----"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
>
>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 27/31] rdma/siw: fix the "close" logic in siw_qp_cm_drop()
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
` (52 preceding siblings ...)
2021-05-11 12:58 ` [PATCH 25/31] rdma/siw: fix double siw_cep_put() in siw_cm_work_handler() Bernard Metzler
@ 2021-05-11 13:02 ` Bernard Metzler
53 siblings, 0 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-11 13:02 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
-----"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
>
>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs
2021-05-07 12:08 ` [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Bernard Metzler
@ 2021-05-26 15:43 ` Stefan Metzmacher
2021-05-28 14:35 ` Bernard Metzler
1 sibling, 0 replies; 57+ messages in thread
From: Stefan Metzmacher @ 2021-05-26 15:43 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma
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
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: Re: [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs
2021-05-07 12:08 ` [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Bernard Metzler
2021-05-26 15:43 ` Stefan Metzmacher
@ 2021-05-28 14:35 ` Bernard Metzler
1 sibling, 0 replies; 57+ messages in thread
From: Bernard Metzler @ 2021-05-28 14:35 UTC (permalink / raw)
To: Stefan Metzmacher; +Cc: linux-rdma
-----"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.
^ permalink raw reply [flat|nested] 57+ messages in thread
end of thread, other threads:[~2021-05-28 14:35 UTC | newest]
Thread overview: 57+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-06 23:36 [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 01/31] rdma/siw: fix warning in siw_proc_send() Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 02/31] rdma/siw: call smp_mb() after mem->stag_valid = 0 in siw_invalidate_stag() too Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 03/31] rdma/siw: remove superfluous siw_cep_put() from siw_connect() error path Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 04/31] rdma/siw: let siw_accept() deferr RDMA_MODE until EVENT_ESTABLISHED Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 05/31] rdma/siw: make use of kernel_{bind,connect,listen}() Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 06/31] rdma/siw: make siw_cm_upcall() a noop without valid 'id' Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 07/31] rdma/siw: split out a __siw_cep_terminate_upcall() function Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 08/31] rdma/siw: use __siw_cep_terminate_upcall() for indirect SIW_CM_WORK_CLOSE_LLP Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 09/31] rdma/siw: use __siw_cep_terminate_upcall() for SIW_CM_WORK_PEER_CLOSE Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 10/31] rdma/siw: use __siw_cep_terminate_upcall() for SIW_CM_WORK_MPATIMEOUT Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 11/31] rdma/siw: introduce SIW_EPSTATE_ACCEPTING/REJECTING for rdma_accept/rdma_reject Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 12/31] rdma/siw: add some debugging of state and sk_state to the teardown process Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 13/31] rdma/siw: handle SIW_EPSTATE_CONNECTING in __siw_cep_terminate_upcall() Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 14/31] rdma/siw: let siw_connect() set AWAIT_MPAREP before siw_send_mpareqrep() Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 15/31] rdma/siw: create a temporary copy of private data Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 16/31] rdma/siw: use error and out logic at the end of siw_connect() Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 17/31] rdma/siw: start mpa timer before calling siw_send_mpareqrep() Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 18/31] rdma/siw: call the blocking kernel_bindconnect() just before siw_send_mpareqrep() Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 19/31] rdma/siw: split out a __siw_cep_close() function Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 20/31] rdma/siw: implement non-blocking connect Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 21/31] rdma/siw: let siw_listen_address() call siw_cep_alloc() first Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 22/31] rdma/siw: let siw_listen_address() call siw_cep_set_inuse() early Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 23/31] rdma/siw: make use of __siw_cep_close() in siw_accept() Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 24/31] rdma/siw: do the full disassociation of cep and qp in siw_qp_llp_close() Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 25/31] rdma/siw: fix double siw_cep_put() in siw_cm_work_handler() Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 26/31] rdma/siw: make use of __siw_cep_close() " Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 27/31] rdma/siw: fix the "close" logic in siw_qp_cm_drop() Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 28/31] rdma/siw: make use of __siw_cep_close() " Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 29/31] rdma/siw: make use of __siw_cep_close() in siw_reject() Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 30/31] rdma/siw: make use of __siw_cep_close() in siw_listen_address() Stefan Metzmacher
2021-05-06 23:36 ` [PATCH 31/31] rdma/siw: make use of __siw_cep_close() in siw_drop_listeners() Stefan Metzmacher
2021-05-07 12:08 ` [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs Bernard Metzler
2021-05-26 15:43 ` Stefan Metzmacher
2021-05-28 14:35 ` Bernard Metzler
2021-05-07 12:15 ` [PATCH 01/31] rdma/siw: fix warning in siw_proc_send() Bernard Metzler
2021-05-07 13:52 ` [PATCH 03/31] rdma/siw: remove superfluous siw_cep_put() from siw_connect() error path Bernard Metzler
2021-05-11 11:31 ` [PATCH 02/31] rdma/siw: call smp_mb() after mem->stag_valid = 0 in siw_invalidate_stag() too Bernard Metzler
2021-05-11 11:36 ` [PATCH 05/31] rdma/siw: make use of kernel_{bind,connect,listen}() Bernard Metzler
2021-05-11 11:38 ` [PATCH 04/31] rdma/siw: let siw_accept() deferr RDMA_MODE until EVENT_ESTABLISHED Bernard Metzler
2021-05-11 11:42 ` [PATCH 06/31] rdma/siw: make siw_cm_upcall() a noop without valid 'id' Bernard Metzler
2021-05-11 11:55 ` [PATCH 07/31] rdma/siw: split out a __siw_cep_terminate_upcall() function Bernard Metzler
2021-05-11 11:56 ` [PATCH 08/31] rdma/siw: use __siw_cep_terminate_upcall() for indirect SIW_CM_WORK_CLOSE_LLP Bernard Metzler
2021-05-11 12:00 ` [PATCH 09/31] rdma/siw: use __siw_cep_terminate_upcall() for SIW_CM_WORK_PEER_CLOSE Bernard Metzler
2021-05-11 12:01 ` [PATCH 10/31] rdma/siw: use __siw_cep_terminate_upcall() for SIW_CM_WORK_MPATIMEOUT Bernard Metzler
2021-05-11 12:03 ` [PATCH 11/31] rdma/siw: introduce SIW_EPSTATE_ACCEPTING/REJECTING for rdma_accept/rdma_reject Bernard Metzler
2021-05-11 12:07 ` [PATCH 12/31] rdma/siw: add some debugging of state and sk_state to the teardown process Bernard Metzler
2021-05-11 12:15 ` [PATCH 15/31] rdma/siw: create a temporary copy of private data Bernard Metzler
2021-05-11 12:18 ` [PATCH 16/31] rdma/siw: use error and out logic at the end of siw_connect() Bernard Metzler
2021-05-11 12:20 ` [PATCH 17/31] rdma/siw: start mpa timer before calling siw_send_mpareqrep() Bernard Metzler
2021-05-11 12:25 ` [PATCH 19/31] rdma/siw: split out a __siw_cep_close() function Bernard Metzler
2021-05-11 12:31 ` [PATCH 20/31] rdma/siw: implement non-blocking connect Bernard Metzler
2021-05-11 12:42 ` [PATCH 22/31] rdma/siw: let siw_listen_address() call siw_cep_set_inuse() early Bernard Metzler
2021-05-11 12:43 ` [PATCH 23/31] rdma/siw: make use of __siw_cep_close() in siw_accept() Bernard Metzler
2021-05-11 12:47 ` [PATCH 24/31] rdma/siw: do the full disassociation of cep and qp in siw_qp_llp_close() Bernard Metzler
2021-05-11 12:58 ` [PATCH 25/31] rdma/siw: fix double siw_cep_put() in siw_cm_work_handler() Bernard Metzler
2021-05-11 13:02 ` [PATCH 27/31] rdma/siw: fix the "close" logic in siw_qp_cm_drop() Bernard Metzler
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.