Re: [PATCH net 2/3] rxrpc: Fix DATA decrypt vs splice() by copying data to buffer in recvmsg

[Jeffrey Altman <jaltman@auristor.com>]

[-- Attachment #1: Type: text/plain, Size: 4497 bytes --]



> On May 13, 2026, at 4:01 AM, David Howells <dhowells@redhat.com> wrote:
> 
> Jeffrey Altman <jaltman@auristor.com> wrote:
> 
>>> + void *rx_dec_buffer; /* Decryption buffer */
>>> + unsigned short rx_dec_bsize; /* rx_dec_buffer size */
>>> + unsigned short rx_dec_offset; /* Decrypted packet data offset */
>>> + unsigned short rx_dec_len; /* Decrypted packet data len */
>>> + rxrpc_seq_t rx_dec_seq; /* Packet in decryption buffer */
>>> 
>>> rxrpc_seq_t rx_highest_seq; /* Higest sequence number received */
>>> rxrpc_seq_t rx_consumed; /* Highest packet consumed */
>> 
>> 
>> Instead of allocating the storage within struct rxrpc_call perhaps
>> It would be better to add them to struct rxrpc_channel.  Doing so 
>> would reduce the allocation/deallocation churn.  The majority of
>> calls are short lived (perhaps a single packet in each direction)
>> but there will be many calls in rapid succession.
> 
> I'm trying to keep the I/O side separate from the application side.  I don't
> particularly want recvmsg (on the app side) reaching into the rxrpc_connection
> struct (on the I/O side).
> 
> Further, by only looking at the rxrpc_call struct, I don't have to deal with
> locking required for the possibility that the next call on that channel will
> start before I've finished with this one (say an incoming call is aborted and
> immediately followed up by the first packet of the next call).

There could only be one rxrpc_call structure at a time referring to the 
rxrpc_channel.  If rxrpc_input_packet_on_conn() receives a DATA packet for a
later call number and there is a prior rxrpc_call assigned to rxrpc_channel.call
then an RX_PACKET_TYPE_BUSY should be sent to the peer if the rx_call cannot be
terminated without blocking the processing of incoming packets.  The Rx BUSY
informs the initiating peer that the DATA packet has been dropped and that it
should be retransmitted (which will occur anyway due to the lack of an Rx ACK
packet.

The busy call channel logic is out of scope for this change and avoiding
per-call allocation churn is an optimization that can be implemented another
day if desired.

> 
>>> + size_t size = umin(round_up(sp->len, 32), 2048);
>> 
>> I think you meant to use max() here so that a minimum of 2048 bytes
>> is allocated.  
> 
> Yeah.
> 
>> I think applying a cap on the allocation size would also be 
>> beneficial.  IBM/Transarc derived Rx implementations have a hard
>> upper-bound of 21180 (15 x 1412) bytes plus one 28 byte rx header.
>> Applying a cap of 32KiB seems prudent.
> 
> This would need checking earlier in the input path.  A DATA packet that's too
> large would need to be rejected as it comes off of the UDP socket if we're not
> going to be able to unpack it later.

ok.

> 
>> It is also worth noting that there are no current implementations
>> of Rx RPC which will send individual Rx DATA packets larger than 
>> 1444 bytes including the Rx header.  Rx RESPONSE packets can be sent
>> as large as 16384 bytes (including the Rx header).  However, it is
>> extremely unlikely that this buffer once allocated would ever need 
>> to be grown.  
> 
> For Rx RESPONSE packets, I'm fine with allocating a buffer on the spur of the
> moment and freeing it immediately.  Ideally, there would only be one RESPONSE
> per connection anyway.  I could do a static buffer with a lock, I suppose, to
> make sure I can process the things under memory pressure-based writeback.

I agree.  The Rx RESPONSE packets belong to the rxrpc_connection and not to 
any particular call.  It is possible for there to be more than one Rx RESPONSE
packet received due to Rx CHALLENGE retransmission or because this peer decided
it can no longer trust the prior authentication and sends a new Rx CHALLENGE.
However, RESPONSE packets are few and far between.  There is no benefit to
adding another 16KB to the rxrpc_connection allocation.

> 
>>> + kfree(call->rx_dec_buffer);
>> 
>> It might be better to avoid deallocating the buffer on the error
>> path and permit it to be freed during normal call (or call channel)
>> deallocation.
> 
> Hmmm.  But I then need some other way to note that the buffer is no longer
> occupied by valid data.  I suppose I could set ->rx_dec_offset to USHRT_MAX.

That meaning was unclear and could warrant a code comment indicating that
is why the allocation is being freed.


> David
> 

Thanks.

Jeffrey Altman

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 4120 bytes --]