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

[David Howells]

David Laight <david.laight.linux@xxxxxxxxx> wrote:

> > Note also that I would generally prefer to replace the buffers of the
> > current sk_buff with a new kmalloc'd buffer of the right size, ditching the
> > old data and frags as this makes the handling of MSG_PEEK easier and
> > removes the double-decryption issue, but this looks like quite a
> > complicated thing to achieve.  skb_morph() looks half way to what I want,
> > but I don't want to have to allocate a new sk_buff.
> 
> Wouldn't you need to do that anyway when the kkb is shared - or can't
> that happen?

Hmmm...  That may well be the case - but if it's shared, do I own the
->next/prev pointers and the ->cb area?

> > +	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 */
> 
> Is it actually worth making those short rather than int?
> I doubt the extra 4 bytes will matter and the generated code might be better.
> (IIRC 32bit arm has a limited offset from 16 bit load/store, dunno about 64bit)

Well, the capacity of a UDP packet less the rxrpc header can't reach 65535, so
on that basis this should be fine.  I'm a little worried about the rxrpc_call
struct's size - it's already ~1.3K.  It's already got a lot of 8- and 16-bit
fields in it.  Of course, it's nowhere near as bit-for-bit optimised as
sk_buff, but I guess there are a lot more of those in a system.

> > +	if (call->rx_dec_bsize < sp->len) {
> 
> IMHO That test is backwards; the 'more constant' value should be on the right.

Actually, the thing you're testing should be on the left and the thing you're
testing against on the right - but, yes, I should switch them around.

> > +		size_t size = umin(round_up(sp->len, 32), 2048);
> 
> Doesn't min() work?

Actually, it should be umax() as I want the largest of the values (as Jeff
pointed out).  I prefer using umin/umax for values that are known to be
unsigned as you don't get casting errors (see the number of places we end up
using min/max_t(<unsigned-type>, ...) when we should use umin/umax() instead)
and the compiler may generate better code as we've told it that it doesn't
have to worry about negatives.

> That doesn't look right.
> If sp->len is bigger than 2048 the you keep allocating a new buffer
> and the call below overruns the allocated buffer.

Yep - see the aforementioned umax comment.

David