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

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