Message ID | 5447FDB2.2010906@gmail.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
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. Then just remove the alloc_percpu() from __tcp_alloc_md5sig_pool() and make this a static per cpu definition (not a dynamic allocation) > > 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... > 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. > Before commit 71cea17ed39fdf1c0634f530ddc6a2c2fc601c2b the struct > tcp_md5sig_pool structs were freed when all users were gone, but that > functionality seems to have been dropped. > > 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 ... > > Here's what I mean by allocating tcp{4,6}_pseudohdr on the stack: Your patch is quite invasive, you should so something simpler to ease backports. Thanks -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/22/14, 3: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. I can confirm this created an issue on our powerpc based switches. My solution in our 3.2 kernel was to allocate the buffer on the stack. I like this solution better. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Crestez Dan Leonard <cdleonard@gmail.com> Date: Wed, 22 Oct 2014 21:55:46 +0300 > static int tcp_v4_md5_hash_pseudoheader(struct tcp_md5sig_pool *hp, > __be32 daddr, __be32 saddr, int nbytes) > { > - struct tcp4_pseudohdr *bp; > + struct tcp4_pseudohdr bp; ... > + sg_init_one(&sg, &bp, sizeof(bp)); > + return crypto_hash_update(&hp->md5_desc, &sg, sizeof(bp)); As others have mentioned, you cannot do this. On some architectures the kernel stack comes from vmalloc() memory too. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
diff --git a/include/net/tcp.h b/include/net/tcp.h index 4062b4f..beabd7b 100644 --- a/include/net/tcp.h +++ b/include/net/tcp.h @@ -1266,33 +1266,9 @@ struct tcp_md5sig_info { struct rcu_head rcu; }; -/* - pseudo header */ -struct tcp4_pseudohdr { - __be32 saddr; - __be32 daddr; - __u8 pad; - __u8 protocol; - __be16 len; -}; - -struct tcp6_pseudohdr { - struct in6_addr saddr; - struct in6_addr daddr; - __be32 len; - __be32 protocol; /* including padding */ -}; - -union tcp_md5sum_block { - struct tcp4_pseudohdr ip4; -#if IS_ENABLED(CONFIG_IPV6) - struct tcp6_pseudohdr ip6; -#endif -}; - /* - pool: digest algorithm, hash description and scratch buffer */ struct tcp_md5sig_pool { struct hash_desc md5_desc; - union tcp_md5sum_block md5_blk; }; /* - functions */ diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c index 94d1a77..e716a67 100644 --- a/net/ipv4/tcp_ipv4.c +++ b/net/ipv4/tcp_ipv4.c @@ -1041,27 +1041,33 @@ static int tcp_v4_parse_md5_keys(struct sock *sk, char __user *optval, GFP_KERNEL); } +struct tcp4_pseudohdr { + __be32 saddr; + __be32 daddr; + __u8 pad; + __u8 protocol; + __be16 len; +}; + static int tcp_v4_md5_hash_pseudoheader(struct tcp_md5sig_pool *hp, __be32 daddr, __be32 saddr, int nbytes) { - struct tcp4_pseudohdr *bp; + 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)); + 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)); } static int tcp_v4_md5_hash_hdr(char *md5_hash, const struct tcp_md5sig_key *key, diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c index 8314955..87a9126 100644 --- a/net/ipv6/tcp_ipv6.c +++ b/net/ipv6/tcp_ipv6.c @@ -568,22 +568,28 @@ static int tcp_v6_parse_md5_keys(struct sock *sk, char __user *optval, AF_INET6, cmd.tcpm_key, cmd.tcpm_keylen, GFP_KERNEL); } +struct tcp6_pseudohdr { + struct in6_addr saddr; + struct in6_addr daddr; + __be32 len; + __be32 protocol; /* including padding */ +}; + static int tcp_v6_md5_hash_pseudoheader(struct tcp_md5sig_pool *hp, const struct in6_addr *daddr, const struct in6_addr *saddr, int nbytes) { - struct tcp6_pseudohdr *bp; + struct tcp6_pseudohdr bp; struct scatterlist sg; - bp = &hp->md5_blk.ip6; /* 1. TCP pseudo-header (RFC2460) */ - bp->saddr = *saddr; - bp->daddr = *daddr; - bp->protocol = cpu_to_be32(IPPROTO_TCP); - bp->len = cpu_to_be32(nbytes); + bp.saddr = *saddr; + bp.daddr = *daddr; + bp.protocol = cpu_to_be32(IPPROTO_TCP); + bp.len = cpu_to_be32(nbytes); - sg_init_one(&sg, bp, sizeof(*bp)); - return crypto_hash_update(&hp->md5_desc, &sg, sizeof(*bp)); + sg_init_one(&sg, &bp, sizeof(bp)); + return crypto_hash_update(&hp->md5_desc, &sg, sizeof(bp)); } static int tcp_v6_md5_hash_hdr(char *md5_hash, struct tcp_md5sig_key *key,