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 10:08 -0700, Gerd Rausch wrote:
> Hi Allison,
> 
> On 2026-09-09 15:55, Allison Henderson wrote:
> > On Wed, 2026-09-09 at 10:25 -0700, Gerd Rausch wrote:
> > > On 2026-09-09 00:09, Allison Henderson via rds-devel wrote:
> > > I don't get this.
> > > 
> > > So RDS should just keep on retrying to connect every second instead
> > > of giving up on that incompatible peer? Forever?
> > > 
> > > The version number the peer uses remains the same until it gets upgraded somehow.
> > 
> > Hi Gerd,
> > 
> > You're right that this ends up as a ~1s retry loop.  While that may
> > not be pretty, I think it's still the right call for a net/stable
> > fix, for a few reasons.
> > 
> > First, it's already what we do for the same condition in the other
> > direction.  When the passive side rejects us with RDS_RDMA_REJ_INCOMPAT,
> > the REJECTED case in rds_rdma_cm_event_handler_cmn() calls
> > rds_conn_drop(), we reconnect with the usual backoff (capped at
> > rds_sysctl_reconnect_max_jiffies, one second), and we log a pr_warn
> > on every attempt.  That's the common direction, a new node talking to
> > an old one.  Aohan's patch just makes the rare direction behave the
> > same way instead of wedging.
> > 
> 
> The important difference is that when this happens, and the client
> sees a RDS_RDMA_REJ_INCOMPAT from the peer, it'll downgrade to
> RDS_PROTOCOL_COMPAT_VERSION (3.1):
> An older version expected to be understood by all RDS modules.
> 
> And thus the connection will be retried with version 3.1,
> and is expected to be accepted by the peer:
> 
> Function rds_ib_cm_connect_complete() explicitly exempts
> RDS_PROTOCOL_COMPAT_VERSION from the rds_conn_destroy() treatment,
> but allows the connection to proceed.
> 
> So it is not the same way at all.
> 
> 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.

Hi Gerd,

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

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.

> 
> > Second, rds_conn_destroy() doesn't actually give up on the peer either.
> > Nothing in RDS remembers that a peer is incompatible.  Destroy frees
> > the conn, and the next rds_sendmsg() to that address either reuses the
> > socket's cached rs->rs_conn or goesthrough rds_conn_create_outgoing()
> > and rds_conn_connect_if_down() and then connects again.  With an
> > application actively sending, that's a create/connect/mismatch/destroy
> > cycle at the application level.  With an idle application the only
> > difference from the drop is whether the retry is timer-driven or
> > send-driven.  Deferring the destroy to a worker doesn't change that
> > picture, it just avoids the deadlock.
> > 
> 
> I can't follow.
> Are you suggesting to ban an IP-address / peer for life?
> 
> A node can always retry an RDS connect, triggered by a sendmsg().
> Much like a failed TCP connect() doesn't ban a peer for life.

No, quite the opposite, apologies if that was unclear.  The point I
was trying to make is that rds_conn_destroy() doesn't "give up" on the
peer either. It frees the conn, and then next sendmsg() recreates it and
connects again.  So the choice here was never "give up" versus "retry". 
It is "retry on the next send" versus "retry on a timer", and the former
also comes with a dead lock.

> 
> There's a big difference between an application trying to send data again,
> and the RDS module itself spinning around every second and re-trying
> a connect with a version# that is known to be incompatible,
> and thus never succeeding, because the fallback mechanism became broken.
> 

Agreed, but that would be the net-next follow-up I suggested: mark the
conn when the peer turns out to be incompatible, have rds_conn_shutdown()
skip the timer-driven rds_queue_reconnect() for it, and let
rds_conn_connect_if_down() from the next sendmsg() be the retry.
That gives you the TCP-like behaviour you describe. I just don't think it
should gate a one-line stable fix for a dead lock.

> > 
> > > 
> > > I understand how the rds_conn_destroy() path is full of bugs.
> > > 
> > > But shouldn't that just mean those bugs ought to be fixed,
> > 
> > While that code path does have bugs, making rds_conn_destroy()
> > safe to call from the CM path is a much bigger change, and one that
> > should probably go through net-next rather than holding up a small
> > fix like this.  The current code hangs every RDS connection on the
> > node the first time this fires, and a one second retry on one conn is
> > strictly better than that.
> > 
> 
> It is not only the infinite retry every second that's of concern:
> 
> This change also implies that all RDS kernel modules speak
> the exact same protocol version.
> I don't see how that assumption can ever be valid.
> 
> Because after this change, if versions differ, there's no fallback to 3.1,
> but connections won't ever get established.

The version negotiation looks unchanged to me.  The passive side still
picks the highest common minor from dp_protocol_minor_mask, or 3.1 for
a 3.1 peer, or rejects with RDS_RDMA_REJ_INCOMPAT.  And the active side
still lowers c_proposed_version to RDS_PROTOCOL_COMPAT_VERSION on that
reject.  The changed line only runs when negotiation has already
produced a result below 3.1, which no fallback can rescue.

FWIW, upstream currently defines both RDS_PROTOCOL_VERSION and
RDS_PROTOCOL_COMPAT_VERSION as 3.1.  And the active side proposes
c_proposed_version, which starts at RDS_PROTOCOL_VERSION.  So, that
means upstream always proposes 3.1.  The downgrade in the REJECTED case
is basically a no-op there.  And an INCOMPAT reject already results in
the once-a-second retry you're describing.  That's what I meant by the
other direction behaving the same way, and it's another reason the "retry
only on send" change is better done once, for both paths, as a follow-up.

I hope that helps?  Let me know if anything is unclear or if you think
I've missed something.

Thank you,
Allison


> 
> Thanks,
> 
>    Gerd
>