Re: [PATCH net-next v8 03/13] net/rds: ib: refuse to attach a connection to a device being removed

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message gets the trigger wrong.

--

net/rds: ib: refuse to attach a connection to a device being removed

This patch adds a shutting_down flag to struct rds_ib_device.
rds_ib_dev_shutdown() sets it under rds_ibdev->spinlock before walking
conn_list. rds_ib_add_conn() checks the flag under the same lock and
returns -ENODEV, which makes rds_ib_setup_qp() fail the connect.

> rds_ib_remove_one() drops every connection on the device's conn_list
> in rds_ib_dev_shutdown(), then clears the client data so that no new
> connect can find the device.  A connect that is already past
> rds_ib_get_client_data() when the walk runs is not covered by either:
> rds_ib_setup_qp() goes on to rds_ib_add_conn(), which moves the
> connection onto the conn_list the walk has just finished with, and
> builds a QP on a device that is on its way out.  Nothing drops that
> connection afterwards - the device's shutdown walk is over, and the
> connection never returns to ib_nodev_conns, which is the only list the
> transport exit sweeps - so it outlives the device, and the module.

[Severity: Low]
Does this description hold for a real IB device hot-removal?

disable_device() in drivers/infiniband/core/device.c removes clients in
LIFO order. That means rds_ib_remove_one() runs before rdma_cm's
cma_remove_one().

cma_process_remove() then sends DEVICE_REMOVAL to every cm_id still on the
device, including the late connection's ic->i_cm_id. It then waits for
those ids to be destroyed:

cma_remove_one()
  cma_process_remove()
    cma_send_device_removal_put(id_priv)
      ...
        rds_rdma_cm_event_handler_cmn()

net/rds/rdma_transport.c:rds_rdma_cm_event_handler_cmn() {
    ...
	case RDMA_CM_EVENT_DEVICE_REMOVAL:
	case RDMA_CM_EVENT_ADDR_CHANGE:
		if (conn)
			rds_conn_drop(conn);
		break;
    ...
}

During a plain hot-removal, rds_destroy_pending() is false, so this
rds_conn_drop() queues the shutdown. The shutdown tears down the QP and
cm_id, and rds_ib_remove_conn() puts the connection back on
ib_nodev_conns. So on hot-removal the connection does not seem to outlive
the device.

The leak described here seems to need the module unload path instead:

rds_ib_exit()
  rds_ib_set_unloading()
  rds_ib_unregister_client()
    rds_ib_remove_one()    <- device stays registered, no DEVICE_REMOVAL
  rds_ib_destroy_nodev_conns()

On this path rds_destroy_pending() is true, so every non-destroy
rds_conn_drop() returns early. rds_ib_destroy_nodev_conns() only sweeps
ib_nodev_conns. A connection attached after the walk is then leaked.

The code change looks correct for both cases. Could the commit message
name the trigger as unloading rds_rdma while a connect is in flight,
rather than device removal in general? The patch carries a Fixes: tag, so
stable backporters will use this description to judge how the bug
reproduces and what it affects.

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