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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 3 · Medium: 0 · 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() still asserts
  BUG_ON(!list_empty(&rm->m_sock_item)) for every message it takes off…
- [High] The quiesce purge unlinks m_conn_item and drops the connection-
  queue message reference without clearing RDS_MSG_ON_CONN and without…
- [High] The new rds_destroy_pending() refusal in rds_send_queue_rm() is
  placed inside the `if (rs->rs_snd_bytes < rds_sk_sndbuf(rs))` branch,…
- [Low] The comment "The code ordering is a little weird, but we're
  trying to minimize the time we hold c_lock" in rds_send_queue_rm() now…
- [Low] Series bisectability: this patch repairs a defect introduced
  earlier in the same series (the preceding commit "net/rds: hold…

--

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

This makes rds_conn_path_quiesce() splice cp_send_queue away under cp_lock
instead of walking it unlocked, and adds an rds_destroy_pending() test under
that same lock to rds_send_queue_rm() and rds_send_probe() so nothing is
queued after the purge.  rds_send_queue_rm() reports the refusal by setting
*queued negative and rds_sendmsg() turns that into -EAGAIN.

A few questions about the new purge and the placement of the refusal test.

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index 965d68e51a1c1..5699f45e4c377 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c
> @@ -606,6 +606,8 @@ void rds_conn_shutdown(struct rds_conn_path *cp)
>  static void rds_conn_path_quiesce(struct rds_conn_path *cp)
>  {
>  	struct rds_message *rm, *rtmp;
> +	unsigned long flags;
> +	LIST_HEAD(purge);
>  
>  	if (!cp->cp_transport_data)
>  		return;
> @@ -617,10 +619,16 @@ static void rds_conn_path_quiesce(struct rds_conn_path *cp)
>  	rds_conn_path_drop(cp, true);
>  	flush_work(&cp->cp_down_w);
>  
> -	/* tear down queued messages */
> -	list_for_each_entry_safe(rm, rtmp,
> -				 &cp->cp_send_queue,
> -				 m_conn_item) {
> +	/* Tear down queued messages.  Take the queue under cp_lock:
> +	 * a sender that still holds a reference can be inside
> +	 * rds_send_queue_rm() right now, and it tests
> +	 * rds_destroy_pending() under the same lock, so after this
> +	 * splice nothing is added behind our back.
> +	 */
> +	spin_lock_irqsave(&cp->cp_lock, flags);
> +	list_splice_init(&cp->cp_send_queue, &purge);
> +	spin_unlock_irqrestore(&cp->cp_lock, flags);
> +	list_for_each_entry_safe(rm, rtmp, &purge, m_conn_item) {
>  		list_del_init(&rm->m_conn_item);
>  		BUG_ON(!list_empty(&rm->m_sock_item));
>  		rds_message_put(rm);

[Severity: High]
The commit message says "A sender that gets there first has its message
purged".  Can that message actually be purged, given the retained
BUG_ON(!list_empty(&rm->m_sock_item))?

rds_send_queue_rm() links a sendmsg message onto both queues in the same
locked region:

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);
	...
	list_add_tail(&rm->m_conn_item, &cp->cp_send_queue);
	set_bit(RDS_MSG_ON_CONN, &rm->m_flags);
	...
}

So a message on cp_send_queue always has a non-empty m_sock_item, and the
sender that "gets there first" hits BUG() rather than a clean purge.

The window exists because the flag the new test reads is published under a
different lock:

net/rds/connection.c:rds_conn_destroy() {
	spin_lock_irq(&rds_conn_lock);
	if (conn->c_destroy_in_prog) {
		spin_unlock_irq(&rds_conn_lock);
		return;
	}
	WRITE_ONCE(conn->c_destroy_in_prog, true);
	...
}

CPU0 (sendmsg)                        CPU1 (destroy)
rds_send_queue_rm()
  spin_lock(&cp->cp_lock)
  rds_destroy_pending() == false
  list_add_tail(m_sock_item, ...)
  list_add_tail(m_conn_item, ...)
  spin_unlock(&cp->cp_lock)
                                      WRITE_ONCE(c_destroy_in_prog, true)
                                      rds_conn_path_quiesce()
                                        splice cp_send_queue
                                        BUG_ON(!list_empty(m_sock_item))

Is there also a non-racy path into the same assertion?  rds_conn_shutdown()
runs before the purge and rds_send_path_reset() puts retransmit messages
back on cp_send_queue:

net/rds/send.c:rds_send_path_reset() {
	...
	list_splice_init(&cp->cp_retrans, &cp->cp_send_queue);
	spin_unlock_irqrestore(&cp->cp_lock, flags);
}

Those messages are still on their socket's send queue, so destroying a
connection while a socket has unacked messages would reach the BUG_ON with
no timing involved at all.

[Severity: High]
The purge unlinks m_conn_item and drops the conn-side reference, but leaves
RDS_MSG_ON_CONN set and does not touch the socket side.  Can that let a
concurrent remover work on the private purge list?

RDS_MSG_ON_CONN is the token other conn-side removers use:

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

rds_send_drop_to() is reachable from rds_release() and from
setsockopt(RDS_CANCEL_SENT_TO).  If it runs while the quiesce is walking
'purge' with cp_lock dropped, it unlinks an entry from that stack-local
list; if it runs after the purge, test_and_clear_bit() still succeeds and
rds_message_put() drops the conn-queue reference a second time.  Since
rds_send_queue_rm() takes one reference per membership, does that underflow
the rds_message refcount and free the message while rs_send_queue and the
sender still point at it?  rds_send_probe() messages, which have an empty
m_sock_item and survive the assertion above, look like the easiest case to
hit.

The established protocol clears the flag under the lock before releasing it:

net/rds/send.c:rds_send_path_drop_acked() {
	...
	list_move(&rm->m_conn_item, &list);
	clear_bit(RDS_MSG_ON_CONN, &rm->m_flags);
	...
	if (!list_empty(&list))
		smp_mb__after_atomic();
	spin_unlock_irqrestore(&cp->cp_lock, flags);
}

Should the splice clear RDS_MSG_ON_CONN for each message under cp_lock, and
retire the socket side (m_sock_item, rs_snd_bytes) through
rds_send_remove_from_sock()?  As written the purge never calls
rds_send_sndbuf_remove(), so rs_snd_bytes stays charged for the purged
messages.

> diff --git a/net/rds/send.c b/net/rds/send.c
> index 2d7839438abdc..f7bc4c5446d61 100644
> --- a/net/rds/send.c
> +++ b/net/rds/send.c
> @@ -928,6 +928,19 @@ static int rds_send_queue_rm(struct rds_sock *rs, struct rds_connection *conn,
>  	 * and poll() now knows no more data can be sent.
>  	 */
>  	if (rs->rs_snd_bytes < rds_sk_sndbuf(rs)) {
> +		/* rds_conn_path_quiesce() empties cp_send_queue under
> +		 * cp_lock once the connection's destroy has begun.  Test
> +		 * for that under the same lock, before touching either
> +		 * queue: a message added after the purge would hold a
> +		 * connection reference nothing ever drops.
> +		 */
> +		spin_lock(&cp->cp_lock);
> +		if (rds_destroy_pending(conn)) {
> +			spin_unlock(&cp->cp_lock);
> +			*queued = -EAGAIN;
> +			goto unlock;
> +		}
> +
>  		rs->rs_snd_bytes += len;

[Severity: High]
The refusal sits inside the "if (rs->rs_snd_bytes < rds_sk_sndbuf(rs))"
branch.  When the send buffer is already full, does *queued ever go
negative?

For a full sndbuf rds_send_queue_rm() skips the whole branch and returns the
unchanged *queued == 0, so rds_sendmsg() keeps sleeping:

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

With the default SO_SNDTIMEO, timeo is MAX_SCHEDULE_TIMEOUT, so the loop
re-sleeps and the new "if (queued < 0)" exit is never reached.

Can the send room ever appear afterwards?  Credit is returned by
rds_send_remove_from_sock() on ack processing, which cannot happen on a
quiesced connection, or by rds_send_drop_to() on close/cancel, which the
blocked task itself is holding the socket against.  The purge in
rds_conn_path_quiesce() touches neither rs_snd_bytes nor sk_sleep(), so
there is no wake-up either.

The commit message states:

  "rds_send_queue_rm()'s *queued becomes negative on refusal so that the
  wait loop in rds_sendmsg() stops waiting for send room that will never
  come."

Is that true for the full-sndbuf case, which is the only case that reaches
the wait loop?  And since rds_sendmsg() holds rds_conn_get(conn) for the
whole call, does a sender parked this way keep t_conn_count non-zero and
leave rds_conn_wait_conns_freed() looping on transport unload?  Would moving
the rds_destroy_pending() test above the sndbuf-room check, and returning
the credit plus a wake-up from the purge, address both?

> @@ -951,7 +964,6 @@ static int rds_send_queue_rm(struct rds_sock *rs, struct rds_connection *conn,
>  		rm->m_inc.i_conn_path = cp;
>  		rds_message_addref(rm);
>  
> -		spin_lock(&cp->cp_lock);
>  		rm->m_inc.i_hdr.h_sequence = cpu_to_be64(cp->cp_next_tx_seq++);
>  		list_add_tail(&rm->m_conn_item, &cp->cp_send_queue);
>  		set_bit(RDS_MSG_ON_CONN, &rm->m_flags);

[Severity: Low]
This isn't a bug, but the comment a few lines above now describes something
the function no longer does:

		/* 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 is now taken at the top of the branch, so it is held across the
socket-queue insertion, sock_hold(), the header population, the
rds_message_addref() and the sequence bump.  Could the comment be updated or
dropped?

> @@ -964,6 +976,7 @@ static int rds_send_queue_rm(struct rds_sock *rs, struct rds_connection *conn,
>  		*queued = 1;
>  	}
>  
> +unlock:
>  	spin_unlock_irqrestore(&rs->rs_lock, flags);
>  out:
>  	return *queued;
> @@ -1485,6 +1498,11 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
>  			ret = -ETIMEDOUT;
>  		goto out;
>  	}
> +	/* rds_send_queue_rm() refused: the connection is being destroyed */
> +	if (queued < 0) {
> +		ret = queued;
> +		goto out;
> +	}
>  
>  	/*
>  	 * By now we've committed to the send.  We reuse rds_send_worker()

[ ... ]

> @@ -1567,6 +1585,14 @@ rds_send_probe(struct rds_conn_path *cp, __be16 sport,
>  		goto out;
>  
>  	spin_lock_irqsave(&cp->cp_lock, flags);
> +	/* Same rule as rds_send_queue_rm(): once the destroy has purged
> +	 * cp_send_queue under this lock, nothing may be added behind it.
> +	 */
> +	if (rds_destroy_pending(cp->cp_conn)) {
> +		spin_unlock_irqrestore(&cp->cp_lock, flags);
> +		ret = -EAGAIN;
> +		goto out;
> +	}
>  	list_add_tail(&rm->m_conn_item, &cp->cp_send_queue);
>  	set_bit(RDS_MSG_ON_CONN, &rm->m_flags);
>  	rds_message_addref(rm);

[Severity: Low]
One question about the series ordering rather than this hunk.  The
serialization added here is what makes the quiesce safe against senders that
are allowed to be in flight by the earlier commits "net/rds: split
connection destroy into quiesce and kref-governed free" and "net/rds: hold
connection references in lookup, sockets and c_passive".

At those intermediate commits the unlocked cp_send_queue walk in
rds_conn_path_quiesce() already races the locked add in
rds_send_queue_rm(), so a bisect or a partial backport lands on a tree with
the list corruption and pinned-connection behaviour described here.

Would folding this change into the prerequisite commit, or adding a Fixes:
tag naming it, keep the series bisectable?

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