Re: [PATCH net-next v5 09/12] net/rds: refuse to queue on a connection being destroyed

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] rds_conn_path_quiesce() (net/rds/connection.c:628-634) purges
  cp_send_queue by only retiring the *connection-side* state of each…
- [High] The new rds_destroy_pending() refusal in rds_send_queue_rm() is
  nested inside the `if (rs->rs_snd_bytes < rds_sk_sndbuf(rs))` branch…
- [High] The quiesce splices cp_send_queue into an on-stack list under
  cp_lock, drops the lock, and then walks and frees that list unlocked…
- [Medium] Concern (baseline verdict: FALSE POSITIVE for the use-after-
  free; the commit-message inaccuracy is real): queued outgoing messages…
- [Low] Moving spin_lock(&cp->cp_lock) above the socket-side bookkeeping
  in rds_send_queue_rm() (net/rds/send.c:937) makes cp_lock cover the…
- [Low] Concern (baseline verdict: FALSE POSITIVE): the patch is claimed
  to lack a Fixes:/dependency note for the hazard enabled by the…

--

net/rds: refuse to queue on a connection being destroyed

This changes rds_conn_path_quiesce() to splice cp_send_queue onto a local
list under cp_lock before purging it, and adds an rds_destroy_pending()
test under cp_lock to rds_send_queue_rm() and rds_send_probe() so that
nothing is queued after the purge.  rds_send_queue_rm() now signals the
refusal by setting *queued negative, and rds_sendmsg() turns that into
-EAGAIN.

A few questions on the commit message and the new locking below.

rds_send_queue_rm() adds to
cp_send_queue under cp_lock, so the unlocked walk races the add and
can corrupt the list.  Worse, a message added after the purge sits on
the queue of a quiesced connection holding the connection reference
rds_send_queue_rm() took for it: the reference is only dropped when
the message is freed, the message is only freed when the queue is
torn down, and the queue is only torn down by the destroy that has
already run.  The connection would never be freed, and with it the
transport could never unload.

[Severity: Medium]
Is this description accurate at this point in the series?  Reading
rds_send_queue_rm() at this commit, the queueing sequence is:

net/rds/send.c:rds_send_queue_rm() {
	...
	rds_message_populate_header(&rm->m_inc.i_hdr, sport, dport, 0);
	rm->m_inc.i_conn = conn;
	rm->m_inc.i_conn_path = cp;
	rds_message_addref(rm);
	...
}

There is no rds_conn_get() here, and rds_message_put() has no matching
rds_conn_put(), so a message does not hold a connection reference yet.
That reference appears later in the series, in "net/rds: hold a
connection reference from struct rds_incoming".

Should the wording be adjusted so it does not describe a reference that
does not exist until a later patch, or should the patch ordering change?

[ ... ]

[Severity: High]
Can this purge hit the BUG_ON for a message a socket still owns?

The commit message says "A sender that gets there first has its message
purged".  A message that got there first was linked on both queues by
rds_send_queue_rm():

net/rds/send.c:rds_send_queue_rm() {
	...
	list_add_tail(&rm->m_sock_item, &rs->rs_send_queue);
	set_bit(RDS_MSG_ON_SOCK, &rm->m_flags);
	rds_message_addref(rm);
	sock_hold(rds_rs_to_sk(rs));
	rm->m_rs = rs;
	...
	list_add_tail(&rm->m_conn_item, &cp->cp_send_queue);
	set_bit(RDS_MSG_ON_CONN, &rm->m_flags);
	...
}

m_sock_item is only unlinked by rds_send_remove_from_sock() (ack driven)
and rds_send_drop_to() (close() or RDS_CANCEL_SENT_TO).  The teardown
path rds_conn_path_drop(cp, true) -> rds_conn_shutdown() ->
rds_send_path_reset() only puts cp_xmit_rm and splices cp_retrans back
into cp_send_queue, so it does not touch the socket side.

Does that mean any unacked message still on cp_send_queue reaches the
purge with a non-empty m_sock_item and trips
BUG_ON(!list_empty(&rm->m_sock_item))?

