Re: [PATCH net-next 1/6] net/rds: make rds_destroy_pending() cover single-connection destroy

[netdev-bot+sashiko@kernel.org]

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

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

- [High] The fix is incomplete against its own stated invariant, so the
  bug class it claims to close stays reachable.
- [Medium] The commit message mischaracterizes which destroy paths this
  patch actually newly guards.
- [Low] The comment on the new struct field misdescribes the write side.

--

net/rds: make rds_destroy_pending() cover single-connection destroy

This adds a per-connection c_destroy_in_prog flag, set in
rds_conn_destroy() before the unhash + synchronize_rcu() sequence and
tested in rds_destroy_pending(), and removes the now-dead
RDS_DESTROY_PENDING cp_flags bit along with its test in
rds_ib_is_unloading().

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index b6c4beb50eaf0..50e1b6bfceeab 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c
> @@ -574,6 +574,14 @@ void rds_conn_destroy(struct rds_connection *conn)
>  		 "%pI4\n", conn, &conn->c_laddr,
>  		 &conn->c_faddr);
>  
> +	/* Make rds_destroy_pending() true for this conn.  Together with
> +	 * the synchronize_rcu() below this stops the work-requeueing
> +	 * sites (which all test rds_destroy_pending() under
> +	 * rcu_read_lock()) from queueing new work on the path
> +	 * workqueues once we start cancelling and destroying them.
> +	 */
> +	WRITE_ONCE(conn->c_destroy_in_prog, true);

[Severity: High]
Is the claim in this comment that the requeueing sites "all test
rds_destroy_pending() under rcu_read_lock()" accurate for the current
tree?

A grep of the queue_work()/queue_delayed_work() callers under net/rds
turns up several arming sites that never consult the predicate at all,
so the new flag cannot suppress them:

net/rds/ib_send.c:rds_ib_send_cqe_handler()
	if (test_and_clear_bit(RDS_LL_SEND_FULL, &conn->c_flags) ||
	    test_bit(0, &conn->c_map_queued))
		queue_delayed_work(conn->c_path->cp_wq, &conn->c_send_w, 0);

net/rds/ib_send.c:rds_ib_send_add_credits()
	if (test_and_clear_bit(RDS_LL_SEND_FULL, &conn->c_flags))
		queue_delayed_work(conn->c_path->cp_wq, &conn->c_send_w, 0);

net/rds/ib_recv.c:rds_ib_recv_refill()
		queue_delayed_work(conn->c_path->cp_wq, &conn->c_recv_w, 1);

net/rds/tcp_listen.c:rds_tcp_accept_one()
		queue_delayed_work(cp->cp_wq, &cp->cp_recv_w, 0);

net/rds/send.c:rds_sendmsg()
		if (!test_and_set_bit(RDS_RECONNECT_PENDING,
				      &conn->c_path[0].cp_flags))
			queue_delayed_work(conn->c_path[0].cp_wq,
					   &conn->c_path[0].cp_conn_w, 0);

Also, the one rds_destroy_pending(conn) test that rds_sendmsg() does
perform is not inside an RCU read-side section (send.c holds
rcu_read_lock() only around the three cp_send_w arming sites), so
the synchronize_rcu() in rds_conn_destroy() does not order against that
reader either.

Given that, can the window that rds_conn_path_quiesce() already warns
about still end in a crash?  It cancels the works while the path is
still RDS_CONN_UP:

	cancel_delayed_work_sync(&cp->cp_send_w);
	cancel_delayed_work_sync(&cp->cp_recv_w);

	rds_conn_path_drop(cp, true);
	flush_work(&cp->cp_down_w);
	...
	WARN_ON(delayed_work_pending(&cp->cp_send_w));
	WARN_ON(delayed_work_pending(&cp->cp_recv_w));
	WARN_ON(delayed_work_pending(&cp->cp_conn_w));

and teardown then proceeds regardless of the warning to

		destroy_workqueue(cp->cp_wq);
		cp->cp_wq = NULL;

followed by kfree(conn->c_path) in rds_conn_destroy().  An IB send or
receive completion landing in that window re-arms through one of the
unguarded sites above.  With delay 0 and cp_wq already NULL,
__queue_delayed_work() falls through to __queue_work(cpu, NULL, work);
with the delay 1 arming in rds_ib_recv_refill() the timer is invisible
to destroy_workqueue() and delayed_work_timer_fn() later calls
__queue_work(dwork->cpu, dwork->wq, ...) on the destroyed workqueue,
from a timer_list living in the freed c_path array.

Would it make sense to either extend the rcu_read_lock() +
rds_destroy_pending() pattern to those sites (or move the workqueue
teardown behind the transport quiesce), or otherwise to soften the
blanket coverage claim in this comment and in the changelog?

