tcp md5: one more crypto-sg-on-the-stack instance

tcp md5: one more crypto-sg-on-the-stack instance

[Andy Lutomirski]

Hi all-

tcp_md5_hash_header does crypto using an sg that points to the stack.
This will break with virtually mapped stacks.  It also looks like it's
probably much slower than it deserves to be (it's trying to compute
the MD5 hash of a few tens of bytes -- going through a scatterlist is
a lot of overhead for an otherwise very fast operation).

I don't suppose one of you could fix it or at least advise as to how
it should be fixed.

Thanks,
Andy

Re: tcp md5: one more crypto-sg-on-the-stack instance

[Eric Dumazet]

On Fri, 2016-06-24 at 18:51 -0700, Andy Lutomirski wrote:
> Hi all-
> 
> tcp_md5_hash_header does crypto using an sg that points to the stack.
> This will break with virtually mapped stacks.  It also looks like it's
> probably much slower than it deserves to be (it's trying to compute
> the MD5 hash of a few tens of bytes -- going through a scatterlist is
> a lot of overhead for an otherwise very fast operation).

I guess nobody cares about TCP MD5 speed really.

> 
> I don't suppose one of you could fix it or at least advise as to how
> it should be fixed.

Simply extend tcp_md5sig_pool to contain a copy of the TCP headers ?

At most 40 bytes of extra per cpu storage is not a big problem.

Re: tcp md5: one more crypto-sg-on-the-stack instance

[Eric Dumazet]

On Sat, 2016-06-25 at 06:11 +0200, Eric Dumazet wrote:

> Simply extend tcp_md5sig_pool to contain a copy of the TCP headers ?
> 
> At most 40 bytes of extra per cpu storage is not a big problem.
> 

Correction : This is exactly 20 bytes for tcphdr, not 40.

Re: tcp md5: one more crypto-sg-on-the-stack instance

[Eric Dumazet]

On Sat, 2016-06-25 at 06:26 +0200, Eric Dumazet wrote:
> On Sat, 2016-06-25 at 06:11 +0200, Eric Dumazet wrote:
> 
> > Simply extend tcp_md5sig_pool to contain a copy of the TCP headers ?
> > 
> > At most 40 bytes of extra per cpu storage is not a big problem.
> > 
> 
> Correction : This is exactly 20 bytes for tcphdr, not 40.

Something like :

 include/net/tcp.h   |    2 +-
 net/ipv4/tcp.c      |   17 -----------------
 net/ipv4/tcp_ipv4.c |   31 +++++++++++++------------------
 net/ipv6/tcp_ipv6.c |   25 ++++++++++++++-----------
 4 files changed, 28 insertions(+), 47 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index a79894b667265cdf9e3fe793b4757e2f932b378a..2dd919e0289839130d2c5435b7925592082d62b5 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1385,6 +1385,7 @@ union tcp_md5sum_block {
 struct tcp_md5sig_pool {
 	struct ahash_request	*md5_req;
 	union tcp_md5sum_block	md5_blk;
+	struct tcphdr		tcp_hdr;
 };
 
 /* - functions */
@@ -1420,7 +1421,6 @@ static inline void tcp_put_md5sig_pool(void)
 	local_bh_enable();
 }
 
-int tcp_md5_hash_header(struct tcp_md5sig_pool *, const struct tcphdr *);
 int tcp_md5_hash_skb_data(struct tcp_md5sig_pool *, const struct sk_buff *,
 			  unsigned int header_len);
 int tcp_md5_hash_key(struct tcp_md5sig_pool *hp,
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 5c7ed147449c1b7ba029b12e033ad779a631460a..5fc9336934a11387e725300d6bca4aabd3991f19 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -3025,23 +3025,6 @@ struct tcp_md5sig_pool *tcp_get_md5sig_pool(void)
 }
 EXPORT_SYMBOL(tcp_get_md5sig_pool);
 
-int tcp_md5_hash_header(struct tcp_md5sig_pool *hp,
-			const struct tcphdr *th)
-{
-	struct scatterlist sg;
-	struct tcphdr hdr;
-
-	/* We are not allowed to change tcphdr, make a local copy */
-	memcpy(&hdr, th, sizeof(hdr));
-	hdr.check = 0;
-
-	/* options aren't included in the hash */
-	sg_init_one(&sg, &hdr, sizeof(hdr));
-	ahash_request_set_crypt(hp->md5_req, &sg, NULL, sizeof(hdr));
-	return crypto_ahash_update(hp->md5_req);
-}
-EXPORT_SYMBOL(tcp_md5_hash_header);
-
 int tcp_md5_hash_skb_data(struct tcp_md5sig_pool *hp,
 			  const struct sk_buff *skb, unsigned int header_len)
 {
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 3708de2a66833cf1d4a221a2b6ce3923bde978c4..c3c5a4cc53aac147b82e85a5d8d7001832594c6a 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1018,27 +1018,26 @@ static int tcp_v4_parse_md5_keys(struct sock *sk, char __user *optval,
 			      GFP_KERNEL);
 }
 
-static int tcp_v4_md5_hash_pseudoheader(struct tcp_md5sig_pool *hp,
-					__be32 daddr, __be32 saddr, int nbytes)
+static int tcp_v4_md5_hash_headers(struct tcp_md5sig_pool *hp,
+				   __be32 daddr, __be32 saddr,
+				   const struct tcphdr *th, int nbytes)
 {
-	struct tcp4_pseudohdr *bp;
+	struct tcp4_pseudohdr *bp = &hp->md5_blk.ip4;
 	struct scatterlist sg;
+	struct tcphdr *_th;
 
-	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));
-	ahash_request_set_crypt(hp->md5_req, &sg, NULL, sizeof(*bp));
+	_th = (struct tcphdr *)(bp + 1);
+	memcpy(_th, th, sizeof(*th));
+	_th->check = 0;
+	sg_init_one(&sg, bp, sizeof(*bp) + sizeof(*th));
+	ahash_request_set_crypt(hp->md5_req, &sg, NULL,
+				sizeof(*bp) + sizeof(*th));
 	return crypto_ahash_update(hp->md5_req);
 }
 
