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

[Allison Henderson <achender@kernel.org>]

On Thu, 2026-09-10 at 16:56 -0700, Gerd Rausch wrote:
> 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.

Yes, err starts as 1 and the conn-create failure and connect-race exits
go to the reject without changing it.  So you are correct that the peer can't tell
"incompatible" from "you raced me".  That's a wart, and worth a patch of its
own to give those exits their own code.  But I still think it's separate from this
fix, since it doesn't touch the reject side in either direction.

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

Also right, and I should have specified "on the active side".  On the
passive side the version was already set from the REQ in
rds_ib_cm_handle_connect(), and it is always 3.1 or newer there
because anything else was rejected before the accept.  So the RTU
arriving with no payload leaves a valid c_version in place and the
branch can't fire.  That also means the changed line only runs
on the active side, and only when the REP says 3.0 or says nothing.

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

That would make more sense in principle. That's what rds_ib_protocol_compatible()
already does before the accept on the passive side.  On the active
side there isn't an earlier hook. AFAICT, rdma_cm moves the QP to RTS and sends
the RTU inside cma_rep_recv() before it delivers RDMA_CM_EVENT_ESTABLISHED.
So the first time the active side sees the version the peer picked, the connection
is already up.  That's why the check ended up in rds_ib_cm_connect_complete().
Its not pretty, but I think that's really more about following CM API.  

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

Well, there is a downgrade in the REJECTED path, and the downgrading side
probably should be the one to retry.  But in the branch this
patch changes, nobody rejected.  The peer accepted, with a version below
the floor, so there's no downgrade available to either side.

So the loop is there either way.  The conn is torn down (via destroy or
by drop), the peer gets a DISCONNECTED, drops, and reconnects to us on
its own timer (IB has no smaller-address rule in rds_queue_reconnect()),
and we reject it in rds_ib_cm_handle_connect() every time.

Destroy doesn't make the peer back off; it just moves the 1 second loop
to their side of the wire.  The next sendmsg() on our side starts it
again from ours. The thing that actually stops the loop is "retry only
when an application sends", which is the follow-up I'd like to see, and
would need to cover the REJECTED path too.


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

Agreed, I don't think anyone would dispute that RDS has plenty
of things to fix.  But to me that's the reason to shepherd small,
targeted fixes through one at a time rather than gate them on
each other, or on a rework of the negotiation.  This one turns a
node-wide hang into the same retry loop the peer already drives today,
with a one-line change that's easy to revert when we get to larger
changes.

So my suggestion would be: take Aohan's fix as is for net, and treat
the reject-code cleanup, the send-driven retry, and any negotiation
rework as separate net-next patches.  I'd be glad to review any of
them, and happy to help with the send-driven one if you'd like to
take it.

Thanks,
Allison



> Thanks,
> 
>    Gerd
>