Re: [rds-devel] [External] : Re: [PATCH net] RDS/IB: validate receive completion payload length

[Gerd Rausch]

Hi Allison,

> On Oct 17, 2026, at 22:38, Allison Henderson <achender@xxxxxxxxxx> wrote:
> On Fri, 2026-10-16 at 13:18 -0700, Gerd Rausch wrote:
>> 
>> On 2026-10-06 18:13, Allison Henderson via rds-devel wrote:
>>>> --- a/net/rds/ib_recv.c
>>>> +++ b/net/rds/ib_recv.c
>>>> @@ -949,6 +949,15 @@ static void rds_ib_process_recv(struct rds_connection *conn,
>>>> }
>>>> }
>>>> 
>>>> + if (data_len != min_t(u32, ic->i_recv_data_rem, RDS_FRAG_SIZE)) {
>>>> + rds_ib_conn_error(conn,
>>>> +  "incoming fragment payload length %u, expected %u; "
>>>> +  "disconnecting and reconnecting\n",
>>>> +  data_len,
>>>> +  min_t(u32, ic->i_recv_data_rem, RDS_FRAG_SIZE));
>>>> + goto done;
>>>> + }
>>>> +
>>>> list_add_tail(&recv->r_frag->f_item, &ibinc->ii_frags);
>>>> recv->r_frag = NULL;
>>>> 
>> How does forcing a reconnect help this situation?
>> 

[...]

> 
> On the retransmit loop: cp_retrans only matters if the sender is our
> own RDS stack, and our stack can't produce the fragment this check
> rejects.

Correct. But this check was presumably added for a reason.

1) An incorrect implementation of RDS (other than the one we're looking at)
2) A bad actor, intentionally crafting such messages.

>> Therefore, apart from security concerns, the previous behavior of garbage-in / garbage-out
>> is IMO better than this behavior of just endlessly forcing a reconnect.
> 
> The trouble is that the garbage going out wasn't the sender's.  The
> receiver delivered h_len bytes of a page that only had data_len bytes
> written into it, so the tail was whatever the page held before: an
> earlier message to another socket (the report demonstrated this across
> UIDs), or unzeroed memory on a fresh page.  That's the bug. And
> refusing to deliver it is the whole fix.

Wait. This patch was created for a hypothetical case, no?
Based on analysis of "Assisted-by: LLM".

This patch does *not* refuse to deliver the bogus message.
That would have been a fine choice.

This patch just drops the underlying connection, in the hopes
that when this message gets sent again, it'll somehow be different.

What would make it different?
Some unknown and non-deterministic sender?

> 
> Dropping the connection is also what rds_ib_process_recv() has always
> done for the neighbouring case, a fragment whose header doesn't match
> the message in progress.  That check has been there since 1e23b3ee0e94
> and has the same "a misbehaving peer can keep us reconnecting"
> property.  This patch doesn't add a new failure mode, it just closes
> the one way a malformed fragment could still get through.

How much benefit a 16bit IPv4 checksum calculation adds on top of
the underlying InifiniBand CRC-32 protection is debatable.

But let's just say somehow (I don't know how) the CRC-32 check missed
a header corruption that ip_fast_csum() catches:
You're correct, the connection would be dropped.
So the assumption here is that this is a transport error,
that fixes itself with a retransmit, and not that it's a bogus
or malicious sender.

Does this patch also assume we're looking at a transport
error, that the CRC-32 didn't catch, and consider that more likely
than a bad or malicious sender?

I suggest that just dropping this message, not tearing down the connection,
would have been a much better choice here.

>> 
> 
>> If there's a strong desire to address this issue, the sender should be
>> told to discard the message. A reconnect doesn't address this issue.
>> 
> I don't think there's a way to say that today.  The RDS header flags are
> congestion-bitmap, ack-required, retransmitted and the extension-header
> bit.  The only error a receiver can signal to its peer is to drop the
> connection.
> 

Not exactly true.
There's already code for the RDMA_CM_EVENT_REJECTED case to downgrade
protocol version.

The RDS code can be changed. The RDS protocol can be extended.
A receiver could be made to tell a sender to drop a message,
if we'd consider that beneficial.

But in this case, since the message itself violates the "unwritten rules"
of the RDS protocol, I'd say just dropping the message would be been
a fine choice.

Dropping the connection however, doesn't do anything.
It just asks the sender to send the same bogus message again.

> A per-message reject would be a protocol extension that both ends have to
> implement.  But that would be net-next material, and it couldn't help
> against the only peers that actually send these.  I'd be happy to review it
> if someone wants to take it on, but I don't think it should hold up a stable
> fix for a data disclosure.
> 

Again, we don't need to do that.
Dropping the message would have been a fine choice here.

Thanks,

  Gerd