* Re: [RFC] tcp md5 use of alloc_percpu
[not found] ` <1414005158.9031.22.camel@edumazet-glaptop2.roam.corp.google.com>
@ 2014-10-22 23:05 ` Crestez Dan Leonard
2014-10-24 9:33 ` Herbert Xu
0 siblings, 1 reply; 2+ messages in thread
From: Crestez Dan Leonard @ 2014-10-22 23:05 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, linux-crypto
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
^ permalink raw reply [flat|nested] 2+ messages in thread