>  
>  	/* Ensure conn will not be scheduled for reconnect */
>  	spin_lock_irq(&rds_conn_lock);
>  	hlist_del_init_rcu(&conn->c_hash_node);
> diff --git a/net/rds/ib.c b/net/rds/ib.c
> index 786f39169bc14..9fe3b9951bd32 100644
> --- a/net/rds/ib.c
> +++ b/net/rds/ib.c
> @@ -525,10 +525,7 @@ static void rds_ib_set_unloading(void)
>  
>  static bool rds_ib_is_unloading(struct rds_connection *conn)
>  {
> -	struct rds_conn_path *cp = &conn->c_path[0];
> -
> -	return (test_bit(RDS_DESTROY_PENDING, &cp->cp_flags) ||
> -		atomic_read(&rds_ib_unloading) != 0);
> +	return atomic_read(&rds_ib_unloading) != 0;
>  }

[Severity: Medium]
Does the changelog describe the right set of paths that this patch newly
guards?  It says:

    rds_conn_destroy() is not limited to the global paths: rds_ib
    destroys connections whose underlying IB device was removed
    (rds_ib_destroy_nodev_conns())

and

    Bring the protection back at the connection level, where it also
    covers the device-removal path that was never guarded

At this revision net/rds/ib.c has no rds_conn_destroy() call at all; the
device removal path (rds_ib_remove_one() / rds_ib_dev_shutdown() /
rds_ib_remove_conn()) only drops connections and moves them onto
ib_nodev_conns.

rds_ib_destroy_nodev_conns() has a single caller, rds_ib_exit(), which
runs before it:

net/rds/ib.c:rds_ib_exit() {
	rds_ib_set_unloading();
	synchronize_rcu();
	...
	rds_ib_unregister_client();
	rds_ib_destroy_nodev_conns();
	...
}

Since rds_ib_is_unloading() is the IB ->t_unloading callback and returns
atomic_read(&rds_ib_unloading) != 0 for the whole of rds_ib_exit(),
wasn't rds_destroy_pending() already true for every conn destroyed on
that path, so the new flag adds nothing there?

That would leave the version-mismatch rds_conn_destroy() in
rds_ib_cm_connect_complete() as the only genuinely unguarded per-conn
destroy, which matches the Fixes: cdc306a5c9cd3 tag.  Could the
changelog be narrowed to that, so stable backport triage does not read
it as "hot-removing an IB device can queue work on a destroyed cp_wq"?

>  
>  void rds_ib_exit(void)
> diff --git a/net/rds/rds.h b/net/rds/rds.h
> index 2db49573dacd5..cede2b03baa50 100644
> --- a/net/rds/rds.h
> +++ b/net/rds/rds.h

[ ... ]

> @@ -148,6 +147,10 @@ struct rds_connection {
>  				c_pad_to_32:29;
>  	int			c_npaths;
>  	bool			c_with_sport_idx;
> +	/* Set (under RCU) when rds_conn_destroy() starts on this conn;
> +	 * read through rds_destroy_pending().
> +	 */
> +	bool			c_destroy_in_prog;

[Severity: Low]
This isn't a bug, but does "Set (under RCU)" match the setter?  The only
write is the plain WRITE_ONCE(conn->c_destroy_in_prog, true) in
rds_conn_destroy(), which is not inside an rcu_read_lock() section; the
RCU involvement is the synchronize_rcu() that follows.

Would it also be worth spelling out the reader-side requirement here,
namely that rcu_read_lock() must be held across both the
rds_destroy_pending() test and the queue_work()/queue_delayed_work()
that follows?  As written, the field comment permits the pattern already
present in rds_sendmsg():

net/rds/send.c:rds_sendmsg() {
	...
	if (rds_destroy_pending(conn)) {
		ret = -EAGAIN;
		goto out;
	}
	...
}

where the predicate is read outside any RCU read section and the
synchronize_rcu() in rds_conn_destroy() fences nothing.

>  	struct rds_connection	*c_passive;
>  	struct rds_transport	*c_trans;
>  
> @@ -994,7 +997,8 @@ void __rds_put_mr_final(struct kref *kref);
>  
>  static inline bool rds_destroy_pending(struct rds_connection *conn)
>  {
> -	return !check_net(rds_conn_net(conn)) ||
> +	return READ_ONCE(conn->c_destroy_in_prog) ||
> +	       !check_net(rds_conn_net(conn)) ||
>  	       (conn->c_trans->t_unloading && conn->c_trans->t_unloading(conn));
>  }

Thanks for looking at these.

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