Re: [RFC] tcp md5 use of alloc_percpu

Re: [RFC] tcp md5 use of alloc_percpu

[Crestez Dan Leonard]

On 10/22/2014 10:12 PM, Eric Dumazet wrote:
> On Wed, 2014-10-22 at 21:55 +0300, Crestez Dan Leonard wrote:
>> Hello,
>>
>> It seems that the TCP MD5 feature allocates a percpu struct
>> tcp_md5sig_pool and uses part of that memory for a scratch buffer to
>> do crypto on. Here is the relevant code:
>>
>> static int tcp_v4_md5_hash_pseudoheader(struct tcp_md5sig_pool *hp,
>>                                          __be32 daddr, __be32 saddr,
>> int nbytes)
>> {
>>          struct tcp4_pseudohdr *bp;
>>          struct scatterlist sg;
>>
>>          bp = &hp->md5_blk.ip4;
>>
>>          /*
>>           * 1. the TCP pseudo-header (in the order: source IP address,
>>           * destination IP address, zero-padded protocol number, and
>>           * segment length)
>>           */
>>          bp->saddr = saddr;
>>          bp->daddr = daddr;
>>          bp->pad = 0;
>>          bp->protocol = IPPROTO_TCP;
>>          bp->len = cpu_to_be16(nbytes);
>>
>>          sg_init_one(&sg, bp, sizeof(*bp));
>>          return crypto_hash_update(&hp->md5_desc, &sg, sizeof(*bp));
>> }
>>
>> sg_init_one does virt_addr on the pointer which assumes it is directly
>> accessible. But the tcp_md5sig_pool pointer comes from alloc_percpu
>> which can return memory from the vmalloc area after the
>> pcpu_first_chunk is exhausted. This looks wrong to me. I'm am getting
>> crashes on mips and I believe this to be the cause.
>>
>> Allocating a scratch buffer this way is very peculiar. The
>> tcp4_pseudohdr struct is only 12 bytes in length. Similar code in
>> tcp_v6_md5_hash_pseudoheader uses a 40 byte tcp6_pseudohdr. I think it
>> is perfectly reasonable to allocate this kind of stuff on the stack,
>> right? These pseudohdr structs are not used at all outside these two
>> static functions and it would simplify the code.
>>
> Yep, but the sg stuff does not allow for stack variables. Because of
> possible offloading and DMA, I dont know...
A stack buffer is used in tcp_md5_hash_header to add a tcphdr to the 
hash. A quick grep for sg_init_one find a couple of additional instances 
of what looks like doing crypto on small stack buffers:

net/bluetooth/smp.e:110
net/sunrpc/auth_gss/gss_krb5_crypto.c:194
net/rxrpc/rxkad.c:multiple

But those might also be bugs.

If the buffers passed to the crypto api need to be DMA-ble then wouldn't 
this also exclude DEFINE_PERCPU? The DMA-API-HOWTO mentions that items 
in data/text/bss might not be DMA-able, presumably depending on the 
architecture.

>> I'm not familiar with the linux crypto API. Isn't there an easier way
>> to get a temporary md5 hasher?
>
> You should CC crypto guys maybe ...
Added linux-crypto in CC. To summarize the question: What kind of memory 
can be passed to crypto api functions like crypto_hash_update?

 >> The whole notion of struct tcp_md5sig_pool seems dubious. This is a
 >> very tiny struct already and after removing the pseudohdr it shrinks
 >> to a percpu hash_desc for md5 (8 or 16 bytes). Wouldn't DEFINE_PERCPU
 >> be more appropriate?
 >
 > Sure. this would be the more appropriate fix IMO.
I'll post this as a patch if somebody can confirm that it is correct and 
portable.

Doing a temp kmalloc/kfree would also work, but it would hurt 
performance. It would be nice to have a generic way to ask for a small 
temporary DMA-ble buffer.

If DEFINE_PERCPU is not suitable then the tcp_md5sig_pool structs should 
be allocated via individual kmallocs for each cpu.

Regards,
Leonard

Re: [RFC] tcp md5 use of alloc_percpu

[Herbert Xu]

Crestez Dan Leonard <cdleonard@gmail.com> wrote:
>
>> Yep, but the sg stuff does not allow for stack variables. Because of
>> possible offloading and DMA, I dont know...
> A stack buffer is used in tcp_md5_hash_header to add a tcphdr to the 
> hash. A quick grep for sg_init_one find a couple of additional instances 
> of what looks like doing crypto on small stack buffers:

First of all crypto_hash_update is obsolete, don't use it in any
new code.  Thanks for reminding me to get rid of existing users.

You should either use crypto_shash_update for small data, e.g., headers
or crypto_ahash_update for large data such as whole packets.

If you use shash then you may allocate your buffer on the stack.  With
ahash stack memory is not allowed.

I hope this clears things up for you.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt