Re: [PATCH net-next v3 09/13] net/rds: refuse to queue a message 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: 1 · High: 2 · Medium: 1 · Low: 1

- [Critical] rds_conn_path_quiesce() keeps
  BUG_ON(!list_empty(&rm->m_sock_item)) (net/rds/connection.c:633) while…
- [High] The new comment in rds_conn_path_quiesce()
  (net/rds/connection.c:622-627) claims that after the cp_lock-protected…
- [High] The rds_destroy_pending() refusal is nested inside `if
  (rs->rs_snd_bytes < rds_sk_sndbuf(rs))` (net/rds/send.c:932, check at…
- [Medium] The changelog ("a message added after the purge sits on the
  queue of a quiesced connection holding the connection reference…
- [Low] This patch repairs a regression created by the immediately
  preceding patch of the same series, "net/rds: hold connection…

--

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

This splices cp_send_queue away under cp_lock in rds_conn_path_quiesce()
rather than walking it unlocked, and makes rds_send_queue_rm() test
rds_destroy_pending() under cp_lock before it queues a message.  On
refusal *queued is set to -EAGAIN and rds_sendmsg() returns that to the
caller.

No Link: tags are present in the commit header.

A few questions on this one.

[Severity: Low]

> rds_conn_path_quiesce() tears down cp_send_queue by walking it with no
> lock held.  That was tolerable while a connection could only be
> destroyed with no sender in flight, but a sender now holds a
> reference across rds_sendmsg(), and rds_conn_destroy() can quiesce
> the connection underneath it.

The "now" here refers to the immediately preceding patch of this series,
"net/rds: hold connection references in lookup, sockets and c_passive",
which adds the rds_conn_get() that keeps the connection alive across
rds_sendmsg().

If that is right, then applying the series up to that commit leaves
rds_conn_path_quiesce() walking cp_send_queue unlocked while a referenced
sender can be adding to it under cp_lock, and a bisect landing there sees
the list corruption described above.

Would it make sense to fold this change into that patch, or to carry a
Fixes: tag or an explicit dependency note?

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index 7ef6fb9d352b..e5a8534c23cf 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_shutdown(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);

[Severity: High]

Is "nothing is added behind our back" true for the other producer of
cp_send_queue?  rds_send_probe() takes cp_lock and adds unconditionally,
with no rds_destroy_pending() test before the add:

net/rds/send.c:rds_send_probe() {
	...
	spin_lock_irqsave(&cp->cp_lock, flags);
	list_add_tail(&rm->m_conn_item, &cp->cp_send_queue);
	set_bit(RDS_MSG_ON_CONN, &rm->m_flags);
	rds_message_addref(rm);
	rm->m_inc.i_conn = cp->cp_conn;
	rm->m_inc.i_conn_path = cp;
	...
	spin_unlock_irqrestore(&cp->cp_lock, flags);

	/* schedule the send work on cp_wq */
	rcu_read_lock();
	if (!rds_destroy_pending(cp->cp_conn))
		queue_delayed_work(cp->cp_wq, &cp->cp_send_w, 1);
	rcu_read_unlock();
	...
}

The destroy test there runs after the message is already linked, and only
suppresses the work that could have drained it.

This is reachable from the syscall being hardened here.  rds_sendmsg()
calls rds_send_ping() -> rds_send_probe() for an mp-capable connection
whose path 0 is down, before its own rds_destroy_pending() check:

net/rds/send.c:rds_sendmsg() {
	...
	if (conn->c_trans->t_mp_capable &&
	    !rds_conn_path_up(&conn->c_path[0])) {
		...
		rds_send_ping(conn, 0);
	}
	...
	if (rds_destroy_pending(conn)) {
		ret = -EAGAIN;
		goto out;
	}

rds_send_pong() reaches the same code from the receive path.

Since the quiesce splices once and never purges again, can a probe that
lands after the splice sit on cp_send_queue forever with no worker to
drain it?  That would leak the rds_message whose reference the queue
holds, and leave rm->m_conn_item and rm->m_inc.i_conn_path pointing into
the c_path array that rds_conn_destroy_fini() kfree()s.

At the end of this series rds_send_probe() also takes rds_conn_get(), so
would the same window then leak a connection reference and leave
rds_conn_wait_conns_freed() waiting for a count that never drops, blocking
transport unload?

> +	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));
              ^^^^^^

[Severity: Critical]

Can this BUG_ON() fire in exactly the interleaving the changelog says is
now handled ("A sender that gets there first has its message purged")?

Every message rds_send_queue_rm() puts on cp_send_queue is linked on the
socket send queue in the same critical section:

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

m_sock_item is emptied only by the ack path via
rds_send_remove_from_sock(), or by rds_send_drop_to() / rds_release().
Nothing in the destroy path does it.  rds_conn_path_quiesce() itself runs
rds_conn_path_drop(cp, true) and flush_work(&cp->cp_down_w) first, and
rds_conn_shutdown() -> rds_conn_path_reset() -> rds_send_path_reset()
puts still socket-linked messages back on cp_send_queue:

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

So an unacked message reaches this loop with a non-empty m_sock_item and
panics the kernel.

Independent of the panic, the loop drops the conn-side reference with
list_del_init(&rm->m_conn_item) + rds_message_put(), but never clears
RDS_MSG_ON_CONN and never undoes the socket-side state (the rs_snd_bytes
charge, RDS_MSG_ON_SOCK, the extra rds_message_addref() and the
sock_hold()).

Because the flag stays set, does a later rds_send_drop_to() still pass
here and issue a second put for the reference already released?

net/rds/send.c:rds_send_drop_to() {
	...
		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);
	...
}

