[PATCH net] rds: ib: use rds_conn_drop() on protocol version mismatch

[PATCH net] rds: ib: use rds_conn_drop() on protocol version mismatch

[henrymei]

From: Aohan Mei <henrymei@tencent.com>

rds_ib_cm_connect_complete() runs from the RDMA-CM event handler with
conn->c_cm_lock held.  When the peer negotiates a protocol version
older than RDS_PROTOCOL_COMPAT_VERSION, the handler calls
rds_conn_destroy(), which is only safe in the rmmod path: it
synchronously tears the connection down and flush_work()es the
shutdown work cp_down_w.

That shutdown work (rds_conn_shutdown()) needs cp_cm_lock, which is
the very lock the event handler still holds, so the flush never
completes: the two workers wait on each other and the RDS connection
workqueues stall for good.

All other RDMA-CM failure paths (REJECTED, CONNECT_ERROR,
DISCONNECTED) use rds_conn_drop(), which marks the connection
RDS_CONN_ERROR and schedules the shutdown work asynchronously.  Use
it here as well.

Fixes: cdc306a5c9cd ("rds: make v3.1 as compat version")
Reported-by: TencentOS Corvus AI <corvus@tencent.com>
Cc: stable@vger.kernel.org
Assisted-by: CodeBuddy:Kimi-K3
Signed-off-by: Aohan Mei <henrymei@tencent.com>
---
 net/rds/ib_cm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
index d46146887ba4..2909da8363f3 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -115,7 +115,7 @@ void rds_ib_cm_connect_complete(struct rds_connection *conn, struct rdma_cm_even
 				  &conn->c_laddr, &conn->c_faddr,
 				  RDS_PROTOCOL_MAJOR(conn->c_version),
 				  RDS_PROTOCOL_MINOR(conn->c_version));
-			rds_conn_destroy(conn);
+			rds_conn_drop(conn);
 			return;
 		}
 	}
-- 
2.43.7

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

[Allison Henderson]

On Tue, 2026-09-08 at 20:33 +0800, henrymei wrote:
> From: Aohan Mei <henrymei@tencent.com>
> 
> rds_ib_cm_connect_complete() runs from the RDMA-CM event handler with
> conn->c_cm_lock held.  When the peer negotiates a protocol version
> older than RDS_PROTOCOL_COMPAT_VERSION, the handler calls
> rds_conn_destroy(), which is only safe in the rmmod path: it
> synchronously tears the connection down and flush_work()es the
> shutdown work cp_down_w.
> 
> That shutdown work (rds_conn_shutdown()) needs cp_cm_lock, which is
> the very lock the event handler still holds, so the flush never
> completes: the two workers wait on each other and the RDS connection
> workqueues stall for good.
> 
> All other RDMA-CM failure paths (REJECTED, CONNECT_ERROR,
> DISCONNECTED) use rds_conn_drop(), which marks the connection
> RDS_CONN_ERROR and schedules the shutdown work asynchronously.  Use
> it here as well.
> 
> Fixes: cdc306a5c9cd ("rds: make v3.1 as compat version")

Hi Aohan,

Thanks for the catch, this looks correct to me.  Just one nit on the
fixes tag.  I think we want: f147dd9ecabf ("RDS/IB: Disallow connections
less than RDS 3.1").  That looks like the commit that added the
rds_conn_destroy() call to the version check.  Other than that, I think
this patch looks good.  With the Fixes tag corrected, you can add my rvb:

Reviewed-by: Allison Henderson <achender@kernel.org>

Thanks!
Allison

> Reported-by: TencentOS Corvus AI <corvus@tencent.com>
> Cc: stable@vger.kernel.org
> Assisted-by: CodeBuddy:Kimi-K3
> Signed-off-by: Aohan Mei <henrymei@tencent.com>
> ---
>  net/rds/ib_cm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
> index d46146887ba4..2909da8363f3 100644
> --- a/net/rds/ib_cm.c
> +++ b/net/rds/ib_cm.c
> @@ -115,7 +115,7 @@ void rds_ib_cm_connect_complete(struct rds_connection *conn, struct rdma_cm_even
>  				  &conn->c_laddr, &conn->c_faddr,
>  				  RDS_PROTOCOL_MAJOR(conn->c_version),
>  				  RDS_PROTOCOL_MINOR(conn->c_version));
> -			rds_conn_destroy(conn);
> +			rds_conn_drop(conn);
>  			return;
>  		}
>  	}

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

