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

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

[Allison Henderson]

RDS only has an IB transport, but it never tells the rdma_cm so.  The
listener created in rds_rdma_listen_init() is therefore installed on
every RDMA device in the system, including iWARP RNICs, and the id an
outgoing connection resolves through in rds_ib_conn_path_connect()
may be bound to whichever device the address resolution picks:
rds_ib_laddr_check() only vouched for the local address being served
by one of RDS's IB devices, while cma_acquire_dev_by_src_ip() walks
every RDMA device for the same address, so a software iWARP device
attached to the same netdev can win.  The event handler then runs
the IB transport's callbacks against a device that is not one.

The rdma_cm has an API for exactly this since commit a760e80e90f5
("RDMA/core: introduce rdma_restrict_node_type()").  Restrict all
three ids RDS creates - the listener, the per-connection id and the
probe id in rds_ib_laddr_check_cm() - to RDMA_NODE_IB_CA before they
are bound, so that the listener is only installed on IB devices, an
outgoing connection can only bind one, and the address check's bind
fails outright on anything else (its explicit node_type test now only
guards against a device with no node type at all).

With that, the connect-request rejection for non-IB devices in the
event handler becomes unreachable; it stays as the last line of
defence and is what stable kernels without the new API rely on.

Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
v2: destroy the freshly created cm_id, and clear ic->i_cm_id, when
    rdma_restrict_node_type() fails in rds_ib_conn_path_connect(), as
    the rdma_resolve_addr() failure path below it does (review of v1).
v1: https://lore.kernel.org/netdev/20260917074108.174262-1-achender@kernel.org/
 net/rds/ib.c             | 11 +++++------
 net/rds/ib_cm.c          | 12 ++++++++++++
 net/rds/rdma_transport.c |  8 ++++++++
 3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/net/rds/ib.c b/net/rds/ib.c