And for messages whose m_sock_item was already unlinked by a racing
rds_send_drop_to(), do rs_snd_bytes and the sock_hold() taken above stay
charged, since the purge neither calls rds_send_sndbuf_remove() nor
sock_put()?

[Severity: High]
Separately: the splice serializes additions, but does it serialize
removals?  The purged messages keep RDS_MSG_ON_CONN set, and
rds_send_drop_to() decides ownership of the connection-queue reference
purely from that bit:

net/rds/send.c:rds_send_drop_to() {
	...
	spin_lock_irqsave(&cp->cp_lock, flags);
	if (!test_and_clear_bit(RDS_MSG_ON_CONN, &rm->m_flags)) {
		spin_unlock_irqrestore(&cp->cp_lock, flags);
		continue;
	}
	list_del_init(&rm->m_conn_item);
	spin_unlock_irqrestore(&cp->cp_lock, flags);
	...
	rds_message_put(rm);
}

After the splice, m_conn_item points into the purge list whose head is on
rds_conn_path_quiesce()'s stack.  If a close() or
setsockopt(RDS_CANCEL_SENT_TO) runs while the unlocked
list_for_each_entry_safe() walk is in progress, can drop_to unlink an
entry and rewrite that stack list head behind the walker, so the cached
next pointer is followed into an already freed message?

Does the same stale bit also give two rds_message_put() calls for one
connection-queue reference, one from the purge and one from drop_to?

The invariant in net/rds/rds.h seems to expect the flag to be cleared
along with the list removal:

 * The RDS_MSG_ON_SOCK and RDS_MSG_ON_CONN flags are used to avoid lock
 * nesting.  As paths iterate over messages on a sock, or conn, they must
 * also lock the conn, or sock, to remove the message from those lists too.

Would clearing RDS_MSG_ON_CONN under cp_lock as part of the splice, or
holding cp_lock across the walk, close this?

[Severity: High]
The refusal sits inside the "if (rs->rs_snd_bytes < rds_sk_sndbuf(rs))"
branch, so it is only evaluated when the socket already has send room.

With a full send buffer rds_send_queue_rm() returns *queued == 0 without
ever testing rds_destroy_pending(), and the sendmsg loop keeps sleeping:

net/rds/send.c:rds_sendmsg() {
	...
	timeo = wait_event_interruptible_timeout(*sk_sleep(sk),
				rds_send_queue_rm(rs, conn, cpath, rm,
						  rs->rs_bound_port,
						  dport,
						  &queued),
				timeo);
	...
	if (timeo > 0 || timeo == MAX_SCHEDULE_TIMEOUT)
		continue;
	...
}

For a blocking socket timeo is MAX_SCHEDULE_TIMEOUT, so the continue arm
re-enters the wait.  Send room is released only by
rds_send_sndbuf_remove() from rds_send_remove_from_sock() (ack driven,
which cannot happen on a quiesced connection) or from
rds_send_drop_to(), and rds_conn_path_quiesce() neither decrements
rs_snd_bytes nor calls rds_wake_sk_sleep().

Does that make the new "if (queued < 0)" escape in rds_sendmsg()
unreachable in exactly the case the commit message names, "so that the
wait loop in rds_sendmsg() stops waiting for send room that will never
come"?

Since that sleeping thread holds the connection reference rds_sendmsg()
took, does this also keep rds_conn_wait_conns_freed() blocked and the
transport module unloadable?

Would hoisting the rds_destroy_pending() test above the sndbuf check
address both?

[Severity: Low]
This isn't a bug, but with the lock taken earlier, the comment a few
lines above the removed spin_lock() is now stale:

net/rds/send.c:rds_send_queue_rm() {
	...
	/* The code ordering is a little weird, but we're
	   trying to minimize the time we hold c_lock */
	rds_message_populate_header(&rm->m_inc.i_hdr, sport, dport, 0);
	...
}

cp_lock now covers the rs_snd_bytes update, the rs_send_queue insertion,
the two rds_message_addref() calls, sock_hold() and the header memset in
rds_message_populate_header().  Could the comment be updated or dropped,
or the destroy test done in its own short cp_lock section?

[ ... ]

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