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

[netdev-bot+sashiko@kernel.org]

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:

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?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922084837.390414-1-achender%40kernel.org