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

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

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

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 — "With that, the
  connect-request rejection for non-IB devices in the event handler…
- [Low] The patch fixes a real, remotely reachable defect (RDS installing
  its rdma_cm listener on every RDMA device — including RDMA_NODE_RNIC…
- [Low] rds: memory leak of rdma_cm_id in rds_ib_conn_path_connect error
  path

--

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(), so the listener is only
installed on IB devices and address resolution can only pick one.

A couple of questions about the commit message and about what the handler
actually does today.

The message says:

    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]
Is there actually a rejection in the event handler?  Looking at
rds_rdma_cm_event_handler_cmn() in net/rds/rdma_transport.c, trans is
declared without an initializer and only ever assigned on the IB path:

	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;

There is no else arm, and no error return.  The switch then dereferences
trans unconditionally:

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

with the same pattern for trans->cm_initiate_connect() under
RDMA_CM_EVENT_ROUTE_RESOLVED and trans->cm_connect_complete() under
RDMA_CM_EVENT_ESTABLISHED.

So on a device whose node_type is not RDMA_NODE_IB_CA, does the handler
reject anything, or does it make an indirect call through an uninitialized
stack slot?  git blame points at dcdede0406d30e ("RDS: Drop stale iWARP
RDMA transport"), which removed the

	else
		trans = &rds_iw_transport;

arm without adding a bail-out.

If that is right, then the sentence quoted above describes the opposite of
what the code does, and the trees the message says are covered have no
mitigation at all.  siw_device_create() in
drivers/infiniband/sw/siw/siw_main.c sets

	base_dev->node_type = RDMA_NODE_RNIC;

and before this patch cma_listen_on_dev() installed the RDS listener on
that device, so a remote peer connecting to the RDS port reaches the
CONNECT_REQUEST case above.

Would it make sense to also add the handler-side bail-out, something like

	if (!trans)
		return -EINVAL;

so that the "last line of defence" the message refers to exists, and so
there is something minimal to backport to trees without
rdma_restrict_node_type()?  The cm_id->device->node_type load itself is
also unguarded, while net/rds/ib.c still tests !cm_id->device.

[ ... ]

[Severity: High]
The message describes this test as follows:

    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)

Is that accurate?  Once restricted_node_type is set on the id,
cma_acquire_dev_by_src_ip() in drivers/infiniband/core/cma.c skips every
candidate device by node type:

		if (id_priv->restricted_node_type != RDMA_NODE_UNSPECIFIED &&
		    id_priv->restricted_node_type != cma_dev->device->node_type)
			continue;

Since RDMA_NODE_UNSPECIFIED is also filtered there, the
cm_id->device->node_type != RDMA_NODE_IB_CA half of the test in
rds_ib_laddr_check_cm() looks dead rather than a guard for "no node type
at all"; only the !cm_id->device half can still fire.  Could the
parenthetical be reworded?

[Severity: Low]
This new error path jumps to out and returns ret with ic->i_cm_id
still pointing at the id rdma_create_id() just handed back, and nothing
between here and the return destroys it.  Compare the
rdma_resolve_addr() failure a few lines further down, which does

	rdma_destroy_id(ic->i_cm_id);
	ic->i_cm_id = NULL;

before falling into the same out label, and the other two sites this
patch touches: in rds_ib_laddr_check_cm() the new goto out lands on a
label that calls rdma_destroy_id(cm_id), and in
rds_rdma_listen_init_common() cm_id is still non-NULL at out so it is
destroyed there too.  So this is the one of the three new calls whose
failure leaks the id, i.e. a struct rdma_id_private and its restrack
entry.

In practice it cannot be reached: rdma_restrict_node_type() only
returns -EINVAL for a node type it does not recognise and -EALREADY if
the id is not in RDMA_CM_IDLE, and a freshly created kernel id being
restricted to RDMA_NODE_IB_CA is neither, so this is a structural
defect rather than something a peer or a local user can trigger.

Still, the surrounding code is careful about this, so which way did you
intend it?  Is the connection teardown path (rds_ib_conn_shutdown(),
which destroys ic->i_cm_id when it is set) meant to clean up after a
failed rds_ib_conn_path_connect(), or should this arm mirror the
resolve failure and do the rdma_destroy_id()/NULL itself?  If the
former, the asymmetry with the rdma_resolve_addr() arm immediately
below is confusing enough that it is worth saying so in a comment.

[Severity: Low]
Should this change carry a Fixes: tag, and a note for stable?  The defect
being closed is remotely reachable (the listener was previously installed
on RDMA_NODE_RNIC devices such as siw, and the handler then runs with
trans unassigned), but there is no Fixes: line among the trailers.

The three new calls here, in rds_ib_conn_path_connect() and in
rds_ib_laddr_check_cm() all depend on a760e80e90f5 ("RDMA/core: introduce
rdma_restrict_node_type()"), so an automated backport to a tree without
that commit will not build.  Could the message state either that
a760e80e90f5 is a prerequisite, or that stable should take a minimal
handler-side fix instead?

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