@@ -1055,9 +1054,7 @@ static int tcp_v4_md5_hash_hdr(char *md5_hash, const struct tcp_md5sig_key *key,
 
 	if (crypto_ahash_init(req))
 		goto clear_hash;
-	if (tcp_v4_md5_hash_pseudoheader(hp, daddr, saddr, th->doff << 2))
-		goto clear_hash;
-	if (tcp_md5_hash_header(hp, th))
+	if (tcp_v4_md5_hash_headers(hp, daddr, saddr, th, th->doff << 2))
 		goto clear_hash;
 	if (tcp_md5_hash_key(hp, key))
 		goto clear_hash;
@@ -1101,9 +1098,7 @@ int tcp_v4_md5_hash_skb(char *md5_hash, const struct tcp_md5sig_key *key,
 	if (crypto_ahash_init(req))
 		goto clear_hash;
 
-	if (tcp_v4_md5_hash_pseudoheader(hp, daddr, saddr, skb->len))
-		goto clear_hash;
-	if (tcp_md5_hash_header(hp, th))
+	if (tcp_v4_md5_hash_headers(hp, daddr, saddr, th, skb->len))
 		goto clear_hash;
 	if (tcp_md5_hash_skb_data(hp, skb, th->doff << 2))
 		goto clear_hash;
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index f36c2d076fce0df30d202d7683a67e3614d77fe9..3cff20dd2ab46fdff5ec82649f20f77f5d11aa9e 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -526,12 +526,14 @@ static int tcp_v6_parse_md5_keys(struct sock *sk, char __user *optval,
 			      AF_INET6, cmd.tcpm_key, cmd.tcpm_keylen, GFP_KERNEL);
 }
 
-static int tcp_v6_md5_hash_pseudoheader(struct tcp_md5sig_pool *hp,
-					const struct in6_addr *daddr,
-					const struct in6_addr *saddr, int nbytes)
+static int tcp_v6_md5_hash_headers(struct tcp_md5sig_pool *hp,
+				   const struct in6_addr *daddr,
+				   const struct in6_addr *saddr,
+				   const struct tcphdr *th, int nbytes)
 {
 	struct tcp6_pseudohdr *bp;
 	struct scatterlist sg;
+	struct tcphdr *_th;
 
 	bp = &hp->md5_blk.ip6;
 	/* 1. TCP pseudo-header (RFC2460) */
@@ -540,8 +542,13 @@ static int tcp_v6_md5_hash_pseudoheader(struct tcp_md5sig_pool *hp,
 	bp->protocol = cpu_to_be32(IPPROTO_TCP);
 	bp->len = cpu_to_be32(nbytes);
 
-	sg_init_one(&sg, bp, sizeof(*bp));
-	ahash_request_set_crypt(hp->md5_req, &sg, NULL, sizeof(*bp));
+	_th = (struct tcphdr *)(bp + 1);
+	memcpy(_th, th, sizeof(*th));
+	_th->check = 0;
+
+	sg_init_one(&sg, bp, sizeof(*bp) + sizeof(*th));
+	ahash_request_set_crypt(hp->md5_req, &sg, NULL,
+				sizeof(*bp) + sizeof(*th));
 	return crypto_ahash_update(hp->md5_req);
 }
 
@@ -559,9 +566,7 @@ static int tcp_v6_md5_hash_hdr(char *md5_hash, struct tcp_md5sig_key *key,
 
 	if (crypto_ahash_init(req))
 		goto clear_hash;
-	if (tcp_v6_md5_hash_pseudoheader(hp, daddr, saddr, th->doff << 2))
-		goto clear_hash;
-	if (tcp_md5_hash_header(hp, th))
+	if (tcp_v6_md5_hash_headers(hp, daddr, saddr, th, th->doff << 2))
 		goto clear_hash;
 	if (tcp_md5_hash_key(hp, key))
 		goto clear_hash;
@@ -606,9 +611,7 @@ static int tcp_v6_md5_hash_skb(char *md5_hash,
 	if (crypto_ahash_init(req))
 		goto clear_hash;
 
-	if (tcp_v6_md5_hash_pseudoheader(hp, daddr, saddr, skb->len))
-		goto clear_hash;
-	if (tcp_md5_hash_header(hp, th))
+	if (tcp_v6_md5_hash_headers(hp, daddr, saddr, th, skb->len))
 		goto clear_hash;
 	if (tcp_md5_hash_skb_data(hp, skb, th->doff << 2))
 		goto clear_hash;

[PATCH net-next] tcp: md5: do not use stack storage in crypto operations

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Some arches have virtually mapped kernel stacks, or will soon have.

tcp_md5_hash_header() uses an automatic variable to copy tcp header
before mangling th->check and calling crypto function, which might
be problematic on such arches.

So use percpu storage as we already do for the pseudo header,
and reduce number of crypto functions calls, as these headers
are ridiculously small.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Andy Lutomirski <luto@amacapital.net>
---
I am not sure the md5 crypto functions have a problem with data backed
in virtually mapped stacks, but this patch seems to remove the doubt.

 include/net/tcp.h   |    2 +-
 net/ipv4/tcp.c      |   17 -----------------
 net/ipv4/tcp_ipv4.c |   29 +++++++++++++----------------
 net/ipv6/tcp_ipv6.c |   27 +++++++++++++++------------
 4 files changed, 29 insertions(+), 46 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index a79894b667265cdf9e3fe793b4757e2f932b378a..2dd919e0289839130d2c5435b7925592082d62b5 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1385,6 +1385,7 @@ union tcp_md5sum_block {
 struct tcp_md5sig_pool {
 	struct ahash_request	*md5_req;
 	union tcp_md5sum_block	md5_blk;
+	struct tcphdr		tcp_hdr;
 };
 
 /* - functions */
@@ -1420,7 +1421,6 @@ static inline void tcp_put_md5sig_pool(void)
 	local_bh_enable();
 }
 
-int tcp_md5_hash_header(struct tcp_md5sig_pool *, const struct tcphdr *);
 int tcp_md5_hash_skb_data(struct tcp_md5sig_pool *, const struct sk_buff *,
 			  unsigned int header_len);
 int tcp_md5_hash_key(struct tcp_md5sig_pool *hp,
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 5c7ed147449c1b7ba029b12e033ad779a631460a..5fc9336934a11387e725300d6bca4aabd3991f19 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -3025,23 +3025,6 @@ struct tcp_md5sig_pool *tcp_get_md5sig_pool(void)
 }
 EXPORT_SYMBOL(tcp_get_md5sig_pool);
 
-int tcp_md5_hash_header(struct tcp_md5sig_pool *hp,
-			const struct tcphdr *th)
-{
-	struct scatterlist sg;
-	struct tcphdr hdr;
-
-	/* We are not allowed to change tcphdr, make a local copy */
-	memcpy(&hdr, th, sizeof(hdr));
-	hdr.check = 0;
-
-	/* options aren't included in the hash */
-	sg_init_one(&sg, &hdr, sizeof(hdr));
-	ahash_request_set_crypt(hp->md5_req, &sg, NULL, sizeof(hdr));
-	return crypto_ahash_update(hp->md5_req);
-}
-EXPORT_SYMBOL(tcp_md5_hash_header);
-
 int tcp_md5_hash_skb_data(struct tcp_md5sig_pool *hp,
 			  const struct sk_buff *skb, unsigned int header_len)
 {
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 3708de2a66833cf1d4a221a2b6ce3923bde978c4..8eb360c9b4a8dc6393e1dfa04957273ec7ac1e97 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1018,27 +1018,28 @@ static int tcp_v4_parse_md5_keys(struct sock *sk, char __user *optval,
 			      GFP_KERNEL);
 }
 
-static int tcp_v4_md5_hash_pseudoheader(struct tcp_md5sig_pool *hp,
-					__be32 daddr, __be32 saddr, int nbytes)
+static int tcp_v4_md5_hash_headers(struct tcp_md5sig_pool *hp,
+				   __be32 daddr, __be32 saddr,
+				   const struct tcphdr *th, int nbytes)
 {
 	struct tcp4_pseudohdr *bp;
 	struct scatterlist sg;
+	struct tcphdr *_th;
 
 	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));
-	ahash_request_set_crypt(hp->md5_req, &sg, NULL, sizeof(*bp));
+	_th = (struct tcphdr *)(bp + 1);
+	memcpy(_th, th, sizeof(*th));
+	_th->check = 0;
+
+	sg_init_one(&sg, bp, sizeof(*bp) + sizeof(*th));
+	ahash_request_set_crypt(hp->md5_req, &sg, NULL,
+				sizeof(*bp) + sizeof(*th));
 	return crypto_ahash_update(hp->md5_req);
 }
 
@@ -1055,9 +1056,7 @@ static int tcp_v4_md5_hash_hdr(char *md5_hash, const struct tcp_md5sig_key *key,
 
 	if (crypto_ahash_init(req))
 		goto clear_hash;
-	if (tcp_v4_md5_hash_pseudoheader(hp, daddr, saddr, th->doff << 2))
-		goto clear_hash;
-	if (tcp_md5_hash_header(hp, th))
+	if (tcp_v4_md5_hash_headers(hp, daddr, saddr, th, th->doff << 2))
 		goto clear_hash;
 	if (tcp_md5_hash_key(hp, key))
 		goto clear_hash;
@@ -1101,9 +1100,7 @@ int tcp_v4_md5_hash_skb(char *md5_hash, const struct tcp_md5sig_key *key,
 	if (crypto_ahash_init(req))
 		goto clear_hash;
 
-	if (tcp_v4_md5_hash_pseudoheader(hp, daddr, saddr, skb->len))
-		goto clear_hash;
-	if (tcp_md5_hash_header(hp, th))
+	if (tcp_v4_md5_hash_headers(hp, daddr, saddr, th, skb->len))
 		goto clear_hash;
 	if (tcp_md5_hash_skb_data(hp, skb, th->doff << 2))
 		goto clear_hash;
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index f36c2d076fce0df30d202d7683a67e3614d77fe9..614d685a3c407d3c9cb62f24505f70aadc7c4873 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -526,12 +526,14 @@ static int tcp_v6_parse_md5_keys(struct sock *sk, char __user *optval,
 			      AF_INET6, cmd.tcpm_key, cmd.tcpm_keylen, GFP_KERNEL);
 }
 
-static int tcp_v6_md5_hash_pseudoheader(struct tcp_md5sig_pool *hp,
-					const struct in6_addr *daddr,
-					const struct in6_addr *saddr, int nbytes)
+static int tcp_v6_md5_hash_headers(struct tcp_md5sig_pool *hp,
+				   const struct in6_addr *daddr,
+				   const struct in6_addr *saddr,
+				   const struct tcphdr *th, int nbytes)
 {
 	struct tcp6_pseudohdr *bp;
 	struct scatterlist sg;
+	struct tcphdr *_th;
 
 	bp = &hp->md5_blk.ip6;
 	/* 1. TCP pseudo-header (RFC2460) */
@@ -540,12 +542,17 @@ static int tcp_v6_md5_hash_pseudoheader(struct tcp_md5sig_pool *hp,
 	bp->protocol = cpu_to_be32(IPPROTO_TCP);
 	bp->len = cpu_to_be32(nbytes);
 
-	sg_init_one(&sg, bp, sizeof(*bp));
-	ahash_request_set_crypt(hp->md5_req, &sg, NULL, sizeof(*bp));
+	_th = (struct tcphdr *)(bp + 1);
+	memcpy(_th, th, sizeof(*th));
+	_th->check = 0;
+
+	sg_init_one(&sg, bp, sizeof(*bp) + sizeof(*th));
+	ahash_request_set_crypt(hp->md5_req, &sg, NULL,
+				sizeof(*bp) + sizeof(*th));
 	return crypto_ahash_update(hp->md5_req);
 }
 
-static int tcp_v6_md5_hash_hdr(char *md5_hash, struct tcp_md5sig_key *key,
+static int tcp_v6_md5_hash_hdr(char *md5_hash, const struct tcp_md5sig_key *key,
 			       const struct in6_addr *daddr, struct in6_addr *saddr,
 			       const struct tcphdr *th)
 {
@@ -559,9 +566,7 @@ static int tcp_v6_md5_hash_hdr(char *md5_hash, struct tcp_md5sig_key *key,
 
 	if (crypto_ahash_init(req))
 		goto clear_hash;
-	if (tcp_v6_md5_hash_pseudoheader(hp, daddr, saddr, th->doff << 2))
-		goto clear_hash;
-	if (tcp_md5_hash_header(hp, th))
+	if (tcp_v6_md5_hash_headers(hp, daddr, saddr, th, th->doff << 2))
 		goto clear_hash;
 	if (tcp_md5_hash_key(hp, key))
 		goto clear_hash;
@@ -606,9 +611,7 @@ static int tcp_v6_md5_hash_skb(char *md5_hash,
 	if (crypto_ahash_init(req))
 		goto clear_hash;
 
-	if (tcp_v6_md5_hash_pseudoheader(hp, daddr, saddr, skb->len))
-		goto clear_hash;
-	if (tcp_md5_hash_header(hp, th))
+	if (tcp_v6_md5_hash_headers(hp, daddr, saddr, th, skb->len))
 		goto clear_hash;
 	if (tcp_md5_hash_skb_data(hp, skb, th->doff << 2))
 		goto clear_hash;

Re: [PATCH net-next] tcp: md5: do not use stack storage in crypto operations

[Andy Lutomirski]

On Sat, Jun 25, 2016 at 9:09 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Some arches have virtually mapped kernel stacks, or will soon have.
>
> tcp_md5_hash_header() uses an automatic variable to copy tcp header
> before mangling th->check and calling crypto function, which might
> be problematic on such arches.
>
> So use percpu storage as we already do for the pseudo header,
> and reduce number of crypto functions calls, as these headers
> are ridiculously small.

Thanks!

Should this go in via net-next or -tip?

--Andy

Re: [PATCH net-next] tcp: md5: do not use stack storage in crypto operations

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 25 Jun 2016 18:09:35 +0200

> From: Eric Dumazet <edumazet@google.com>
> 
> Some arches have virtually mapped kernel stacks, or will soon have.
> 
> tcp_md5_hash_header() uses an automatic variable to copy tcp header
> before mangling th->check and calling crypto function, which might
> be problematic on such arches.
> 
> So use percpu storage as we already do for the pseudo header,
> and reduce number of crypto functions calls, as these headers
> are ridiculously small.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Andy Lutomirski <luto@amacapital.net>
> ---
> I am not sure the md5 crypto functions have a problem with data backed
> in virtually mapped stacks, but this patch seems to remove the doubt.

In a non-SMP build this storage will be in the kernel image itself.

And this violates the crypto layer's requirements.

It has to be memory taken from the page allocator or kmalloc.  So
before vmalloc'd stacks, the current code is perfectly fine.  And
with vmalloc'd stacks this patch trades one bug for another.

Re: [PATCH net-next] tcp: md5: do not use stack storage in crypto operations

[Andy Lutomirski]

On Mon, Jun 27, 2016 at 7:51 AM, David Miller <davem@davemloft.net> wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Sat, 25 Jun 2016 18:09:35 +0200
>
>> From: Eric Dumazet <edumazet@google.com>
>>
>> Some arches have virtually mapped kernel stacks, or will soon have.
>>
>> tcp_md5_hash_header() uses an automatic variable to copy tcp header
>> before mangling th->check and calling crypto function, which might
>> be problematic on such arches.
>>
>> So use percpu storage as we already do for the pseudo header,
>> and reduce number of crypto functions calls, as these headers
>> are ridiculously small.
>>
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> Reported-by: Andy Lutomirski <luto@amacapital.net>
>> ---
>> I am not sure the md5 crypto functions have a problem with data backed
>> in virtually mapped stacks, but this patch seems to remove the doubt.
>
> In a non-SMP build this storage will be in the kernel image itself.
>
> And this violates the crypto layer's requirements.
>
> It has to be memory taken from the page allocator or kmalloc.  So
> before vmalloc'd stacks, the current code is perfectly fine.  And
> with vmalloc'd stacks this patch trades one bug for another.

After poking around a bit: could this use crypto_shash_init +
crypto_shash_finup?  Those functions seem like they'll be just fine
with any virtual address, and I bet they're considerably faster than
using the ahash API.

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH net-next] tcp: md5: do not use stack storage in crypto operations

[Eric Dumazet]

On Mon, 2016-06-27 at 10:51 -0400, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Sat, 25 Jun 2016 18:09:35 +0200
> 
> > From: Eric Dumazet <edumazet@google.com>
> > 
> > Some arches have virtually mapped kernel stacks, or will soon have.
> > 
> > tcp_md5_hash_header() uses an automatic variable to copy tcp header
> > before mangling th->check and calling crypto function, which might
> > be problematic on such arches.
> > 
> > So use percpu storage as we already do for the pseudo header,
> > and reduce number of crypto functions calls, as these headers
> > are ridiculously small.
> > 
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Reported-by: Andy Lutomirski <luto@amacapital.net>
> > ---
> > I am not sure the md5 crypto functions have a problem with data backed
> > in virtually mapped stacks, but this patch seems to remove the doubt.
> 
> In a non-SMP build this storage will be in the kernel image itself.
> 
> And this violates the crypto layer's requirements.
> 
> It has to be memory taken from the page allocator or kmalloc.  So
> before vmalloc'd stacks, the current code is perfectly fine.  And
> with vmalloc'd stacks this patch trades one bug for another.

OK, so lets simply use kmalloc() allocated storage instead of percpu.

[PATCH v2 net-next] tcp: md5: use kmalloc() backed scratch areas

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Some arches have virtually mapped kernel stacks, or will soon have.

tcp_md5_hash_header() uses an automatic variable to copy tcp header
before mangling th->check and calling crypto function, which might
be problematic on such arches.

David says that using percpu storage is also problematic on non SMP
builds.

Just use kmalloc() to allocate scratch areas.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Andy Lutomirski <luto@amacapital.net>
---
 include/net/tcp.h   |    3 +--
 net/ipv4/tcp.c      |   10 ++++++++++
 net/ipv4/tcp_ipv4.c |   31 ++++++++++++++-----------------
 net/ipv6/tcp_ipv6.c |   29 ++++++++++++++++-------------
 4 files changed, 41 insertions(+), 32 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index a79894b667265cdf9e3fe793b4757e2f932b378a..7d892f65d6c88a4520e4e2a9695478e0e8a7a7f7 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1384,7 +1384,7 @@ union tcp_md5sum_block {
 /* - pool: digest algorithm, hash description and scratch buffer */
 struct tcp_md5sig_pool {
 	struct ahash_request	*md5_req;
-	union tcp_md5sum_block	md5_blk;
+	void			*scratch;
 };
 
 /* - functions */
@@ -1420,7 +1420,6 @@ static inline void tcp_put_md5sig_pool(void)
 	local_bh_enable();
 }
 
-int tcp_md5_hash_header(struct tcp_md5sig_pool *, const struct tcphdr *);
 int tcp_md5_hash_skb_data(struct tcp_md5sig_pool *, const struct sk_buff *,
 			  unsigned int header_len);
 int tcp_md5_hash_key(struct tcp_md5sig_pool *hp,
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 5c7ed147449c1b7ba029b12e033ad779a631460a..fddc0ab799996c1df82cb05dba03271b773e3b2d 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2969,8 +2969,18 @@ static void __tcp_alloc_md5sig_pool(void)
 		return;
 
 	for_each_possible_cpu(cpu) {
+		void *scratch = per_cpu(tcp_md5sig_pool, cpu).scratch;
 		struct ahash_request *req;
 
+		if (!scratch) {
+			scratch = kmalloc_node(sizeof(union tcp_md5sum_block) +
+					       sizeof(struct tcphdr),
+					       GFP_KERNEL,
+					       cpu_to_node(cpu));
+			if (!scratch)
+				return;
+			per_cpu(tcp_md5sig_pool, cpu).scratch = scratch;
+		}
 		if (per_cpu(tcp_md5sig_pool, cpu).md5_req)
 			continue;
 
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 3708de2a66833cf1d4a221a2b6ce3923bde978c4..32b048e524d6773538918eca175b3f422f9c2aa7 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1018,27 +1018,28 @@ static int tcp_v4_parse_md5_keys(struct sock *sk, char __user *optval,
 			      GFP_KERNEL);
 }
 
-static int tcp_v4_md5_hash_pseudoheader(struct tcp_md5sig_pool *hp,
-					__be32 daddr, __be32 saddr, int nbytes)
+static int tcp_v4_md5_hash_headers(struct tcp_md5sig_pool *hp,
+				   __be32 daddr, __be32 saddr,
+				   const struct tcphdr *th, int nbytes)
 {
 	struct tcp4_pseudohdr *bp;
 	struct scatterlist sg;
+	struct tcphdr *_th;
 
-	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 = hp->scratch;
 	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));
-	ahash_request_set_crypt(hp->md5_req, &sg, NULL, sizeof(*bp));
+	_th = (struct tcphdr *)(bp + 1);
+	memcpy(_th, th, sizeof(*th));
+	_th->check = 0;
+
+	sg_init_one(&sg, bp, sizeof(*bp) + sizeof(*th));
+	ahash_request_set_crypt(hp->md5_req, &sg, NULL,
+				sizeof(*bp) + sizeof(*th));
 	return crypto_ahash_update(hp->md5_req);
 }
 
@@ -1055,9 +1056,7 @@ static int tcp_v4_md5_hash_hdr(char *md5_hash, const struct tcp_md5sig_key *key,
 
 	if (crypto_ahash_init(req))
 		goto clear_hash;
-	if (tcp_v4_md5_hash_pseudoheader(hp, daddr, saddr, th->doff << 2))
-		goto clear_hash;
-	if (tcp_md5_hash_header(hp, th))
+	if (tcp_v4_md5_hash_headers(hp, daddr, saddr, th, th->doff << 2))
 		goto clear_hash;
 	if (tcp_md5_hash_key(hp, key))
 		goto clear_hash;
@@ -1101,9 +1100,7 @@ int tcp_v4_md5_hash_skb(char *md5_hash, const struct tcp_md5sig_key *key,
 	if (crypto_ahash_init(req))
 		goto clear_hash;
 
-	if (tcp_v4_md5_hash_pseudoheader(hp, daddr, saddr, skb->len))
-		goto clear_hash;
-	if (tcp_md5_hash_header(hp, th))
+	if (tcp_v4_md5_hash_headers(hp, daddr, saddr, th, skb->len))
 		goto clear_hash;
 	if (tcp_md5_hash_skb_data(hp, skb, th->doff << 2))
 		goto clear_hash;
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index f36c2d076fce0df30d202d7683a67e3614d77fe9..bf3556f1808fd4e625c9c4b69431d2ee22463f5e 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -526,26 +526,33 @@ static int tcp_v6_parse_md5_keys(struct sock *sk, char __user *optval,
 			      AF_INET6, cmd.tcpm_key, cmd.tcpm_keylen, GFP_KERNEL);
 }
 
-static int tcp_v6_md5_hash_pseudoheader(struct tcp_md5sig_pool *hp,
-					const struct in6_addr *daddr,
-					const struct in6_addr *saddr, int nbytes)
+static int tcp_v6_md5_hash_headers(struct tcp_md5sig_pool *hp,
+				   const struct in6_addr *daddr,
+				   const struct in6_addr *saddr,
+				   const struct tcphdr *th, int nbytes)
 {
 	struct tcp6_pseudohdr *bp;
 	struct scatterlist sg;
+	struct tcphdr *_th;
 
-	bp = &hp->md5_blk.ip6;
+	bp = hp->scratch;
 	/* 1. TCP pseudo-header (RFC2460) */
 	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));
-	ahash_request_set_crypt(hp->md5_req, &sg, NULL, sizeof(*bp));
+	_th = (struct tcphdr *)(bp + 1);
+	memcpy(_th, th, sizeof(*th));
+	_th->check = 0;
+
+	sg_init_one(&sg, bp, sizeof(*bp) + sizeof(*th));
+	ahash_request_set_crypt(hp->md5_req, &sg, NULL,
+				sizeof(*bp) + sizeof(*th));
 	return crypto_ahash_update(hp->md5_req);
 }
 
-static int tcp_v6_md5_hash_hdr(char *md5_hash, struct tcp_md5sig_key *key,
+static int tcp_v6_md5_hash_hdr(char *md5_hash, const struct tcp_md5sig_key *key,
 			       const struct in6_addr *daddr, struct in6_addr *saddr,
 			       const struct tcphdr *th)
 {
@@ -559,9 +566,7 @@ static int tcp_v6_md5_hash_hdr(char *md5_hash, struct tcp_md5sig_key *key,
 
 	if (crypto_ahash_init(req))
 		goto clear_hash;
-	if (tcp_v6_md5_hash_pseudoheader(hp, daddr, saddr, th->doff << 2))
-		goto clear_hash;
-	if (tcp_md5_hash_header(hp, th))
+	if (tcp_v6_md5_hash_headers(hp, daddr, saddr, th, th->doff << 2))
 		goto clear_hash;
 	if (tcp_md5_hash_key(hp, key))
 		goto clear_hash;
@@ -606,9 +611,7 @@ static int tcp_v6_md5_hash_skb(char *md5_hash,
 	if (crypto_ahash_init(req))
 		goto clear_hash;
 
-	if (tcp_v6_md5_hash_pseudoheader(hp, daddr, saddr, skb->len))
-		goto clear_hash;
-	if (tcp_md5_hash_header(hp, th))
+	if (tcp_v6_md5_hash_headers(hp, daddr, saddr, th, skb->len))
 		goto clear_hash;
 	if (tcp_md5_hash_skb_data(hp, skb, th->doff << 2))
 		goto clear_hash;

Re: [PATCH v2 net-next] tcp: md5: use kmalloc() backed scratch areas

[Andy Lutomirski]

On Mon, Jun 27, 2016 at 9:51 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Some arches have virtually mapped kernel stacks, or will soon have.
>
> tcp_md5_hash_header() uses an automatic variable to copy tcp header
> before mangling th->check and calling crypto function, which might
> be problematic on such arches.
>
> David says that using percpu storage is also problematic on non SMP
> builds.
>
> Just use kmalloc() to allocate scratch areas.

Seems reasonable.

I wonder if it's worth switching from ahash to shash, though.  It
would probably be simpler and faster.

--Andy

Re: [PATCH v2 net-next] tcp: md5: use kmalloc() backed scratch areas

[Cong Wang]

On Mon, Jun 27, 2016 at 9:51 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 5c7ed147449c1b7ba029b12e033ad779a631460a..fddc0ab799996c1df82cb05dba03271b773e3b2d 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2969,8 +2969,18 @@ static void __tcp_alloc_md5sig_pool(void)
>                 return;
>
>         for_each_possible_cpu(cpu) {
> +               void *scratch = per_cpu(tcp_md5sig_pool, cpu).scratch;
>                 struct ahash_request *req;
>
> +               if (!scratch) {
> +                       scratch = kmalloc_node(sizeof(union tcp_md5sum_block) +
> +                                              sizeof(struct tcphdr),
> +                                              GFP_KERNEL,
> +                                              cpu_to_node(cpu));
> +                       if (!scratch)
> +                               return;
> +                       per_cpu(tcp_md5sig_pool, cpu).scratch = scratch;
> +               }
>                 if (per_cpu(tcp_md5sig_pool, cpu).md5_req)
>                         continue;

Not a problem of your patch, but it seems these allocations never
get freed once we start using tcp md5. Maybe we should free them
when the last socket using tcp md5 is gone?

Re: [PATCH v2 net-next] tcp: md5: use kmalloc() backed scratch areas

[Eric Dumazet]

On Mon, 2016-06-27 at 10:58 -0700, Andy Lutomirski wrote:

> Seems reasonable.
> 
> I wonder if it's worth switching from ahash to shash, though.  It
> would probably be simpler and faster.

Well, I have no opinion on this, I will let a crypto guy doing this
change if he cares ;)

Thanks.

Re: [PATCH v2 net-next] tcp: md5: use kmalloc() backed scratch areas

[Eric Dumazet]

On Mon, 2016-06-27 at 11:31 -0700, Cong Wang wrote:

> Not a problem of your patch, but it seems these allocations never
> get freed once we start using tcp md5. Maybe we should free them
> when the last socket using tcp md5 is gone?

If we constantly allocate-deallocate these tiny blocks for occasional
TCP MD5 use, it becomes quite expensive.

With current code, only first TCP MD5 usage trigger an extra setup cost.

Re: [PATCH v2 net-next] tcp: md5: use kmalloc() backed scratch areas

[Herbert Xu]

On Mon, Jun 27, 2016 at 10:58:42AM -0700, Andy Lutomirski wrote:
>
> I wonder if it's worth switching from ahash to shash, though.  It
> would probably be simpler and faster.

No shash is not appropriate here because it needs to hash skb
frags which are SG lists.

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

Re: [PATCH v2 net-next] tcp: md5: use kmalloc() backed scratch areas

[Andy Lutomirski]

On Mon, Jun 27, 2016 at 8:41 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Mon, Jun 27, 2016 at 10:58:42AM -0700, Andy Lutomirski wrote:
>>
>> I wonder if it's worth switching from ahash to shash, though.  It
>> would probably be simpler and faster.
>
> No shash is not appropriate here because it needs to hash skb
> frags which are SG lists.
>

Do you mean this code:

    for (i = 0; i < shi->nr_frags; ++i) {
        const struct skb_frag_struct *f = &shi->frags[i];
        unsigned int offset = f->page_offset;
        struct page *page = skb_frag_page(f) + (offset >> PAGE_SHIFT);

        sg_set_page(&sg, page, skb_frag_size(f),
                offset_in_page(offset));
        ahash_request_set_crypt(req, &sg, NULL, skb_frag_size(f));
        if (crypto_ahash_update(req))
            return 1;
    }

I'm wondering why support for scatterlists is all-or-nothing.  Why
can't we initialize a hash object and then alternate between passing
it scatterlists and pointers?

I'm guessing that ahash enables async operation and shash is
synchronous only.  If I'm right, I understand why ahash requires a
scatterlist.  What I don't understand is why shash can't also accept a
scatterlist.  It appears that most of the ahash users in the tree
actually want synchronous crypto and are presumably using ahash for
some other reason such as ahash's ability to hash via scatterlist (in
this case, struct page *).

--Andy

Re: [PATCH v2 net-next] tcp: md5: use kmalloc() backed scratch areas

[Herbert Xu]

On Tue, Jun 28, 2016 at 10:35:31AM -0700, Andy Lutomirski wrote:
>
> Do you mean this code:

Yes.
 
> I'm wondering why support for scatterlists is all-or-nothing.  Why
> can't we initialize a hash object and then alternate between passing
> it scatterlists and pointers?

Because once you have started hashing the hash state is not stored
in a consistent format.  Our software code may maintain one format
while a hardware implementation could do something else altogether.
So you have to stick with one implementation throughout a particular
hashing session.

> I'm guessing that ahash enables async operation and shash is
> synchronous only.  If I'm right, I understand why ahash requires a
> scatterlist.  What I don't understand is why shash can't also accept a
> scatterlist.  It appears that most of the ahash users in the tree
> actually want synchronous crypto and are presumably using ahash for
> some other reason such as ahash's ability to hash via scatterlist (in
> this case, struct page *).

ahash is meant to be the interface everyone uses regardless of
whether they want sync-only or async.  shash should only be used
for small amounts of hashing on virtual addresses.

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

Re: [PATCH v2 net-next] tcp: md5: use kmalloc() backed scratch areas

[Andy Lutomirski]

On Tue, Jun 28, 2016 at 7:23 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Tue, Jun 28, 2016 at 10:35:31AM -0700, Andy Lutomirski wrote:
>>
>> Do you mean this code:
>
> Yes.
>
>> I'm wondering why support for scatterlists is all-or-nothing.  Why
>> can't we initialize a hash object and then alternate between passing
>> it scatterlists and pointers?
>
> Because once you have started hashing the hash state is not stored
> in a consistent format.  Our software code may maintain one format
> while a hardware implementation could do something else altogether.
> So you have to stick with one implementation throughout a particular
> hashing session.
>
>> I'm guessing that ahash enables async operation and shash is
>> synchronous only.  If I'm right, I understand why ahash requires a
>> scatterlist.  What I don't understand is why shash can't also accept a
>> scatterlist.  It appears that most of the ahash users in the tree
>> actually want synchronous crypto and are presumably using ahash for
>> some other reason such as ahash's ability to hash via scatterlist (in
>> this case, struct page *).
>
> ahash is meant to be the interface everyone uses regardless of
> whether they want sync-only or async.  shash should only be used
> for small amounts of hashing on virtual addresses.

I suspect that, if you compare a synchronous implementation that can
use virtual addresses to a DMA based implementation that can't, you'll
find that, for short messages like tcp md5 uses, the synchronous
implementation would win every time.  I'm wondering if shash should
gain the ability to use scatterlists and, if it doesn't already have
it, the ability to use synchronous hardware implementations (like
SHA-NI, for example, although I don't think that's useful for MD5).

--Andy

Re: [PATCH v2 net-next] tcp: md5: use kmalloc() backed scratch areas

[Herbert Xu]

On Wed, Jun 29, 2016 at 07:59:22AM -0700, Andy Lutomirski wrote:
>> I suspect that, if you compare a synchronous implementation that can
> use virtual addresses to a DMA based implementation that can't, you'll
> find that, for short messages like tcp md5 uses, the synchronous
> implementation would win every time.  I'm wondering if shash should
> gain the ability to use scatterlists and, if it doesn't already have
> it, the ability to use synchronous hardware implementations (like
> SHA-NI, for example, although I don't think that's useful for MD5).

I don't understand, if you add SGs to shash you get ahash.  So
why wouldn't you just use ahash?

AFAICS tcp md5 already uses ahash in sync mode so there is nothing
asynchronous here at all.

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

Re: [PATCH v2 net-next] tcp: md5: use kmalloc() backed scratch areas

[Andy Lutomirski]

On Wed, Jun 29, 2016 at 8:02 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Wed, Jun 29, 2016 at 07:59:22AM -0700, Andy Lutomirski wrote:
>>> I suspect that, if you compare a synchronous implementation that can
>> use virtual addresses to a DMA based implementation that can't, you'll
>> find that, for short messages like tcp md5 uses, the synchronous
>> implementation would win every time.  I'm wondering if shash should
>> gain the ability to use scatterlists and, if it doesn't already have
>> it, the ability to use synchronous hardware implementations (like
>> SHA-NI, for example, although I don't think that's useful for MD5).
>
> I don't understand, if you add SGs to shash you get ahash.  So
> why wouldn't you just use ahash?

Two reasons:

1. Code like tcp md5 would be simpler if it could pass a scatterlist
to hash the skb but use a virtual address for the header.

2. The actual calling sequence and the amount of indirection is much
less for shash, so hashing short buffer is probably *much* faster.

ahash is very featureful, but it's also quite heavyweight and it's
missing the ability to use virtual addresses directly (for good
reason).  shash is simpler and probably much faster on short buffers,
but the only feature it's missing for most callers (the ones that want
synchronous operation) is the ability to use a scatterlist.  Given
that the crypto code already has the ability to walk a scatterlist,
map it, and hash it, it seems like it might be a nice addition to let
shash objects invoke that code path if they want
(crypto_shash_update_sg?).  This would have no overhead for users that
don't call it, and I bet it would both speed up and reduce the amount
of code in users like tcp md5.

--Andy

Re: [PATCH v2 net-next] tcp: md5: use kmalloc() backed scratch areas

[Herbert Xu]

On Wed, Jun 29, 2016 at 08:26:43AM -0700, Andy Lutomirski wrote:
> 
> Two reasons:
> 
> 1. Code like tcp md5 would be simpler if it could pass a scatterlist
> to hash the skb but use a virtual address for the header.

True.  But I bet we can make it simpler in other ways without
creating special interfaces for it.  Look at how we do IPsec
encryption/hashing, this is what TCP md5 should look like.  But
nobody in their right mind would bother doing this because this
is dead code.

> 2. The actual calling sequence and the amount of indirection is much
> less for shash, so hashing short buffer is probably *much* faster.

Really?

Have you measured the speed difference between the ahash and shash
interfaces? Are you sure that this would matter when compared
against the speed of hashing a single MD5 block?

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

Re: [PATCH v2 net-next] tcp: md5: use kmalloc() backed scratch areas

[Andy Lutomirski]

On Wed, Jun 29, 2016 at 8:38 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Wed, Jun 29, 2016 at 08:26:43AM -0700, Andy Lutomirski wrote:
>>
>> Two reasons:
>>
>> 1. Code like tcp md5 would be simpler if it could pass a scatterlist
>> to hash the skb but use a virtual address for the header.
>
> True.  But I bet we can make it simpler in other ways without
> creating special interfaces for it.  Look at how we do IPsec
> encryption/hashing, this is what TCP md5 should look like.  But
> nobody in their right mind would bother doing this because this
> is dead code.
>
>> 2. The actual calling sequence and the amount of indirection is much
>> less for shash, so hashing short buffer is probably *much* faster.
>
> Really?
>
> Have you measured the speed difference between the ahash and shash
> interfaces? Are you sure that this would matter when compared
> against the speed of hashing a single MD5 block?
>

It's non-negligible.  If I had SHA-NI, it would probably be a bigger
relative difference.

[    0.535717] SHA1 ahash: warming up
[    0.737471] SHA1 ahash: 500000 loops, 386 ns/loop
[    0.737937] SHA1 shash: warming up
[    0.903131] SHA1 shash: 500000 loops, 323 ns/loop
[    1.055409] SHA1 shash_digest: 500000 loops, 303 ns/loop

[    0.574410] SHA1 ahash: warming up
[    0.796927] SHA1 ahash: 500000 loops, 414 ns/loop
[    0.797695] SHA1 shash: warming up
[    0.959772] SHA1 shash: 500000 loops, 316 ns/loop
[    1.100886] SHA1 shash_digest: 500000 loops, 280 ns/loop

Here's MD5:

[    0.504708] MD5 ahash: warming up
[    0.688155] MD5 ahash: 500000 loops, 344 ns/loop
[    0.688971] MD5 shash: warming up
[    0.834953] MD5 shash: 500000 loops, 285 ns/loop
[    0.982021] MD5 shash_digest: 500000 loops, 292 ns/loop

[    0.570780] MD5 ahash: warming up
[    0.754325] MD5 ahash: 500000 loops, 357 ns/loop
[    0.754807] MD5 shash: warming up
[    0.906861] MD5 shash: 500000 loops, 297 ns/loop
[    1.059057] MD5 shash_digest: 500000 loops, 303 ns/loop

If you throw in sub-one-block requests, it'll be worse, because those
requests don't do real work.

Overall, it looks like there's overhead of something like 50ns for
each ahash invocation vs the shash equivalent.  It's not huge, but
it's there.  (This is cache-hot.  I bet it's considerably worse if
cache-cold, because ahash will require a lot more code cache lines as
well as the extra cache lines involved in the scatterlist and whatever
arch stuff is needed to map back and forth between virtual and
physical addresses.

Here's the code:

static void test_ahash(void)
{
    struct crypto_ahash *hash;
    struct ahash_request *req;
    struct scatterlist sg_in;
    char *buf, *out;
    int i;
    u64 end, start;
    unsigned loops = 500000;

    hash = crypto_alloc_ahash("sha1", 0, CRYPTO_ALG_ASYNC);
    if (IS_ERR(hash))
        return;

    req = ahash_request_alloc(hash, GFP_KERNEL);
    if (!req)
        return;

    ahash_request_set_callback(req, 0, NULL, NULL);

    buf = kmalloc(64, GFP_KERNEL);
    memset(buf, 0, 64);
    out = kmalloc(20, GFP_KERNEL);

    pr_err("SHA1 ahash: warming up\n");

    for (i = 0; i < 10000; i++) {
        crypto_ahash_init(req);
        sg_init_one(&sg_in, buf, 64);
        ahash_request_set_crypt(req, &sg_in, NULL, 64);
        if (crypto_ahash_update(req))
            return;
        ahash_request_set_crypt(req, NULL, out, 0);
        if (crypto_ahash_final(req))
            return;
    }

    start = ktime_get_ns();
    for (i = 0; i < loops; i++) {
        crypto_ahash_init(req);
        sg_init_one(&sg_in, buf, 64);
        ahash_request_set_crypt(req, &sg_in, NULL, 64);
        if (crypto_ahash_update(req))
            return;
        ahash_request_set_crypt(req, NULL, out, 0);
        if (crypto_ahash_final(req))
            return;
    }
    end = ktime_get_ns();

    pr_err("SHA1 ahash: %u loops, %llu ns/loop\n", loops, (unsigned
long long)((end-start)/loops));
}

static void test_shash(void)
{
    struct crypto_shash *hash;
    struct shash_desc *desc;
    char *buf, *out;
    int i;
    u64 end, start;
    unsigned loops = 500000;

    hash = crypto_alloc_shash("sha1", 0, CRYPTO_ALG_ASYNC);
    if (IS_ERR(hash))
        return;

    desc = kmalloc(crypto_shash_descsize(hash), GFP_KERNEL);
    if (!desc)
        return;
    desc->tfm = hash;
    desc->flags = 0;

    /*
     * Hmm.  This interface isn't conducive to the use of hardware that
     * needs state beyond what lives in a single memory buffer -- how would
     * the shash implementation know when to free the state?
     */

    buf = kmalloc(64, GFP_KERNEL);
    memset(buf, 0, 64);
    out = kmalloc(20, GFP_KERNEL);

    pr_err("SHA1 shash: warming up\n");

    for (i = 0; i < 10000; i++) {
        crypto_shash_init(desc);
        if (crypto_shash_update(desc, buf, 64))
            return;
        if (crypto_shash_final(desc, out))
            return;
    }

    start = ktime_get_ns();
    for (i = 0; i < loops; i++) {
        /*
         * For fairness, this does init; update; final instead of
         * just digest.
         */
        crypto_shash_init(desc);
        if (crypto_shash_update(desc, buf, 64))
            return;
        if (crypto_shash_final(desc, out))
            return;
    }
    end = ktime_get_ns();

    pr_err("SHA1 shash: %u loops, %llu ns/loop\n", loops, (unsigned
long long)((end-start)/loops));

    start = ktime_get_ns();
    for (i = 0; i < loops; i++) {
        crypto_shash_digest(desc, buf, 64, out);
    }
    end = ktime_get_ns();

    pr_err("SHA1 shash_digest: %u loops, %llu ns/loop\n", loops,
(unsigned long long)((end-start)/loops));
}

Re: [PATCH v2 net-next] tcp: md5: use kmalloc() backed scratch areas

[Eric Dumazet]

On Wed, 2016-06-29 at 09:41 -0700, Andy Lutomirski wrote:

> Overall, it looks like there's overhead of something like 50ns for
> each ahash invocation vs the shash equivalent.  It's not huge, but
> it's there.  (This is cache-hot.  I bet it's considerably worse if
> cache-cold, because ahash will require a lot more code cache lines as
> well as the extra cache lines involved in the scatterlist and whatever
> arch stuff is needed to map back and forth between virtual and
> physical addresses.

I am kind of mystified seeing someone caring about TCP MD5, other than
just making sure it wont crash the host when it needs to be used ;)

The real useful work would be to use a jump label so that we can avoid
spending cycles for non TCP MD5 sessions, when a host never had to use
any MD5 negotiation.

Re: [PATCH v2 net-next] tcp: md5: use kmalloc() backed scratch areas

[Andy Lutomirski]

On Wed, Jun 29, 2016 at 2:44 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2016-06-29 at 09:41 -0700, Andy Lutomirski wrote:
>
>> Overall, it looks like there's overhead of something like 50ns for
>> each ahash invocation vs the shash equivalent.  It's not huge, but
>> it's there.  (This is cache-hot.  I bet it's considerably worse if
>> cache-cold, because ahash will require a lot more code cache lines as
>> well as the extra cache lines involved in the scatterlist and whatever
>> arch stuff is needed to map back and forth between virtual and
>> physical addresses.
>
> I am kind of mystified seeing someone caring about TCP MD5, other than
> just making sure it wont crash the host when it needs to be used ;)
>
> The real useful work would be to use a jump label so that we can avoid
> spending cycles for non TCP MD5 sessions, when a host never had to use
> any MD5 negotiation.
>
>
>

I don't care about TCP MD5 performance at all.  Ease of maintenance is
nice, though, and maybe there are other places in the kernel where
performance does matter.

--Andy

Re: [PATCH v2 net-next] tcp: md5: use kmalloc() backed scratch areas

[Herbert Xu]

On Wed, Jun 29, 2016 at 03:39:37PM -0700, Andy Lutomirski wrote:
>
> I don't care about TCP MD5 performance at all.  Ease of maintenance is
> nice, though, and maybe there are other places in the kernel where
> performance does matter.

TCP MD5 is using ahash because it touches SG lists.  Touching
SG lists is a pretty reliable indicator for hashing a large amount
of data.  In fact TCP MD5 hashes the entire packet content so we're
talking about ~1000 bytes, just like IPsec.

Therefore it is completely pointless to use something like shash
for it as shash is only meant to be used for those hashing less
one block of data or less.

If you're aware of any other user in the kernel that is using
ahash and is hashing a small amount of data in aggregate (not
per update) please let me know.

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

Re: [PATCH v2 net-next] tcp: md5: use kmalloc() backed scratch areas

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 27 Jun 2016 18:51:53 +0200

> From: Eric Dumazet <edumazet@google.com>
> 
> Some arches have virtually mapped kernel stacks, or will soon have.
> 
> tcp_md5_hash_header() uses an automatic variable to copy tcp header
> before mangling th->check and calling crypto function, which might
> be problematic on such arches.
> 
> David says that using percpu storage is also problematic on non SMP
> builds.
> 
> Just use kmalloc() to allocate scratch areas.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Andy Lutomirski <luto@amacapital.net>

Applied, thanks.