Re: [PATCH net-next v2 1/9] 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 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 2

- [Low] At this commit the five newly added `rcu_read_lock() +
  rds_destroy_pending()` guards (net/rds/ib_send.c:302-306 and :429-433,…
- [Low] The subject claims to guard "every work-requeueing site" and the
  body states exactly "Five arming sites never got that guard", but four…

--

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

This adds rcu_read_lock() + rds_destroy_pending() guards around five
queue_delayed_work() sites that re-arm cp_send_w, cp_recv_w and cp_conn_w:
two in net/rds/ib_send.c, one in net/rds/ib_recv.c, one in
net/rds/tcp_listen.c and one in net/rds/send.c.  The stated goal is that no
new path work can be queued once rds_conn_path_destroy() has started
cancelling works and is about to call destroy_workqueue().

A couple of questions on the changelog and on the effect of the guards at
this commit.

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

[ ... ]

>     Five arming sites never got that guard:

[Severity: Low]
Are those five really all of the requeueing sites?  net/rds/threads.c has
four more queue_delayed_work() calls on the same works that stay unguarded.

rds_send_worker():

	switch (ret) {
	case -EAGAIN:
		rds_stats_inc(s_send_immediate_retry);
		queue_delayed_work(cp->cp_wq, &cp->cp_send_w, 0);
		break;
	case -ENOMEM:
		rds_stats_inc(s_send_delayed_retry);
		queue_delayed_work(cp->cp_wq, &cp->cp_send_w, 2);

rds_recv_worker() has the matching pair for cp_recv_w.

These four are self-requeues issued from inside the running work item, and
in this tree cancel_delayed_work_sync() -> __cancel_work_sync() disables the
work with WORK_CANCEL_DISABLE before __flush_work(), so a requeue from the
still-running callback is rejected while the cancel is in progress and no
use-after-free follows.

The subject says "every work-requeueing site" and the body gives an exact
count of five, and the following patch in the series repeats that "all of
those sites guard the queueing with rds_destroy_pending() under
rcu_read_lock() (the last stragglers were converted by the previous patch)".
Could the changelog either convert the threads.c sites too, or say that
in-callback self-requeues are exempt and why?

The Fixes: tag points at ebeeb1ad9b8a, and on trees before
cancel_delayed_work_sync() gained the disable semantics a self-requeue from
the callback was not rejected, so a backporter reading this message cannot
tell whether those sites were considered.

> diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c
> index bd6cb3ffaa571..7d45808544a0d 100644
> --- a/net/rds/ib_recv.c
> +++ b/net/rds/ib_recv.c
> @@ -458,7 +458,11 @@ void rds_ib_recv_refill(struct rds_connection *conn, int prefill, gfp_t gfp)
>  	    (must_wake ||
>  	    (can_wait && rds_ib_ring_low(&ic->i_recv_ring)) ||
>  	    rds_ib_ring_empty(&ic->i_recv_ring))) {
> -		queue_delayed_work(conn->c_path->cp_wq, &conn->c_recv_w, 1);
> +		rcu_read_lock();
> +		if (!rds_destroy_pending(conn))
> +			queue_delayed_work(conn->c_path->cp_wq,
> +					   &conn->c_recv_w, 1);
> +		rcu_read_unlock();
>  	}

[Severity: Low]
Can these guards close the cancel -> destroy_workqueue() window as of this
commit?  rds_destroy_pending() in net/rds/rds.h carries no per-connection
destroy state here:

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));
}

and rds_conn_destroy() publishes nothing that the guards can observe before
the grace period:

	spin_lock_irq(&rds_conn_lock);
	hlist_del_init_rcu(&conn->c_hash_node);
	spin_unlock_irq(&rds_conn_lock);
	synchronize_rcu();

For a single-connection teardown with a live netns and the module loaded,
for example the protocol version mismatch path
rds_ib_cm_connect_complete() -> rds_conn_destroy(), the predicate reads
false, so a completion landing in rds_ib_recv_refill() or
rds_ib_send_cqe_handler() can still arm cp_recv_w/cp_send_w between
cancel_delayed_work_sync() and destroy_workqueue() in
rds_conn_path_destroy().

The next patch in the series, "net/rds: make rds_destroy_pending() cover
single-connection destroy", adds conn->c_destroy_in_prog, sets it under
rds_conn_lock before hlist_del_init_rcu() and synchronize_rcu(), and tests
it first in rds_destroy_pending(), which is what gives these five guards
their effect, so at the end of the series the window is closed.

Given the Fixes: tag invites a standalone backport, would it help to state
in this changelog that the guards only take effect together with the
c_destroy_in_prog change, or to reorder the two patches?

[ ... ]

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