Linux-RDMA Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH rdma-next] IB/cm: Consider local communication ID when check for a stale connection
@ 2020-07-16 10:41 Leon Romanovsky
  2020-07-31 17:47 ` Jason Gunthorpe
  0 siblings, 1 reply; 2+ messages in thread
From: Leon Romanovsky @ 2020-07-16 10:41 UTC (permalink / raw)
  To: Doug Ledford, Jason Gunthorpe
  Cc: Mark Zhang, Alex Rosenbaum, linux-rdma, Parav Pandit

From: Mark Zhang <markz@mellanox.com>

Made the check for duplicate/stale CM more strict by adding comparison
for remote communication ID field.

The absence of such strict check causes to the following flows not being
handled properly:
1. Client tries to setup more than one connection with same QP in
   a server (e.g., when use external QP), the client would reject
   the reply.
2. Client node reboots, and when it gets the same QP number as that
   of before the reboot time, the server would rejects the request.

Signed-off-by: Parav Pandit <parav@mellanox.com>
Signed-off-by: Mark Zhang <markz@mellanox.com>
Reviewed-by: Alex Rosenbaum <alexr@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/cm.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index 0d1377232933..23dfba947357 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -775,12 +775,17 @@ static struct cm_timewait_info * cm_insert_remote_qpn(struct cm_timewait_info
 	struct cm_timewait_info *cur_timewait_info;
 	__be64 remote_ca_guid = timewait_info->remote_ca_guid;
 	__be32 remote_qpn = timewait_info->remote_qpn;
+	__be32 remote_id = timewait_info->work.remote_id;
 
 	while (*link) {
 		parent = *link;
 		cur_timewait_info = rb_entry(parent, struct cm_timewait_info,
 					     remote_qp_node);
-		if (be32_lt(remote_qpn, cur_timewait_info->remote_qpn))
+		if (be32_lt(remote_id, cur_timewait_info->work.remote_id))
+			link = &(*link)->rb_left;
+		else if (be32_gt(remote_id, cur_timewait_info->work.remote_id))
+			link = &(*link)->rb_right;
+		else if (be32_lt(remote_qpn, cur_timewait_info->remote_qpn))
 			link = &(*link)->rb_left;
 		else if (be32_gt(remote_qpn, cur_timewait_info->remote_qpn))
 			link = &(*link)->rb_right;
-- 
2.26.2


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH rdma-next] IB/cm: Consider local communication ID when check for a stale connection
  2020-07-16 10:41 [PATCH rdma-next] IB/cm: Consider local communication ID when check for a stale connection Leon Romanovsky
@ 2020-07-31 17:47 ` Jason Gunthorpe
  0 siblings, 0 replies; 2+ messages in thread
From: Jason Gunthorpe @ 2020-07-31 17:47 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Doug Ledford, Mark Zhang, Alex Rosenbaum, linux-rdma, Parav Pandit

On Thu, Jul 16, 2020 at 01:41:58PM +0300, Leon Romanovsky wrote:
> From: Mark Zhang <markz@mellanox.com>
> 
> Made the check for duplicate/stale CM more strict by adding comparison
> for remote communication ID field.
> 
> The absence of such strict check causes to the following flows not being
> handled properly:
> 1. Client tries to setup more than one connection with same QP in
>    a server (e.g., when use external QP), the client would reject
>    the reply.

This is correct and required behavior by IBA:

 12.9.8.3.1 REQ RECEIVED / REP RECEIVED

 (RC, UC, XRC) A CM may receive a REQ/REP specifying a remote QPN in
 “REQ:local QPN”/”REP:local QPN” that the CM already considers
 connected to a local QP.

> 2. Client node reboots, and when it gets the same QP number as that
>    of before the reboot time, the server would rejects the request.

IBA has a specific flow that should be followed for this case:

 When a CM receives such a REQ/REP it shall abort the connection
 establishment by issuing REJ to the REQ/REP. It shall then issue DREQ,
 with “DREQ:remote QPN” set to the remote QPN from the REQ/REP, until
 DREP is received or Max Retries is exceeded, and place the local QP
 in the TimeWait state.

The fundmental issue is IBA does not include the SRC QPN in any RC
packets. It is the responsiblity of each end port to ensure that only
one RC QP is setup sending traffic to a given DLID/DQPN pair.

This is what the timewait mechanism and these checks in the CM are
for. The proper CM use will ensure that the local QP targetting the
DLID/DQPN is destroyed before the cm_id enters timedwait, and the
timedwait will prevent a new QP from being established with the same
parameters until the network has flushed all packets related to the
old sending QP.

Jason

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-16 10:41 [PATCH rdma-next] IB/cm: Consider local communication ID when check for a stale connection Leon Romanovsky
2020-07-31 17:47 ` Jason Gunthorpe

Linux-RDMA Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rdma/0 linux-rdma/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rdma linux-rdma/ https://lore.kernel.org/linux-rdma \
		linux-rdma@vger.kernel.org
	public-inbox-index linux-rdma

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rdma


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git