That is a refcount_t underflow on rm->m_refcount and a premature free.
And since that path holds cp_lock while the quiesce walks its private
purge list with no lock at all, it can also list_del_init() an entry
living on the purge list, so does the splice really establish the
exclusive ownership the new comment claims?

Should the purge clear RDS_MSG_ON_CONN under cp_lock and detach the
message from its socket queue the way rds_send_drop_to() and
rds_send_remove_from_sock() do, instead of asserting m_sock_item is empty?

>  		rds_message_put(rm);
>  	}

> diff --git a/net/rds/send.c b/net/rds/send.c
> index 1ae1f24c24e8..94d6ac174dde 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.
> +		 */

[Severity: Medium]

Does the connection reference this comment describes exist yet?  The same
claim is in the changelog:

> 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.

At this commit rds_send_queue_rm() takes only a message reference:

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

and rds_message_put() goes rds_message_purge() -> rds_message_free()
without touching rm->m_inc.i_conn, so nothing here pins the connection.
The rds_conn_get() with the "put in rds_message_put()" annotation only
appears with the last patch of the series, "net/rds: hold a connection
reference from struct rds_incoming".

The damage from a post-purge add in this tree state looks like a leaked
rds_message plus a list linkage dangling into the c_path array that
rds_conn_destroy_fini() kfree()s, not a connection that can never be
freed.

Could the changelog and the comment be adjusted for the tree state they
land in, or this patch ordered after the incoming-reference patch?

> +		spin_lock(&cp->cp_lock);
> +		if (rds_destroy_pending(conn)) {
> +			spin_unlock(&cp->cp_lock);
> +			*queued = -EAGAIN;
> +			goto unlock;
> +		}
> +

[Severity: High]

The refusal is nested inside if (rs->rs_snd_bytes < rds_sk_sndbuf(rs)),
so it only fires when send room exists.  Does that leave the blocked
sender, the one the changelog is about, still waiting forever?

With the send buffer full, rds_send_queue_rm() falls through and returns
0, and rds_sendmsg() re-arms the wait with timeo == MAX_SCHEDULE_TIMEOUT
on a blocking socket:

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

rs_snd_bytes is decremented only by rds_send_sndbuf_remove() from the ack
path via rds_send_remove_from_sock(), or from rds_send_drop_to().  After
the connection is quiesced no acks arrive, and the purge in
rds_conn_path_quiesce() calls neither rds_send_sndbuf_remove() nor
rds_wake_sk_sleep() - it only drops the conn-side message reference.

So the wake condition can never become non-zero, and the new
if (queued < 0) exit below is never evaluated.  Should the destroy test
move outside the sndbuf-room branch, and should the purge release the
sndbuf accounting and wake sk_sleep()?

>  		rs->rs_snd_bytes += len;
>  
>  		/* let recv side know we are close to send space exhaustion.
> @@ -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);

[ ... ]

> @@ -1474,6 +1487,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;
> +	}

Thanks for looking at these.

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