Re: [rds-devel] [External] : Re: [PATCH net] rds: ib: use rds_conn_drop() on protocol version mismatch

[Gerd Rausch <gerd.rausch@oracle.com>]

Hi Allison,

On 2026-09-10 15:58, Allison Henderson wrote:
> On Thu, 2026-09-10 at 10:08 -0700, Gerd Rausch wrote:
>> Before this patch, a version 3.1 connection is expected to be established.
>> After this patch, there's an infinite connection retry attempt every second,
>> that'll never succeed.
> 
> I think we might be talking about different branches of code?  It
> looks like the downgrade you're describing is in the REJECTED case of
> rds_rdma_cm_event_handler_cmn().  But this patch doesn't touch that.

Of course, it doesn't.

But rds_ib_cm_handle_connect() issues a rdma_reject(IB_CM_REJ_CONSUMER_DEFINED) with payload
err = 1 (aka RDS_RDMA_REJ_INCOMPAT) not only if rds_ib_protocol_compatible()
returns zero, but all sorts of other scenarios also.

And when that rdma_reject(IB_CM_REJ_CONSUMER_DEFINED, 1) is issued,
then received by the peer, it lands in rds_rdma_cm_event_handler_cmn(), where this:
--------%<--------%<--------%<--------%<--------%<--------
     conn->c_proposed_version = RDS_PROTOCOL_COMPAT_VERSION;
--------%<--------%<--------%<--------%<--------%<--------
version downgrade is expected to happen.

Subsequently it should be the node that did the downgrade to COMPAT_VERSION
that tries again.

Not the peer. The peer should back off, as it didn't go to COMPAT_VERSION.

Looking at rds_ib_protocol_compatible(), it seems terribly broken,
and AFAICT, none of this stuff works intuitively or even correctly.

But one thing is very clear to me:
We should never let the side that has an incompatible / wrong
"conn->c_version" value continue to attempt to connect.

That side never downgraded the "c_proposed_version" and will
just keep on repeating what didn't work before.

And yet, that is exactly what this patch proposed:
Don't destroy the connection, but keep on trying with
the wrong "conn->c_version" / "conn->c_proposed_version".

> The 3.1 exemption in rds_ib_cm_connect_complete() is a few lines above
> the one being changed, and it isn't touched either.  So a 3.1 connection
> should proceed exactly as before.
>

I don't understand.

Why would the connection proceed exactly as before,
when "before" only one side retried with a downgraded 3.1 version,
but "after" both sides retry with a mix of 3.1 and and whatever incompatible
version the other node had?
It'll be a game of "who's fastest".

> The changes here are only reached in the RDMA_CM_EVENT_ESTABLISHED case,
> after the peer has already accepted, and only when the version it accepted
> with is below 3.1. IOW: that is specifically a peer that answered our
> 3.1 proposal with 3.0.  Or an accept that carried no (or truncated)
> private data, which leaves c_version at 0 on a fresh conn.
> 

What you described as "carried no (or truncated) private data"
is the normal case for IB_CM_RTU_RECEIVED:

The RDS/client issues an rdma_connect() (REQ) with user specified payload.
That arrives at RDS/server inside rds_ib_cm_handle_connect(),
which inspects protocol-version etc.

Then the RDS/server issues an rdma_accept() (REP) again with user specified payload.
That arrives at the RDS/client as RDMA_CM_EVENT_ESTABLISHED *with* payload
and inside RDS' function rds_ib_cm_connect_complete().

And then the RDS/client (actually the CM layer) sends out an RTU, that makes the RDS/server
land inside rds_ib_cm_connect_complete() too, but with "private_data_len==0".

So a "major == 0" doesn't imply something being truncated nor it being
a fresh conn, but simply is the last step you see on an RDS/server
during connection establishment.

It is rather curious that RDS performs a compatibility check inside
rds_ib_cm_connect_complete(), i.e. after a RDMA_CM_EVENT_ESTABLISHED
was received.

IMHO the compatibility ought to be checked prior to issuing an "rdma_accept()",
proposing common denominator version(s) that are appropriate for both sides.

Not after the connection had already been established.

The question if an incompatible connection ought to be destroyed or not
wouldn't come up, if we didn't allow incompatible connections to be created
in the first place.

> There is no fallback for that case before or after the patch. 3.1 is the
> compat floor, so there's nothing lower to fall back to.  Before
> the patch, that branch calls rds_conn_destroy() under c_cm_lock and
> deadlocks against rds_conn_shutdown().  Which then stalls every RDS
> connection on the node.  After the patch it drops the conn and
> retries with backoff.  Neither case establishes a connection, and neither
> was ever going to.
> 

The important bit that's missing here is that the side that did the
"c_proposed_version = RDS_PROTOCOL_COMPAT_VERSION" downgrade
is the one that needs to initiate.

Or else, we run the risk of running into endless loops of trying the same
thing again and again and again.

But this whole protocol negotation in RDS is rather broken.
I guess some would call it "organically grown over the years".

Thanks,

   Gerd