[Gerd Rausch]

Hi,

On 2026-09-09 00:09, Allison Henderson via rds-devel wrote:
> On Tue, 2026-09-08 at 20: 33 +0800, henrymei wrote: > From: Aohan Mei <henrymei@ tencent. com> > > rds_ib_cm_connect_complete() runs from the RDMA-CM event handler with > conn->c_cm_lock held. When the peer negotiates a protocol
> On Tue, 2026-09-08 at 20:33 +0800, henrymei wrote:
>> From: Aohan Mei <henrymei@tencent.com>
>> 
>> rds_ib_cm_connect_complete() runs from the RDMA-CM event handler with
>> conn->c_cm_lock held.  When the peer negotiates a protocol version
>> older than RDS_PROTOCOL_COMPAT_VERSION, the handler calls
>> rds_conn_destroy(), which is only safe in the rmmod path: it
>> synchronously tears the connection down and flush_work()es the
>> shutdown work cp_down_w.
>> 
>> That shutdown work (rds_conn_shutdown()) needs cp_cm_lock, which is
>> the very lock the event handler still holds, so the flush never
>> completes: the two workers wait on each other and the RDS connection
>> workqueues stall for good.
>> 
>> All other RDMA-CM failure paths (REJECTED, CONNECT_ERROR,
>> DISCONNECTED) use rds_conn_drop(), which marks the connection
>> RDS_CONN_ERROR and schedules the shutdown work asynchronously.  Use
>> it here as well.
>> 

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.

I understand how the rds_conn_destroy() path is full of bugs.

But shouldn't that just mean those bugs ought to be fixed,
instead of no longer retiring any connection ever,
even to old & incompatible peers we can't really talk to?

Thanks,

   Gerd

>> Fixes: cdc306a5c9cd ("rds: make v3.1 as compat version")
> 
> Hi Aohan,
> 
> Thanks for the catch, this looks correct to me.  Just one nit on the
> fixes tag.  I think we want: f147dd9ecabf ("RDS/IB: Disallow connections
> less than RDS 3.1").  That looks like the commit that added the
> rds_conn_destroy() call to the version check.  Other than that, I think
> this patch looks good.  With the Fixes tag corrected, you can add my rvb:
> 
> Reviewed-by: Allison Henderson <achender@kernel.org>
> 
> Thanks!
> Allison
> 
>> Reported-by: TencentOS Corvus AI <corvus@tencent.com>
>> Cc: stable@vger.kernel.org
>> Assisted-by: CodeBuddy:Kimi-K3
>> Signed-off-by: Aohan Mei <henrymei@tencent.com>
>> ---
>>  net/rds/ib_cm.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
>> index d46146887ba4..2909da8363f3 100644
>> --- a/net/rds/ib_cm.c
>> +++ b/net/rds/ib_cm.c
>> @@ -115,7 +115,7 @@ void rds_ib_cm_connect_complete(struct rds_connection *conn, struct rdma_cm_even
>>  				  &conn->c_laddr, &conn->c_faddr,
>>  				  RDS_PROTOCOL_MAJOR(conn->c_version),
>>  				  RDS_PROTOCOL_MINOR(conn->c_version));
>> -			rds_conn_destroy(conn);
>> +			rds_conn_drop(conn);
>>  			return;
>>  		}
>>  	}
> 
> 
> _______________________________________________
> rds-devel mailing list
> rds-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/rds-devel
> 

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

[Allison Henderson]

