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

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

- [Medium] Incomplete coverage vs. the patch's own claim: the subject
  says "guard every work-requeueing site with rds_destroy_pending()",…

--

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

This adds the rcu_read_lock() + rds_destroy_pending() guard around five
path-work arming sites: the two cp_send_w re-arms in ib_send.c, the
cp_recv_w re-arm in rds_ib_recv_refill(), the cp_recv_w kick in
rds_tcp_accept_one(), and the cp_conn_w arm in rds_sendmsg().

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

[Severity: Medium]
Is "every work-requeueing site" accurate here?  After this patch four
arming sites in net/rds/threads.c still queue path works with no
rcu_read_lock() and no rds_destroy_pending() test.

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

rds_recv_worker():

	switch (ret) {
	case -EAGAIN:
		rds_stats_inc(s_recv_immediate_retry);
		queue_delayed_work(cp->cp_wq, &cp->cp_recv_w, 0);
		break;
	case -ENOMEM:
		rds_stats_inc(s_recv_delayed_retry);
		queue_delayed_work(cp->cp_wq, &cp->cp_recv_w, 2);
		break;

These four look functionally safe to me, since they are self-requeues
issued from inside the running work item and rds_conn_path_destroy()
disables the work before flushing it:

net/rds/connection.c:rds_conn_path_destroy() {
	/* make sure lingering queued work won't try to ref the conn */
	cancel_delayed_work_sync(&cp->cp_send_w);
	cancel_delayed_work_sync(&cp->cp_recv_w);
	...
}

__cancel_work_sync() grabs the pending bit (deleting any armed
delayed-work timer) and calls work_offqd_disable() before flushing, and
queue_delayed_work_on() then drops the queue attempt via
clear_pending_if_disabled(), so a requeue from the callback that is
being flushed is discarded and the callback cannot run again afterwards.

The following patch in the series also adds a comment on
struct rds_connection::c_destroy_in_prog that states the rule with no
exception:

	 * Set once, by rds_conn_destroy(), before it cancels the path
	 * works; read through rds_destroy_pending().  A site that arms
	 * a path work must test the predicate and queue the work inside
	 * one rcu_read_lock() section: ...

Could either the subject and changelog note the self-requeue exemption
(and the cancel_delayed_work_sync() reason it holds), or the four
threads.c sites be converted too, so the documented invariant and the
code agree?

Also, the changelog argues for the IB sites:

>     The IB completion sites are reachable from soft-irq at any point
>     before the QP is drained, so a completion landing in the window
>     between the cancel and destroy_workqueue() in rds_conn_path_destroy()
>     re-arms a work on a workqueue that is about to be destroyed: with
>     delay 0 the work is queued directly on the freed workqueue, and with
>     delay 1 the timer survives destroy_workqueue() unseen and fires
>     afterwards, queueing from a timer_list that lives in the freed c_path
>     array.

The same delay-2 timer shape appears in the threads.c -ENOMEM cases, so
a reader may conclude those are equally exposed.  Would it help to say
explicitly why the threads.c requeues are not in the same category?

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