Re: [PATCH net-next 2/6] net/rds: split connection destroy into quiesce and kref-governed free

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (1) · Medium: 0 · Low: 3

- [Low] Commit message misattributes the non-rmmod destroy paths.
- [Low] Concern (assessed as a false positive, retained for policy
  reasons): the new c_refcount comment in net/rds/rds.h:140-144 was read…
- [Low] Stale symbol reference after the rename performed by this patch:
  the correctness argument inside rds_conn_shutdown() still says…

Pre-existing issues:
- [High] Deadlock when an IB connection is destroyed from the CM event
  handler after unsupported protocol negotiation:…

--

net/rds: split connection destroy into quiesce and kref-governed free

This patch adds a kref to struct rds_connection and splits the teardown so
that rds_conn_destroy() still quiesces synchronously while the frees (the
transport conn_free, the path workqueues, the c_path array and the slab
object) move to rds_conn_destroy_fini(), which runs from the last
rds_conn_put().  rds_conn_get()/rds_conn_put() are exported for the
reference holders added in later patches.

No Link: tags are present in the commit header.

A couple of questions below, one about the changelog and two about the
teardown path.

[Severity: Low]

The changelog says:

    Today, connections can also be destroyed when the underlying IB
    device is removed (in rds_ib_destroy_nodev_conns()), or when a peer
    negotiates an unsupported protocol version.

Is rds_ib_destroy_nodev_conns() the device removal path?  It has a single
caller, rds_ib_exit(), which is the rds_rdma module unload path, i.e. the
same rmmod case the old comment allowed.

The IB device removal path, rds_ib_dev_shutdown(), only drops paths:

	list_for_each_entry(ic, &rds_ibdev->conn_list, ib_node)
		rds_conn_path_drop(&ic->conn->c_path[0], true);

Meanwhile the two destroy paths that really are not rmmod are the netns
teardown ones, and they are not mentioned:

	rds_tcp_exit_net() -> rds_tcp_kill_sock() -> rds_conn_destroy()
	rds_loop_exit_net() -> rds_loop_kill_conns() -> rds_conn_destroy()

Would citing those two, together with the peer protocol version case in
rds_ib_cm_connect_complete(), describe the motivation more accurately?

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index 50e1b6bfceeab..f45fd1fb1843d 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c

[ ... ]

> @@ -515,10 +516,12 @@ void rds_conn_shutdown(struct rds_conn_path *cp)
>  		conn->c_trans->conn_slots_available(conn, false);
>  }
>  
> -/* destroy a single rds_conn_path. rds_conn_destroy() iterates over
> - * all paths using rds_conn_path_destroy()
> +/* quiesce a single rds_conn_path: shut it down and tear down any
> + * queued messages.  rds_conn_destroy() iterates over all paths using
> + * rds_conn_path_quiesce(); the transport state and the workqueue are
> + * freed later, from rds_conn_path_free().
>   */
> -static void rds_conn_path_destroy(struct rds_conn_path *cp)

[Severity: Low]

With rds_conn_path_destroy() gone, the comment inside rds_conn_shutdown()
that justifies the cancel_delayed_work_sync(&cp->cp_conn_w) call still
names it:

			 * Quiesce the reconnect timer before bailing
			 * out, though.  When a pending destroy did
			 * suppress the queue, no later pass runs, and
			 * rds_conn_path_destroy() is about to flush
			 * cp_down_w and free the path: it must not
			 * find cp_conn_w still armed.

That comment is now the only match for rds_conn_path_destroy in the tree,
and it stays that way through the end of the series.  Since the flush of
cp_down_w is done by rds_conn_path_quiesce() while the free happens later
in rds_conn_path_free(), should the comment be reworded to name
rds_conn_path_quiesce() and note that the free is deferred?

> +static void rds_conn_path_quiesce(struct rds_conn_path *cp)
>  {
>  	struct rds_message *rm, *rtmp;
>  

[Severity: High]

This isn't a bug introduced by this patch, the same sequence exists in the
baseline rds_conn_path_destroy(), but can the quiesce deadlock when it is
reached from the RDMA CM event handler?

rds_conn_path_quiesce() keeps the drop plus synchronous flush:

	rds_conn_path_drop(cp, true);
	flush_work(&cp->cp_down_w);

rds_rdma_cm_event_handler_cmn() holds the mutex across the callback:

	if (conn) {
		mutex_lock(&conn->c_cm_lock);
	...
		trans->cm_connect_complete(conn, event);

and rds_single_path.h aliases that mutex to the path one:

	#define	c_cm_lock		c_path[0].cp_cm_lock

If the peer negotiates a version below RDS_PROTOCOL_VERSION,
rds_ib_cm_connect_complete() destroys the connection with that mutex still
held:

		rds_conn_destroy(conn);
		return;

rds_conn_path_drop(cp, true) sets cp_state to RDS_CONN_ERROR and queues
cp_down_w, so the flushed worker runs rds_shutdown_worker() ->
rds_conn_shutdown(), where the initial transition fails and it blocks on
the same mutex:

	if (!rds_conn_path_transition(cp, RDS_CONN_DOWN, RDS_CONN_DOWN)) {
		...
		mutex_lock(&cp->cp_cm_lock);

Does flush_work() then wait for a worker that is waiting for the mutex the
blocked CM handler owns?  The chain has no trylock or timeout, and it is
unchanged at the head of the series, so would this be a good place to
break the cycle while the destroy path is being reworked?

>  	WARN_ON(delayed_work_pending(&cp->cp_recv_w));
>  	WARN_ON(delayed_work_pending(&cp->cp_conn_w));
>  	WARN_ON(work_pending(&cp->cp_down_w));

[ ... ]

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