On 3/5/2019 11:04 PM, Gerd Rausch wrote:
Prior to
commit d021fabf525ff ("rds: rdma: add consumer reject")
function "rds_rdma_cm_event_handler_cmn" would always honor a rejected
connection attempt by issuing a "rds_conn_drop".
The commit mentioned above added a "break", eliminating
the "fallthrough" case and made the "rds_conn_drop" rather conditional:
Now it only happens if a "consumer defined" reject (i.e. "rdma_reject")
carries an integer-value of "1" inside "private_data":
if (!conn)
+ break;
+ err = (int *)rdma_consumer_reject_data(cm_id, event, &len);
+ if (!err || (err && ((*err) == RDS_RDMA_REJ_INCOMPAT))) {
+ pr_warn("RDS/RDMA: conn <%pI6c, %pI6c> rejected, dropping
connection\n",
+ &conn->c_laddr, &conn->c_faddr);
+ conn->c_proposed_version = RDS_PROTOCOL_COMPAT_VERSION;
+ rds_conn_drop(conn);
+ }
rdsdebug("Connection rejected: %s\n",
rdma_reject_msg(cm_id, event->status));
+ break;
/* FALLTHROUGH */
A number of issues are worth mentioning here:
#1) Previous versions of the RDS code simply rejected a connection
by calling "rdma_reject(cm_id, NULL, 0);"
So the value of the payload in "private_data" will not be "1",
but "0".
#2) Now the code has become dependent on host byte order and sizing.
If one peer is big-endian, the other is little-endian,
or there's a difference in sizeof(int) (e.g. ILP64 vs LP64),
the *err check does not work as intended.
#3) There is no check for "len" to see if the data behind *err is even valid.
Luckily, it appears that the "rdma_reject(cm_id, NULL, 0)" will always
carry 148 bytes of zeroized payload.
But that should probably not be relied upon here.
#4) With the added "break;",
we might as well drop the misleading "/* FALLTHROUGH */" comment.
This commit does _not_ address issue #2, as the sender would have to
agree on a byte order as well.
Here is the sequence of messages in this observed error-scenario:
Host-A is pre-QoS changes (excluding the commit mentioned above)
Host-B is post-QoS changes (including the commit mentioned above)
#1 Host-B
issues a connection request via function "rds_conn_path_transition"
connection state transitions to "RDS_CONN_CONNECTING"
#2 Host-A
rejects the incompatible connection request (from #1)
It does so by calling "rdma_reject(cm_id, NULL, 0);"
#3 Host-B
receives an "RDMA_CM_EVENT_REJECTED" event (from #2)
But since the code is changed in the way described above,
it won't drop the connection here, simply because "*err == 0".
#4 Host-A
issues a connection request
#5 Host-B
receives an "RDMA_CM_EVENT_CONNECT_REQUEST" event
and ends up calling "rds_ib_cm_handle_connect".
But since the state is already in "RDS_CONN_CONNECTING"
(as of #1) it will end up issuing a "rdma_reject" without
dropping the connection:
if (rds_conn_state(conn) == RDS_CONN_CONNECTING) {
/* Wait and see - our connect may still be succeeding */
rds_ib_stats_inc(s_ib_connect_raced);
}
goto out;
#6 Host-A
receives an "RDMA_CM_EVENT_REJECTED" event (from #5),
drops the connection and tries again (goto #4) until it gives up.
Orabug: 29444532
Signed-off-by: Gerd Rausch <gerd.rau...@oracle.com>
---
net/rds/rdma_transport.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
index 46bce8389066..f628e7fda66d 100644
--- a/net/rds/rdma_transport.c
+++ b/net/rds/rdma_transport.c
@@ -112,7 +112,7 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id
*cm_id,
if (!conn)
break;
err = (int *)rdma_consumer_reject_data(cm_id, event, &len);
- if (!err || (err && ((*err) == RDS_RDMA_REJ_INCOMPAT))) {
+ if (!err || (err && len >= sizeof(*err) && ((*err) <=
RDS_RDMA_REJ_INCOMPAT))) {
pr_warn("RDS/RDMA: conn <%pI6c, %pI6c> rejected, dropping
connection\n",
&conn->c_laddr, &conn->c_faddr);
conn->c_proposed_version = RDS_PROTOCOL_COMPAT_VERSION;
@@ -122,7 +122,6 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id
*cm_id,
rdsdebug("Connection rejected: %s\n",
rdma_reject_msg(cm_id, event->status));
break;
- /* FALLTHROUGH */
case RDMA_CM_EVENT_ADDR_ERROR:
case RDMA_CM_EVENT_ROUTE_ERROR:
case RDMA_CM_EVENT_CONNECT_ERROR:
Very similar test diff [1] I sent Yanjun to test yesterday... Thanks for
checking. Will submit a cleaned up fix Gerd. Thansk for checking
From 82c638f11137e29220051c704b63e3d7ed7d44d0 Mon Sep 17 00:00:00 2001
From: Santosh Shilimkar <santosh.shilim...@oracle.com>
Date: Tue, 5 Mar 2019 18:19:52 -0800
Subject: [PATCH] TEST debug patch
Signed-off-by: Santosh Shilimkar <santosh.shilim...@oracle.com>
---
net/rds/rdma_transport.c | 16 ++++++++++++++++
1 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
index 46bce83..c7b1fff 100644
--- a/net/rds/rdma_transport.c
+++ b/net/rds/rdma_transport.c
@@ -109,8 +109,10 @@ static int rds_rdma_cm_event_handler_cmn(struct
rdma_cm_id *cm_id,
break;
case RDMA_CM_EVENT_REJECTED:
+ err = (int *)event->param.conn.private_data;
if (!conn)
break;
+#if 0
err = (int *)rdma_consumer_reject_data(cm_id, event, &len);
if (!err || (err && ((*err) == RDS_RDMA_REJ_INCOMPAT))) {
pr_warn("RDS/RDMA: conn <%pI6c, %pI6c> rejected, dropping
connection\n",
@@ -119,6 +121,20 @@ static int rds_rdma_cm_event_handler_cmn(struct
rdma_cm_id *cm_id,
conn->c_tos = 0;
rds_conn_drop(conn);
}
+#endif
+ if (event->status == RDS_REJ_CONSUMER_DEFINED &&
+ (*err) == RDS_RDMA_REJ_INCOMPAT) {
+
+ pr_warn("RDS/RDMA: conn <%pI6c, %pI6c> rejected, dropping
connection\n",
+ &conn->c_laddr, &conn->c_faddr);
+ conn->c_proposed_version = RDS_PROTOCOL_COMPAT_VERSION;
+ conn->c_tos = 0;
+ rds_conn_drop(conn);
+ } else {
+ pr_debug("RDS/RDMA: conn <%pI6c, %pI6c> rejected, dropping
connection\n",
+ &conn->c_laddr, &conn->c_faddr);
+ rds_conn_drop(conn);
+ }
rdsdebug("Connection rejected: %s\n",
rdma_reject_msg(cm_id, event->status));
break;
--
1.7.1
