Re: [PATCH net-next v9 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 2 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 0 · 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] In the target commit, the fix only covers half of the in-flight-
  connect race on rds_rdma unload.
- [High] The new refusal path in rds_ib_setup_qp()
  (net/rds/ib_cm.c:527-529) jumps to 'out:' and calls…

--

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 it walks
conn_list. rds_ib_add_conn() now refuses, with -ENODEV, to attach a
connection to a device marked that way, and rds_ib_setup_qp() fails the
connect when that happens.

> diff --git a/net/rds/ib.c b/net/rds/ib.c
> index 4ea9838d090c3..a7647ec01a115 100644
> --- a/net/rds/ib.c
> +++ b/net/rds/ib.c
> @@ -86,6 +86,11 @@ static void rds_ib_dev_shutdown(struct rds_ib_device *rds_ibdev)
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&rds_ibdev->spinlock, flags);
> +	/* Close the device to new connections under the same lock that
> +	 * rds_ib_add_conn() attaches them under, so that every
> +	 * connection is either dropped by the walk below or refused.
> +	 */
> +	rds_ibdev->shutting_down = true;
>  	list_for_each_entry(ic, &rds_ibdev->conn_list, ib_node)
>  		rds_conn_path_drop(&ic->conn->c_path[0], true);

[Severity: High]
When rds_rdma is unloaded, are the connections this walk drops actually
torn down?

rds_conn_path_drop(cp, true) only queues cp_down_w on cp->cp_wq.
__rds_conn_create() gives every path its own ordered workqueue. The
connection goes back to ib_nodev_conns only when that worker reaches:

rds_ib_conn_path_shutdown()
    if (ic->rds_ibdev)
        rds_ib_remove_conn(ic->rds_ibdev, conn);

Nothing on the unload path waits for that worker:

rds_ib_exit()
    rds_ib_unregister_client()
        ib_unregister_client(&rds_ib_client);
        flush_workqueue(rds_wq);    <-- rds_wq only, not cp_wq
    rds_ib_destroy_nodev_conns();

cp_down_w can still be running when the sweep happens. For example, it
can be blocked on c_cm_lock behind a connect that attached just before
shutting_down was set, or waiting for the rings to drain. In that case
the connection is still on the device's conn_list, and the sweep misses
it.

rds_ib_destroy_nodev_conns() also does not reinitialise the global head
after the splice:

	spin_lock_irq(&ib_nodev_conns_lock);
	list_splice(&ib_nodev_conns, &tmp_list);
	spin_unlock_irq(&ib_nodev_conns_lock);

A late rds_ib_remove_conn() would then link the node through the stale
ib_nodev_conns head into the sweep's on-stack tmp_list.

Could this leave the connection, with its QP, cm_id, cp_wq and device
reference, alive after the module is gone, and late shutdown work running
rds_rdma code? If so, the new comment's claim that every connection is
"either dropped by the walk below or refused" holds at this commit only
for the refused case.

Later patches in the series appear to fix this. "net/rds: unlink
transport nodes before a possibly deferred connection free" replaces the
list_splice() with a per-entry claim under ib_nodev_conns_lock. "net/rds:
wait for connections to be freed on transport unload" adds
rds_conn_wait_conns_freed(), which sweeps again for late arrivals and
then flushes rds_wq once more. Should those changes come before this
patch, so that a bisect cannot land on this intermediate state?

[ ... ]

> diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
> index 8b16bd7c40ce5..82ecbb9a3da1f 100644
> --- a/net/rds/ib_cm.c
> +++ b/net/rds/ib_cm.c
> @@ -524,7 +524,9 @@ static int rds_ib_setup_qp(struct rds_connection *conn)
>  	fr_queue_space = RDS_IB_DEFAULT_FR_WR;
>  
>  	/* add the conn now so that connection establishment has the dev */
> -	rds_ib_add_conn(rds_ibdev, conn);
> +	ret = rds_ib_add_conn(rds_ibdev, conn);
> +	if (ret)
> +		goto out;

[Severity: High]
Can the rds_ib_dev_put() on this refusal path be the last put on the
device, and can it happen after the only rds_wq flush on unload?

The out label drops the reference taken by rds_ib_get_client_data():

out:
	rds_ib_dev_put(rds_ibdev);

Once shutting_down is set, rds_ib_remove_one() goes on to drop both of
the device's own references:

	synchronize_rcu();
	rds_ib_dev_put(rds_ibdev);
	rds_ib_dev_put(rds_ibdev);

A connect sitting between rds_ib_get_client_data() and rds_ib_add_conn()
then holds the last reference, and rds_ib_dev_put() does:

	if (refcount_dec_and_test(&rds_ibdev->refcount))
		queue_work(rds_wq, &rds_ibdev->free_work);

The CM handler holds c_cm_lock across rds_ib_setup_qp(). The exit sweep's
rds_conn_destroy() of the refused connection waits on that lock. So the
put can happen during the sweep, after the single flush, and
rds_ib_exit() does not flush rds_wq again:

rds_ib_exit()
    rds_ib_unregister_client()   <-- only flush_workqueue(rds_wq)
    rds_ib_destroy_nodev_conns()
    rds_ib_sysctl_exit()
    rds_ib_recv_exit()
    rds_trans_unregister(&rds_ib_transport)
    rds_ib_mr_exit()             <-- destroys rds_ib_mr_wq

rds_wq belongs to rds.ko and stays alive. Could rds_ib_dev_free()
(rds_ib_destroy_mr_pool(), ib_dealloc_pd(), kfree()) race with
destroy_workqueue(rds_ib_mr_wq), or run after the rds_rdma text has been
unloaded?

This looks new with this patch. Before it, the same connect attached and
leaked, so the device stayed referenced. The commit message says:

    rds_ib_setup_qp() has not taken anything from the
    device at that point, so the failure needs no unwinding beyond the
    client-data reference it already releases.

That release can now be the final, deferred put.

This also appears to be fixed later in the series. "net/rds: wait for
connections to be freed on transport unload" adds a second
flush_workqueue(rds_wq) after rds_conn_wait_conns_freed() in
rds_ib_exit().

[ ... ]

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