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

[David Howells <dhowells@redhat.com>]

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

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

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