Re: [PATCH net-next v3] net/rds: restrict the rdma_cm ids to IB devices

[Allison Henderson <achender@kernel.org>]

On Sun, 2026-09-27 at 12:11 +0100, Simon Horman wrote:
> On Fri, Sep 25, 2026 at 11:49:05AM +0000, netdev-bot+sashiko@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential
> > issue(s) to consider.
> > 
> > Critical: 0 · High: 0 · Medium: 1 · Low: 0 (1)
> > 
> > If you believe none of the issues are legitimate, please reply to each
> > issue of Medium severity or higher and explain why the AI is wrong.
> > If any of them are legitimate and you plan to respin, please let patchwork
> > know by including "pw-bot: cr" as a separate line at the end of your reply
> > (one such reply per series is enough).
> > 
> > - [Medium] Commit-message misclassification: the changelog declares this
> >   change "hardening on top of it, not a fix in its own right, hence no…
> > 
> > Pre-existing issues:
> > - [Low] rds_rdma_listen_init_common() (net/rds/rdma_transport.c) logs the
> >   listening port as the compile-time constant RDS_PORT (18634) at line…
> > 
> > --
> > 
> > net/rds: restrict the rdma_cm ids to IB devices
> > 
> > This restricts the three rdma_cm ids RDS creates - the listener, the
> > per-connection id in rds_ib_conn_path_connect() and the probe id in
> > rds_ib_laddr_check_cm() - to RDMA_NODE_IB_CA with
> > rdma_restrict_node_type() before they are bound.  The explicit node_type
> > test in rds_ib_laddr_check_cm() is dropped as redundant.
> > 
> > > From the changelog:
> > 
> > > With that, no rdma_cm event reaches RDS's handler from a device it
> > > has no transport for.  The handler itself still assumes IB: it assigns
> > > its transport pointer only for RDMA_NODE_IB_CA and dereferences it
> > > regardless, which is the crash that first surfaced this.  The fix for
> > > that is a separate net patch, Aohan Mei's "net: rds: fix uninitialized
> > > trans dereference in CM event handler", and is what stable kernels
> > > without rdma_restrict_node_type() - which arrived in commit
> > > a760e80e90f5 ("RDMA/core: introduce rdma_restrict_node_type()") - have
> > > to take; this patch is hardening on top of it, not a fix in its own
> > > right, hence no Fixes tag.
> > 
> > [Severity: Medium]
> > Is the "not a fix in its own right, hence no Fixes tag" classification
> > accurate for the tree this applies to?
> > 
> > The referenced companion patch ("net: rds: fix uninitialized trans
> > dereference in CM event handler") is not present here.  In
> > rds_rdma_cm_event_handler_cmn() the transport pointer is still
> > uninitialized on entry:
> 
> I find this to be a specious argument.
> Of course a kernel without a fix applied is not fixed.
> 
> I believe the appropriate question, regarding fixing the Kernel, is if the
> cited fixed actually fixes the problem.  That question is not addressed
> here.  And indeed it's not a question to be answered here as it's a question
> to be answered in the context of reviewing the cited fix.

Agreed, I had sent out a v4 last night just before this response. The code change
is the same, but I did add the Fixes tag for the listeners the review talked about.
As well a comment stating the patches are independent.

The companion patch is Aohan's, and his v2 thread has been quiet since late
August, so I'll carry it forward as a v3 to net with the review comments
on v2 addressed.

> 
> > 
> > net/rds/rdma_transport.c:rds_rdma_cm_event_handler_cmn() {
> > 	struct rds_connection *conn = cm_id->context;
> > 	struct rds_transport *trans;
> > 	...
> > 	if (cm_id->device->node_type == RDMA_NODE_IB_CA)
> > 		trans = &rds_ib_transport;
> > 	...
> > 	case RDMA_CM_EVENT_CONNECT_REQUEST:
> > 		ret = trans->cm_handle_connect(cm_id, event, isv6);
> > 	...
> > 	case RDMA_CM_EVENT_ESTABLISHED:
> > 		if (conn)
> > 			trans->cm_connect_complete(conn, event);
> > }
> > 
> > So for any event arriving from a device whose node_type is not
> > RDMA_NODE_IB_CA, trans is dereferenced as stack garbage, and the
> > CONNECT_REQUEST case is reachable from an inbound connection on the RDS
> > port.
> > 
> > Before this patch cma_listen_on_dev() installed the RDS listener on every
> > RDMA device, and cma_acquire_dev_by_src_ip() could pick a software iWARP
> > device for the same netdev, which is what made that path reachable.  With
> > the listener restriction added below, cma_listen_on_dev() honours
> > restricted_node_type and no longer installs the listener on such a device.
> > 
> > That makes this patch the change that removes the remotely reachable route
> > to that indirect call in this tree.  Should it then carry a Fixes tag and
> > state the ordering dependency on the companion handler patch, rather than
> > being described as hardening only?
> > 
> > > diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
> > > index b15cf316b23a2..91ff1dde26af0 100644
> > > --- a/net/rds/rdma_transport.c
> > > +++ b/net/rds/rdma_transport.c
> > > @@ -210,6 +210,14 @@ static int rds_rdma_listen_init_common(rdma_cm_event_handler handler,
> > >  		return ret;
> > >  	}
> > >  
> > > +	/* Only the IB transport is left, so only listen on IB devices */
> > > +	ret = rdma_restrict_node_type(cm_id, RDMA_NODE_IB_CA);
> > > +	if (ret) {
> > > +		pr_err("RDS/RDMA: failed to setup listener, rdma_restrict_node_type() returned %d\n",
> > > +		       ret);
> > > +		goto out;
> > > +	}
> > > +
> > >  	/*
> > >  	 * XXX I bet this binds the cm_id to a device.  If we want to support
> > >  	 * fail-over we'll have to take this into consideration.
> > 
> > [Severity: Low]
> > This isn't a bug introduced by this patch, but since the patch touches
> > rds_rdma_listen_init_common() anyway: the debug line further down in the
> > same function hard-codes the port,
> > 
> > 	rdsdebug("cm %p listening on port %u\n", cm_id, RDS_PORT);
> > 
> > while the helper has two callers with different ports.
> > rds_rdma_listen_init() binds the IPv4 listener with
> > sin.sin_port = htons(RDS_PORT) and the IPv6 listener with
> > sin6.sin6_port = htons(RDS_CM_PORT), as the comment above it describes.
> > 
> > For the IPv6 listener the message therefore prints 18634 rather than the
> > 16385 that was bound.  Would it make sense to print the port from the sa
> > argument that was just bound instead?
> 
> If this is a problem it is separate to this patch,
> and should be addressed separately.

Yes, v4 has that as a separate second patch to address this part

> 
> In all, this patch looks good to me.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
Thank you!  I'll carry your rvb on patch 1 of v4 unless you'd
rather look at it again.

Allison