Re: [PATCH net] net/rds: tcp: bound the message length announced by the peer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] net/rds: tcp: rds_tcp_data_recv() drop stalls shutdown and
  clobbers cp_state
--

commit e71e7a51b12574a1429c193c4c2a910762c2f915
Author: Weidong Wang <weidongw@illinois.edu>

net/rds: tcp: bound the message length announced by the peer

This commit bounds the incoming message payload length in
rds_tcp_data_recv() to RDS_MAX_MSG_SIZE and drops the connection path
when a peer announces a larger header length.

> diff --git a/net/rds/tcp_recv.c b/net/rds/tcp_recv.c
> index ffe843ca219c7..29db73df84fa3 100644
> --- a/net/rds/tcp_recv.c
> +++ b/net/rds/tcp_recv.c
> @@ -213,6 +213,20 @@ static int rds_tcp_data_recv(read_descriptor_t *desc, struct sk_buff *skb,
>  			}
>  		}
>  
> +		/*
> +		 * The payload is queued on tinc until the whole message has
> +		 * arrived, so a peer must not announce more than a sender is
> +		 * allowed to send. Check on every pass, not only when the
> +		 * header completes: the drop is asynchronous and more data
> +		 * can arrive before the connection is shut down.
> +		 */
> +		if (!tc->t_tinc_hdr_rem &&
> +		    tc->t_tinc_data_rem > RDS_MAX_MSG_SIZE) {
> +			rds_conn_path_drop(cp, false);
> +			desc->error = -EPROTO;
> +			goto out;
> +		}
> +
>  		if (left && tc->t_tinc_data_rem) {
>  			to_copy = min(tc->t_tinc_data_rem, left);
>  

[Severity: Medium]
Does returning here without draining the socket or resetting
tc->t_tinc_data_rem stall rds_tcp_conn_path_shutdown() for 5 seconds and
clobber cp->cp_state when any payload bytes follow the oversized header?

When the header is followed by payload bytes, the payload remains in
sk->sk_receive_queue while cp->cp_down_w runs rds_conn_shutdown() ->
rds_tcp_conn_path_shutdown(). Before releasing the socket or resetting
tc->t_tinc_data_rem, rds_tcp_conn_path_shutdown() tries to drain the
receive queue:

net/rds/tcp_connect.c:rds_tcp_conn_path_shutdown() {
	...
		rounds = 0;
		do {
			rds_tcp_recv_path(cp);
		} while (!wait_event_timeout(tc->t_recv_done_waitq,
				(sk->sk_state == TCP_CLOSING ||
				 sk->sk_state == TCP_TIME_WAIT ||
				 sk->sk_state == TCP_CLOSE_WAIT ||
				 sk->sk_state == TCP_LAST_ACK ||
				 sk->sk_state == TCP_CLOSE) &&
				skb_queue_empty_lockless(&sk->sk_receive_queue),
				msecs_to_jiffies(100)) &&
			 ++rounds < 50);
	...
}

Because tc->t_tinc_data_rem is still greater than RDS_MAX_MSG_SIZE, each
rds_tcp_recv_path() call hits the new check and returns 0 without
consuming sk->sk_receive_queue, stalling cp->cp_wq for all 50 rounds
(5 seconds) before closing the socket. In addition, calling
rds_conn_path_drop() during this loop overwrites cp->cp_state from
RDS_CONN_DISCONNECTING back to RDS_CONN_ERROR and re-queues
cp->cp_down_w, causing rds_conn_shutdown() to fail its transition to
RDS_CONN_DOWN and run a redundant second shutdown pass.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-h047-send-v1-1-2727a38dbfcd@illinois.edu?part=1