index 786f39169bc1..6d367518aafb 100644
--- a/net/rds/ib.c
+++ b/net/rds/ib.c
@@ -414,13 +414,14 @@ static int rds_ib_laddr_check_cm(struct net *net, const struct in6_addr *addr,
 	bool isv4;
 
 	isv4 = ipv6_addr_v4mapped(addr);
-	/* Create a CMA ID and try to bind it. This catches both
-	 * IB and iWARP capable NICs.
-	 */
+	/* Create a CMA ID restricted to IB devices and try to bind it. */
 	cm_id = rdma_create_id(&init_net, rds_rdma_cm_event_handler,
 			       NULL, RDMA_PS_TCP, IB_QPT_RC);
 	if (IS_ERR(cm_id))
 		return PTR_ERR(cm_id);
+	ret = rdma_restrict_node_type(cm_id, RDMA_NODE_IB_CA);
+	if (ret)
+		goto out;
 
 	if (isv4) {
 		memset(&sin, 0, sizeof(sin));
@@ -473,10 +474,8 @@ static int rds_ib_laddr_check_cm(struct net *net, const struct in6_addr *addr,
 #endif
 	}
 
-	/* rdma_bind_addr will only succeed for IB & iWARP devices */
+	/* the restriction above means this only succeeds for IB devices */
 	ret = rdma_bind_addr(cm_id, sa);
-	/* due to this, we will claim to support iWARP devices unless we
-	   check node_type. */
 	if (ret || !cm_id->device ||
 	    cm_id->device->node_type != RDMA_NODE_IB_CA)
 		ret = -EADDRNOTAVAIL;
diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
index 4feb0edc360c..d64dad317ebe 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -999,6 +999,18 @@ int rds_ib_conn_path_connect(struct rds_conn_path *cp)
 		goto out;
 	}
 
+	/* rds_ib_laddr_check() only vouched for the local address being
+	 * on an IB device; the address resolution below picks the device
+	 * on its own, so restrict it to the same kind.
+	 */
+	ret = rdma_restrict_node_type(ic->i_cm_id, RDMA_NODE_IB_CA);
+	if (ret) {
+		rdsdebug("rdma_restrict_node_type() failed: %d\n", ret);
+		rdma_destroy_id(ic->i_cm_id);
+		ic->i_cm_id = NULL;
+		goto out;
+	}
+
 	rdsdebug("created cm id %p for conn %p\n", ic->i_cm_id, conn);
 
 	if (ipv6_addr_v4mapped(&conn->c_faddr)) {
diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
index b15cf316b23a..91ff1dde26af 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.
-- 
2.25.1

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

[sashiko-bot]

> RDS only has an IB transport, but it never tells the rdma_cm so.  The
> listener created in rds_rdma_listen_init() is therefore installed on
> every RDMA device in the system, including iWARP RNICs, and the id an
> outgoing connection resolves through in rds_ib_conn_path_connect()
> may be bound to whichever device the address resolution picks:
> rds_ib_laddr_check() only vouched for the local address being served
> by one of RDS's IB devices, while cma_acquire_dev_by_src_ip() walks
> every RDMA device for the same address, so a software iWARP device
> attached to the same netdev can win.  The event handler then runs
> the IB transport's callbacks against a device that is not one.
> 
> The rdma_cm has an API for exactly this since commit a760e80e90f5
> ("RDMA/core: introduce rdma_restrict_node_type()").  Restrict all
> three ids RDS creates - the listener, the per-connection id and the
> probe id in rds_ib_laddr_check_cm() - to RDMA_NODE_IB_CA before they
> are bound, so that the listener is only installed on IB devices, an
> outgoing connection can only bind one, and the address check's bind
> fails outright on anything else (its explicit node_type test now only
> guards against a device with no node type at all).
> 
> With that, the connect-request rejection for non-IB devices in the
> event handler becomes unreachable; it stays as the last line of
> defence and is what stable kernels without the new API rely on.
> 
> Assisted-by: Claude-Code:claude-fable-5
> Signed-off-by: Allison Henderson <achender@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260919061042.250462-1-achender@kernel.org?part=1

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 0

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

- [High] The commit message's closing paragraph asserts that "the
  connect-request rejection for non-IB devices in the event handler…
- [Medium] The patch fixes a real, reachable defect but carries no Fixes:
  tag and no stable disclosure.

--

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 and the probe id in rds_ib_laddr_check_cm()) to
RDMA_NODE_IB_CA via rdma_restrict_node_type() before they are bound, so
the listener is only installed on IB devices and an outgoing connection
can only bind one.

A couple of questions below.

> With that, the connect-request rejection for non-IB devices in the
> event handler becomes unreachable; it stays as the last line of
> defence and is what stable kernels without the new API rely on.

[Severity: High]

Which rejection is this referring to?  Looking at
rds_rdma_cm_event_handler_cmn() in net/rds/rdma_transport.c, the only
node type test has no else arm and no early return:

	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;

trans is then dereferenced unconditionally in the switch below:

	case RDMA_CM_EVENT_CONNECT_REQUEST:
		ret = trans->cm_handle_connect(cm_id, event, isv6);
		break;

and the same for trans->cm_initiate_connect() on the
RDMA_CM_EVENT_ROUTE_RESOLVED arm and trans->cm_connect_complete() on the
RDMA_CM_EVENT_ESTABLISHED arm.  rds_ib_cm_handle_connect() in
net/rds/ib_cm.c does not check node_type either, and a grep of net/rds/
for RDMA_NODE only turns up rds_ib_laddr_check_cm() and rds_ib_add_one()
in net/rds/ib.c besides the line above.

So for a device whose node_type is not RDMA_NODE_IB_CA, isn't the
described "last line of defence" actually an indirect call through an
uninitialized stack pointer rather than a rejection?  git blame points
the missing else arm at dcdede0406d30 ("RDS: Drop stale iWARP RDMA
transport").

That also affects the stable statement: kernels without
rdma_restrict_node_type() would have no protection here at all rather
than a fallback.  If the event handler check is meant to be the retained
fallback, should this patch add it, for example initializing trans = NULL
and returning an error when
cm_id->device->node_type != RDMA_NODE_IB_CA?  That form would also be
backportable on its own.

> diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
> index 6e3110a04ae6..e7014453eaec 100644
> --- a/net/rds/ib_cm.c
> +++ b/net/rds/ib_cm.c
> @@ -999,6 +999,18 @@ int rds_ib_conn_path_connect(struct rds_conn_path *cp)
>  		goto out;
>  	}
>  
> +	/* rds_ib_laddr_check() only vouched for the local address being
> +	 * on an IB device; the address resolution below picks the device
> +	 * on its own, so restrict it to the same kind.
> +	 */
> +	ret = rdma_restrict_node_type(ic->i_cm_id, RDMA_NODE_IB_CA);
> +	if (ret) {
> +		rdsdebug("rdma_restrict_node_type() failed: %d\n", ret);
> +		rdma_destroy_id(ic->i_cm_id);
> +		ic->i_cm_id = NULL;
> +		goto out;
> +	}
> +
>  	rdsdebug("created cm id %p for conn %p\n", ic->i_cm_id, conn);
>  
>  	if (ipv6_addr_v4mapped(&conn->c_faddr)) {

[ ... ]

> diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
> index b15cf316b23a..91ff1dde26af 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;
> +	}
> +

[Severity: Medium]

Should this carry a Fixes: tag?  Both halves of the change look like they
correct reachable behaviour that predates the patch.

rdma_create_id() initializes id_priv->restricted_node_type to
RDMA_NODE_UNSPECIFIED, and cma_listen_on_dev() only skips a device when a
restriction is set, so before this patch rds_rdma_listen_init_common()
installed the RDS listener on every RDMA device, iWARP RNICs included.

Similarly cma_acquire_dev_by_src_ip() walks every device, so the
rdma_resolve_addr() in rds_ib_conn_path_connect() could bind a non-IB
device for an address that rds_ib_laddr_check() only vouched for on an IB
device.

git blame suggests dcdede0406d30 ("RDS: Drop stale iWARP RDMA transport")
as the candidate commit, since it removed the iWARP transport arm while
leaving the common listener and the per-connection id unrestricted.

Since the fix depends on the new rdma_restrict_node_type() API, could the
message also state explicitly that stable trees need a separate minimal
fix, so backporters know what to pick?

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