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

[netdev-bot+sashiko@kernel.org]

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

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

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

- [Medium] After the new check in rds_tcp_data_recv() fires, the receive
  path stops consuming data altogether, and this breaks the drain loop…

Pre-existing issues:
- [High] The patch does not cover the RDS IB/RDMA receive path, which has
  the same flaw it fixes for TCP: the peer-announced h_len is never…

--

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

This patch adds a check to rds_tcp_data_recv(). Once the header is
complete, the h_len announced by the peer is compared against
RDS_MAX_MSG_SIZE. If it is larger, the connection path is dropped and the
receive actor returns with -EPROTO.

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

[Severity: Medium]
After this check fires, tc->t_tinc is still set, with t_tinc_hdr_rem == 0
and t_tinc_data_rem > RDS_MAX_MSG_SIZE. Won't every later call into
rds_tcp_data_recv() hit the same check straight away and return 0 without
consuming anything?

__tcp_read_sock() stops on a 0 return:

    used = recv_actor(desc, skb, offset, len);
    if (used <= 0) {
        if (!copied)
            copied = used;
        break;
    }

So any payload after the oversized header stays on sk_receive_queue.

The drain loop in rds_tcp_conn_path_shutdown() only exits early when that
queue is empty:

    rds_tcp_recv_path(cp);
    } while (!wait_event_timeout(tc->t_recv_done_waitq,
            (sk->sk_state == TCP_CLOSING ||
             ...
             sk->sk_state == TCP_CLOSE) &&
            skb_queue_empty_lockless(&sk->sk_receive_queue),
            msecs_to_jiffies(100)) &&
         ++rounds < 50);

tc->t_tinc is only reset after this loop. Does that mean the loop now
always runs the full 50 x 100ms (about 5 seconds), even when the peer
answers our FIN promptly?

Each round goes through this chain and calls rds_conn_path_drop() again:

  rds_tcp_recv_path()->rds_tcp_read_sock()->tcp_read_sock()->rds_tcp_data_recv()

That call does atomic_set(&cp->cp_state, RDS_CONN_ERROR) while the path is
in RDS_CONN_DISCONNECTING, and it re-queues cp_down_w. The
DISCONNECTING->DOWN transition in rds_conn_shutdown() then fails and
returns early. Is a second full shutdown pass forced before the reconnect?

This holds up the path's ordered workqueue, or the shared rds_wq if
alloc_ordered_workqueue() failed, and it delays reconnect. It would also
apply to every reconnect cycle of the old-kernel peers that the changelog
says will now be dropped.

Would it work better to consume and discard the rest of the stream once
the protocol error is detected, by returning the full len without cloning?
The drain loop could then finish, and the drop would not be repeated.

[Severity: High]
This isn't a bug introduced by this patch, but the IB transport appears to
have the same unbounded h_len problem. In net/rds/ib_recv.c,
rds_ib_process_recv() does:

    memcpy(hdr, ihdr, sizeof(*hdr));
    ic->i_recv_data_rem = be32_to_cpu(hdr->h_len);

There is no RDS_MAX_MSG_SIZE check. Each fragment is then kept until
i_recv_data_rem reaches 0:

    list_add_tail(&recv->r_frag->f_item, &ibinc->ii_frags);

Continuation fragments are only checked for a matching header and a
minimum length. rds_ib_refill_one_frag() allocates a replacement frag for
each one consumed, with no limit.

The rds_ib_sysctl_max_recv_allocation cap in rds_ib_refill_one_inc()
counts rds_ib_incoming structures. A single huge message uses only one of
those.

Can an IB/RoCE peer that sends h_len = 0xffffffff, followed by full-sized
fragments, make the receiver hold up to about 4 GiB per connection?

rds.h describes RDS_MAX_MSG_SIZE as the limit for "both RDMA and non-RDMA
RDS message". Should rds_ib_process_recv() get an equivalent check,
followed by rds_ib_conn_error()?

Since this is tagged for stable, would it also help for the changelog to
say that only the TCP receive path is bounded?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-h047-send-v1-1-2727a38dbfcd%40illinois.edu