On Wed, 2026-09-09 at 10:25 -0700, Gerd Rausch wrote:
> Hi,
> 
> On 2026-09-09 00:09, Allison Henderson via rds-devel wrote:
> > On Tue, 2026-09-08 at 20: 33 +0800, henrymei wrote: > From: Aohan Mei <henrymei@ tencent. com> > > rds_ib_cm_connect_complete() runs from the RDMA-CM event handler with > conn->c_cm_lock held. When the peer negotiates a protocol
> > On Tue, 2026-09-08 at 20:33 +0800, henrymei wrote:
> > > From: Aohan Mei <henrymei@tencent.com>
> > > 
> > > rds_ib_cm_connect_complete() runs from the RDMA-CM event handler with
> > > conn->c_cm_lock held.  When the peer negotiates a protocol version
> > > older than RDS_PROTOCOL_COMPAT_VERSION, the handler calls
> > > rds_conn_destroy(), which is only safe in the rmmod path: it
> > > synchronously tears the connection down and flush_work()es the
> > > shutdown work cp_down_w.
> > > 
> > > That shutdown work (rds_conn_shutdown()) needs cp_cm_lock, which is
> > > the very lock the event handler still holds, so the flush never
> > > completes: the two workers wait on each other and the RDS connection
> > > workqueues stall for good.
> > > 
> > > All other RDMA-CM failure paths (REJECTED, CONNECT_ERROR,
> > > DISCONNECTED) use rds_conn_drop(), which marks the connection
> > > RDS_CONN_ERROR and schedules the shutdown work asynchronously.  Use
> > > it here as well.
> > > 
> 
> 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.

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

> instead of no longer retiring any connection ever,
> even to old & incompatible peers we can't really talk to?
> 
> Thanks,
> 
>    Gerd
> 

I do think there's potential for a good follow-up in what you're
describing through. A separate patch could fix both directions at
once: a per-conn flag set from the REJ_INCOMPAT case and from this one
that makes rds_conn_shutdown() skip the timer-driven rds_queue_reconnect().
Then the conn would then sit in RDS_CONN_DOWN until a socket actually sends
to it.  At which point rds_conn_connect_if_down() gives the peer another
chance in case it was upgraded.  That also gets rid of the once-a-second
log line, which we have today on the REJECTED side too.  That seems like
net-next material to me rather than something to hold this fix on, but
I'd be happy to review it.

I also think it's worth pointing out which peers this actually affects.
Upstream has RDS_PROTOCOL_VERSION and RDS_PROTOCOL_COMPAT_VERSION both
at 3.1.  So this branch only fires for a 3.0 peer (OFED 1.3 era) or for
an accept that carries no or truncated private data.  That would leave
c_version at 0 on a fresh conn.  In practice that's a broken or non-RDS
peer on the RDS port, not a cluster of un-upgraded nodes. So retrying
those on a backoff like any other unreachable peer seems appropriate.

Thanks,
Allison    

> > > Fixes: cdc306a5c9cd ("rds: make v3.1 as compat version")
> > 
> > Hi Aohan,
> > 
> > Thanks for the catch, this looks correct to me.  Just one nit on the
> > fixes tag.  I think we want: f147dd9ecabf ("RDS/IB: Disallow connections
> > less than RDS 3.1").  That looks like the commit that added the
> > rds_conn_destroy() call to the version check.  Other than that, I think
> > this patch looks good.  With the Fixes tag corrected, you can add my rvb:
> > 
> > Reviewed-by: Allison Henderson <achender@kernel.org>
> > 
> > Thanks!
> > Allison
> > 
> > > Reported-by: TencentOS Corvus AI <corvus@tencent.com>
> > > Cc: stable@vger.kernel.org
> > > Assisted-by: CodeBuddy:Kimi-K3
> > > Signed-off-by: Aohan Mei <henrymei@tencent.com>
> > > ---
> > >  net/rds/ib_cm.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
> > > index d46146887ba4..2909da8363f3 100644
> > > --- a/net/rds/ib_cm.c
> > > +++ b/net/rds/ib_cm.c
> > > @@ -115,7 +115,7 @@ void rds_ib_cm_connect_complete(struct rds_connection *conn, struct rdma_cm_even
> > >  				  &conn->c_laddr, &conn->c_faddr,
> > >  				  RDS_PROTOCOL_MAJOR(conn->c_version),
> > >  				  RDS_PROTOCOL_MINOR(conn->c_version));
> > > -			rds_conn_destroy(conn);
> > > +			rds_conn_drop(conn);
> > >  			return;
> > >  		}
> > >  	}
> > 
> > 
> > _______________________________________________
> > rds-devel mailing list
> > rds-devel@oss.oracle.com
> > https://oss.oracle.com/mailman/listinfo/rds-devel
> > 

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

[Gerd Rausch]

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.

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

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.

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

Thanks,

   Gerd

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

[Allison Henderson]

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
> 

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

[Gerd Rausch]

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

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

[Allison Henderson]

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
>