Re: [PATCH net-next v5 03/12] net/rds: guard every work-requeueing site with rds_destroy_pending()

[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] Commit-message precision only (no reachable bug at this SHA): the
  opening paragraph of "net/rds: guard every work-requeueing site with…

--

net/rds: guard every work-requeueing site with rds_destroy_pending()

This adds the rcu_read_lock() + rds_destroy_pending() guard to five sites
that re-arm path works: the two IB send paths, the IB recv refill, the TCP
accept path and the multipath reconnect in rds_sendmsg().  The four
self-requeues in rds_send_worker() and rds_recv_worker() are deliberately
left unguarded.

The code changes themselves look fine to me; my only question is about the
changelog, so the diff hunks are snipped.

> net/rds: guard every work-requeueing site with rds_destroy_pending()
>
> rds_conn_destroy() cancels the path works and then destroys the
> per-path workqueue.  The sites that can re-arm those works are
> supposed to test rds_destroy_pending() under rcu_read_lock() first,
> paired with the synchronize_rcu() in the destroy path, so that no new
> work can be queued once the cancellation has begun.

[Severity: Low]

Is "paired with the synchronize_rcu() in the destroy path" the right
attribution at this point in the series?

At this commit the predicate has no per-connection destroy term:

net/rds/rds.h:
	static inline bool rds_destroy_pending(struct rds_connection *conn)
	{
		return !check_net(rds_conn_net(conn)) ||
		       (conn->c_trans->t_unloading && conn->c_trans->t_unloading(conn));
	}

The only per-path destroy bit the IB hook consults is RDS_DESTROY_PENDING:

net/rds/ib.c:rds_ib_is_unloading()
	return (test_bit(RDS_DESTROY_PENDING, &cp->cp_flags) ||
		atomic_read(&rds_ib_unloading) != 0);

and that bit is defined in rds.h but never set anywhere in net/rds, so the
IB hook reduces to "the module is unloading".

The synchronize_rcu() inside rds_conn_destroy() publishes only the
hlist_del_init_rcu() of c_hash_node:

net/rds/connection.c:rds_conn_destroy()
	spin_lock_irq(&rds_conn_lock);
	hlist_del_init_rcu(&conn->c_hash_node);
	spin_unlock_irq(&rds_conn_lock);
	synchronize_rcu();

rds_destroy_pending() does not read c_hash_node, so that grace period is
not what makes the new guards fire.  The grace periods that do are in the
callers: rds_ib_exit() and rds_tcp_exit() call their set_unloading()
helpers followed by synchronize_rcu(), and cleanup_net() is what makes
check_net() false for the netns-teardown callers.

The last paragraph of the changelog does disclose the limitation:

> With the predicate as it stands the guards cover the netns teardown and
> module unload cases; the following patch extends it to the destroy of a
> single connection.

and the following patch in the series does close the gap: rds.h grows
READ_ONCE(conn->c_destroy_in_prog) in the predicate, rds_conn_destroy()
does WRITE_ONCE(conn->c_destroy_in_prog, true) before hlist_del_init_rcu()
and synchronize_rcu(), RDS_DESTROY_PENDING is removed, and all five guards
added here survive.

Walking the rds_conn_destroy() callers at this commit, none of them runs
with the netns alive and the module loaded: rds_ib_destroy_nodev_conns()
is reached only from rds_ib_exit(), rds_tcp_destroy_conns() and
rds_tcp_kill_sock() only from rds_tcp_exit() and rds_tcp_exit_net(),
rds_loop_kill_conns() likewise, and ib_cm.c no longer calls it.  So no
crash path is reachable here and no code change seems needed - would it be
worth rewording the first paragraph to credit the unload and netns grace
periods rather than rds_conn_destroy()'s own synchronize_rcu()?

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