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

[Simon Horman <horms@kernel.org>]

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.

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

In all, this patch looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>