[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 at 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 at kernel.org>
> > 
> > Thanks!
> > Allison
> > 
> > > Reported-by: TencentOS Corvus AI <corvus at tencent.com>
> > > Cc: stable at vger.kernel.org
> > > Assisted-by: CodeBuddy:Kimi-K3
> > > Signed-off-by: Aohan Mei <henrymei at 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 at oss.oracle.com
> > https://oss.oracle.com/mailman/listinfo/rds-devel
> >