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

[David Laight]

On Wed, 13 May 2026 09:01:14 +0100
David Howells <dhowells at redhat.com> wrote:

> Jeffrey Altman <jaltman at 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 are also loads of other allocates and frees (eg the skb itself).
One more isn't really going to be significant.
Especially for sub-page sizes that just come of a per-cpu list.

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

A 16K block of static data is rather a waste.
Under that much memory pressure something has to give.
Dropping a packet and forcing the remote to resend on timeout
may actually be the best thing to do.

-- David L

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