[RFC] tcp md5 use of alloc_percpu

[RFC] tcp md5 use of alloc_percpu

[Crestez Dan Leonard]

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.

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? 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?

Here's what I mean by allocating tcp{4,6}_pseudohdr on the stack:

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,

Re: [RFC] tcp md5 use of alloc_percpu

[Eric Dumazet]

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

Re: [RFC] tcp md5 use of alloc_percpu

[Jonathan Toppins]

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.

Re: [RFC] tcp md5 use of alloc_percpu

[David Miller]

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.

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

[Jonathan Toppins]

On 10/22/14, 2:55 PM, Crestez Dan Leonard wrote:
> 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.

Thinking about this more if the issue really is sg_init_one assumes a
directly accessible memory region, can we just modify the zone
allocation to GFP_DMA using alloc_percpu_gfp()? Does this satisfy the
assumptions made by sg_init_one?

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 1bec4e7..6924320 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2889,7 +2889,7 @@ static void __tcp_alloc_md5sig_pool(void)
        int cpu;
        struct tcp_md5sig_pool __percpu *pool;

-       pool = alloc_percpu(struct tcp_md5sig_pool);
+       pool = alloc_percpu_gfp(struct tcp_md5sig_pool, GFP_DMA);
        if (!pool)
                return;

Re: [RFC] tcp md5 use of alloc_percpu

[Crestez Dan Leonard]

On 10/23/2014 02:38 AM, Jonathan Toppins wrote:
> On 10/22/14, 2:55 PM, Crestez Dan Leonard wrote:
>> 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.
>
> Thinking about this more if the issue really is sg_init_one assumes a
> directly accessible memory region, can we just modify the zone
> allocation to GFP_DMA using alloc_percpu_gfp()? Does this satisfy the
> assumptions made by sg_init_one?
I don't think that alloc_percpu_gfp can be used that way. Looking at the 
code it only checks for GFP_KERNEL and behaves "atomically" if it is not 
present. This means that it fails rather than vmalloc a new percpu_chunk.

The problem is not that the memory is not allocated with GFP_DMA but 
rather that the memory is allocated with vmalloc.

Regards,
Leonard

Re: [RFC] tcp md5 use of alloc_percpu

[Eric Dumazet]

On Thu, 2014-10-23 at 04:00 +0300, Crestez Dan Leonard wrote:
> On 10/23/2014 02:38 AM, Jonathan Toppins wrote:
> > On 10/22/14, 2:55 PM, Crestez Dan Leonard wrote:
> >> 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.
> >
> > Thinking about this more if the issue really is sg_init_one assumes a
> > directly accessible memory region, can we just modify the zone
> > allocation to GFP_DMA using alloc_percpu_gfp()? Does this satisfy the
> > assumptions made by sg_init_one?
> I don't think that alloc_percpu_gfp can be used that way. Looking at the 
> code it only checks for GFP_KERNEL and behaves "atomically" if it is not 
> present. This means that it fails rather than vmalloc a new percpu_chunk.
> 
> The problem is not that the memory is not allocated with GFP_DMA but 
> rather that the memory is allocated with vmalloc.

Could you try the following patch ?

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 1bec4e76d88c..d253ad8ced64 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2868,30 +2868,29 @@ EXPORT_SYMBOL(compat_tcp_getsockopt);
 #endif
 
 #ifdef CONFIG_TCP_MD5SIG
-static struct tcp_md5sig_pool __percpu *tcp_md5sig_pool __read_mostly;
+static DEFINE_PER_CPU(struct tcp_md5sig_pool, tcp_md5sig_pool);
 static DEFINE_MUTEX(tcp_md5sig_mutex);
+static bool tcp_md5sig_pool_populated = false;
 
-static void __tcp_free_md5sig_pool(struct tcp_md5sig_pool __percpu *pool)
+static void tcp_free_md5sig_pool(void)
 {
 	int cpu;
 
 	for_each_possible_cpu(cpu) {
-		struct tcp_md5sig_pool *p = per_cpu_ptr(pool, cpu);
+		struct crypto_hash *hash;
+
+		hash = per_cpu(tcp_md5sig_pool, cpu).md5_desc.tfm;
 
-		if (p->md5_desc.tfm)
-			crypto_free_hash(p->md5_desc.tfm);
+		if (hash) {
+			crypto_free_hash(hash);
+			per_cpu(tcp_md5sig_pool, cpu).md5_desc.tfm = NULL;
+		}
 	}
-	free_percpu(pool);
 }
 
 static void __tcp_alloc_md5sig_pool(void)
 {
 	int cpu;
-	struct tcp_md5sig_pool __percpu *pool;
-
-	pool = alloc_percpu(struct tcp_md5sig_pool);
-	if (!pool)
-		return;
 
 	for_each_possible_cpu(cpu) {
 		struct crypto_hash *hash;
@@ -2900,29 +2899,29 @@ static void __tcp_alloc_md5sig_pool(void)
 		if (IS_ERR_OR_NULL(hash))
 			goto out_free;
 
-		per_cpu_ptr(pool, cpu)->md5_desc.tfm = hash;
+		per_cpu(tcp_md5sig_pool, cpu).md5_desc.tfm = hash;
 	}
-	/* before setting tcp_md5sig_pool, we must commit all writes
-	 * to memory. See ACCESS_ONCE() in tcp_get_md5sig_pool()
+	/* before setting tcp_md5sig_pool_populated, we must commit all writes
+	 * to memory. See smp_rmb() in tcp_get_md5sig_pool()
 	 */
 	smp_wmb();
-	tcp_md5sig_pool = pool;
+	tcp_md5sig_pool_populated = true;
 	return;
 out_free:
-	__tcp_free_md5sig_pool(pool);
+	tcp_free_md5sig_pool();
 }
 
 bool tcp_alloc_md5sig_pool(void)
 {
-	if (unlikely(!tcp_md5sig_pool)) {
+	if (unlikely(!tcp_md5sig_pool_populated)) {
 		mutex_lock(&tcp_md5sig_mutex);
 
-		if (!tcp_md5sig_pool)
+		if (!tcp_md5sig_pool_populated)
 			__tcp_alloc_md5sig_pool();
 
 		mutex_unlock(&tcp_md5sig_mutex);
 	}
-	return tcp_md5sig_pool != NULL;
+	return tcp_md5sig_pool_populated;
 }
 EXPORT_SYMBOL(tcp_alloc_md5sig_pool);
 
@@ -2936,13 +2935,13 @@ EXPORT_SYMBOL(tcp_alloc_md5sig_pool);
  */
 struct tcp_md5sig_pool *tcp_get_md5sig_pool(void)
 {
-	struct tcp_md5sig_pool __percpu *p;
-
 	local_bh_disable();
-	p = ACCESS_ONCE(tcp_md5sig_pool);
-	if (p)
-		return raw_cpu_ptr(p);
 
+	if (tcp_md5sig_pool_populated) {
+		/* coupled with smp_wmb() in __tcp_alloc_md5sig_pool */
+		smp_rmb();
+		return this_cpu_ptr(&tcp_md5sig_pool);
+	}
 	local_bh_enable();
 	return NULL;
 }

Re: [RFC] tcp md5 use of alloc_percpu

[David Ahern]

On 10/22/14, 12:55 PM, 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:

This is a forward port of a local change to address the problem (local 
kernel version is 3.4 so perhaps my quick bump to top of tree is off but 
it shows the general idea). Been on my to-do list to figure out why this 
is needed, but it seems related to your problem:

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 1bec4e76d88c..833a676bd4b0 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2941,7 +2941,7 @@ struct tcp_md5sig_pool *tcp_get_md5sig_pool(void)
     local_bh_disable();
     p = ACCESS_ONCE(tcp_md5sig_pool);
     if (p)
-       return raw_cpu_ptr(p);
+       return __va(per_cpu_ptr_to_phys(raw_cpu_ptr(p)));

     local_bh_enable();
     return NULL;

David

Re: [RFC] tcp md5 use of alloc_percpu

[Eric Dumazet]

On Wed, 2014-10-22 at 22:40 -0600, David Ahern wrote:
> On 10/22/14, 12:55 PM, 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:
> 
> This is a forward port of a local change to address the problem (local 
> kernel version is 3.4 so perhaps my quick bump to top of tree is off but 
> it shows the general idea). Been on my to-do list to figure out why this 
> is needed, but it seems related to your problem:
> 
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 1bec4e76d88c..833a676bd4b0 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2941,7 +2941,7 @@ struct tcp_md5sig_pool *tcp_get_md5sig_pool(void)
>      local_bh_disable();
>      p = ACCESS_ONCE(tcp_md5sig_pool);
>      if (p)
> -       return raw_cpu_ptr(p);
> +       return __va(per_cpu_ptr_to_phys(raw_cpu_ptr(p)));
> 
>      local_bh_enable();
>      return NULL;

per_cpu_ptr_to_phys() can be pretty expensive and should not be called
in fast path.

Re: [RFC] tcp md5 use of alloc_percpu

[Eric Dumazet]

On Wed, 2014-10-22 at 22:23 -0700, Eric Dumazet wrote:
> On Wed, 2014-10-22 at 22:40 -0600, David Ahern wrote:
> > On 10/22/14, 12:55 PM, 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:
> > 
> > This is a forward port of a local change to address the problem (local 
> > kernel version is 3.4 so perhaps my quick bump to top of tree is off but 
> > it shows the general idea). Been on my to-do list to figure out why this 
> > is needed, but it seems related to your problem:
> > 
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index 1bec4e76d88c..833a676bd4b0 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -2941,7 +2941,7 @@ struct tcp_md5sig_pool *tcp_get_md5sig_pool(void)
> >      local_bh_disable();
> >      p = ACCESS_ONCE(tcp_md5sig_pool);
> >      if (p)
> > -       return raw_cpu_ptr(p);
> > +       return __va(per_cpu_ptr_to_phys(raw_cpu_ptr(p)));
> > 
> >      local_bh_enable();
> >      return NULL;
> 
> per_cpu_ptr_to_phys() can be pretty expensive and should not be called
> in fast path.
> 

My updated patch would be :

 net/ipv4/tcp.c |   66 +++++++++++++++++++----------------------------
 1 file changed, 28 insertions(+), 38 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 1bec4e76d88c..af4dc16b61f6 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2868,61 +2868,51 @@ EXPORT_SYMBOL(compat_tcp_getsockopt);
 #endif
 
 #ifdef CONFIG_TCP_MD5SIG
-static struct tcp_md5sig_pool __percpu *tcp_md5sig_pool __read_mostly;
+static DEFINE_PER_CPU(struct tcp_md5sig_pool, *tcp_md5sig_pool);
 static DEFINE_MUTEX(tcp_md5sig_mutex);
-
-static void __tcp_free_md5sig_pool(struct tcp_md5sig_pool __percpu *pool)
-{
-	int cpu;
-
-	for_each_possible_cpu(cpu) {
-		struct tcp_md5sig_pool *p = per_cpu_ptr(pool, cpu);
-
-		if (p->md5_desc.tfm)
-			crypto_free_hash(p->md5_desc.tfm);
-	}
-	free_percpu(pool);
-}
+static bool tcp_md5sig_pool_populated = false;
 
 static void __tcp_alloc_md5sig_pool(void)
 {
 	int cpu;
-	struct tcp_md5sig_pool __percpu *pool;
-
-	pool = alloc_percpu(struct tcp_md5sig_pool);
-	if (!pool)
-		return;
 
 	for_each_possible_cpu(cpu) {
+		struct tcp_md5sig_pool *pool;
 		struct crypto_hash *hash;
 
-		hash = crypto_alloc_hash("md5", 0, CRYPTO_ALG_ASYNC);
-		if (IS_ERR_OR_NULL(hash))
-			goto out_free;
-
-		per_cpu_ptr(pool, cpu)->md5_desc.tfm = hash;
+		pool = per_cpu(tcp_md5sig_pool, cpu);
+		if (!pool) {
+			pool = kzalloc_node(sizeof(*pool), GFP_KERNEL,
+					    cpu_to_node(cpu));
+			if (!pool)
+				return;
+			per_cpu(tcp_md5sig_pool, cpu) = pool;
+		}
+		if (!pool->md5_desc.tfm) {
+			hash = crypto_alloc_hash("md5", 0, CRYPTO_ALG_ASYNC);
+			if (IS_ERR_OR_NULL(hash))
+				return;
+			pool->md5_desc.tfm = hash;
+		}
 	}
-	/* before setting tcp_md5sig_pool, we must commit all writes
-	 * to memory. See ACCESS_ONCE() in tcp_get_md5sig_pool()
+	/* before setting tcp_md5sig_pool_populated, we must commit all writes
+	 * to memory. See smp_rmb() in tcp_get_md5sig_pool()
 	 */
 	smp_wmb();
-	tcp_md5sig_pool = pool;
-	return;
-out_free:
-	__tcp_free_md5sig_pool(pool);
+	tcp_md5sig_pool_populated = true;
 }
 
 bool tcp_alloc_md5sig_pool(void)
 {
-	if (unlikely(!tcp_md5sig_pool)) {
+	if (unlikely(!tcp_md5sig_pool_populated)) {
 		mutex_lock(&tcp_md5sig_mutex);
 
-		if (!tcp_md5sig_pool)
+		if (!tcp_md5sig_pool_populated)
 			__tcp_alloc_md5sig_pool();
 
 		mutex_unlock(&tcp_md5sig_mutex);
 	}
-	return tcp_md5sig_pool != NULL;
+	return tcp_md5sig_pool_populated;
 }
 EXPORT_SYMBOL(tcp_alloc_md5sig_pool);
 
@@ -2936,13 +2926,13 @@ EXPORT_SYMBOL(tcp_alloc_md5sig_pool);
  */
 struct tcp_md5sig_pool *tcp_get_md5sig_pool(void)
 {
-	struct tcp_md5sig_pool __percpu *p;
-
 	local_bh_disable();
-	p = ACCESS_ONCE(tcp_md5sig_pool);
-	if (p)
-		return raw_cpu_ptr(p);
 
+	if (tcp_md5sig_pool_populated) {
+		/* coupled with smp_wmb() in __tcp_alloc_md5sig_pool */
+		smp_rmb();
+		return this_cpu_read(tcp_md5sig_pool);
+	}
 	local_bh_enable();
 	return NULL;
 }

Re: [RFC] tcp md5 use of alloc_percpu

[Jonathan Toppins]

On 10/23/14, 1:38 AM, Eric Dumazet wrote:
> On Wed, 2014-10-22 at 22:23 -0700, Eric Dumazet wrote:
>> On Wed, 2014-10-22 at 22:40 -0600, David Ahern wrote:
>>> On 10/22/14, 12:55 PM, 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:
>>>
>>> This is a forward port of a local change to address the problem (local 
>>> kernel version is 3.4 so perhaps my quick bump to top of tree is off but 
>>> it shows the general idea). Been on my to-do list to figure out why this 
>>> is needed, but it seems related to your problem:
>>>
>>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
>>> index 1bec4e76d88c..833a676bd4b0 100644
>>> --- a/net/ipv4/tcp.c
>>> +++ b/net/ipv4/tcp.c
>>> @@ -2941,7 +2941,7 @@ struct tcp_md5sig_pool *tcp_get_md5sig_pool(void)
>>>      local_bh_disable();
>>>      p = ACCESS_ONCE(tcp_md5sig_pool);
>>>      if (p)
>>> -       return raw_cpu_ptr(p);
>>> +       return __va(per_cpu_ptr_to_phys(raw_cpu_ptr(p)));
>>>
>>>      local_bh_enable();
>>>      return NULL;
>>
>> per_cpu_ptr_to_phys() can be pretty expensive and should not be called
>> in fast path.
>>
> 
> My updated patch would be :
> 
>  net/ipv4/tcp.c |   66 +++++++++++++++++++----------------------------
>  1 file changed, 28 insertions(+), 38 deletions(-)
> 
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 1bec4e76d88c..af4dc16b61f6 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2868,61 +2868,51 @@ EXPORT_SYMBOL(compat_tcp_getsockopt);
>  #endif
>  
>  #ifdef CONFIG_TCP_MD5SIG
> -static struct tcp_md5sig_pool __percpu *tcp_md5sig_pool __read_mostly;
> +static DEFINE_PER_CPU(struct tcp_md5sig_pool, *tcp_md5sig_pool);
>  static DEFINE_MUTEX(tcp_md5sig_mutex);
> -
> -static void __tcp_free_md5sig_pool(struct tcp_md5sig_pool __percpu *pool)
> -{
> -	int cpu;
> -
> -	for_each_possible_cpu(cpu) {
> -		struct tcp_md5sig_pool *p = per_cpu_ptr(pool, cpu);
> -
> -		if (p->md5_desc.tfm)
> -			crypto_free_hash(p->md5_desc.tfm);
> -	}
> -	free_percpu(pool);
> -}
> +static bool tcp_md5sig_pool_populated = false;
>  
>  static void __tcp_alloc_md5sig_pool(void)
>  {
>  	int cpu;
> -	struct tcp_md5sig_pool __percpu *pool;
> -
> -	pool = alloc_percpu(struct tcp_md5sig_pool);
> -	if (!pool)
> -		return;
>  
>  	for_each_possible_cpu(cpu) {
> +		struct tcp_md5sig_pool *pool;
>  		struct crypto_hash *hash;
>  
> -		hash = crypto_alloc_hash("md5", 0, CRYPTO_ALG_ASYNC);
> -		if (IS_ERR_OR_NULL(hash))
> -			goto out_free;
> -
> -		per_cpu_ptr(pool, cpu)->md5_desc.tfm = hash;
> +		pool = per_cpu(tcp_md5sig_pool, cpu);
> +		if (!pool) {
> +			pool = kzalloc_node(sizeof(*pool), GFP_KERNEL,
GFP_DMA | GFP_KERNEL
This memory will possibly be used in a DMA correct? (thinking crypto
hardware offload)
> +					    cpu_to_node(cpu));
> +			if (!pool)
> +				return;
> +			per_cpu(tcp_md5sig_pool, cpu) = pool;
> +		}
> +		if (!pool->md5_desc.tfm) {
> +			hash = crypto_alloc_hash("md5", 0, CRYPTO_ALG_ASYNC);
> +			if (IS_ERR_OR_NULL(hash))
> +				return;
> +			pool->md5_desc.tfm = hash;
> +		}
>  	}
> -	/* before setting tcp_md5sig_pool, we must commit all writes
> -	 * to memory. See ACCESS_ONCE() in tcp_get_md5sig_pool()
> +	/* before setting tcp_md5sig_pool_populated, we must commit all writes
> +	 * to memory. See smp_rmb() in tcp_get_md5sig_pool()
>  	 */
>  	smp_wmb();
> -	tcp_md5sig_pool = pool;
> -	return;
> -out_free:
> -	__tcp_free_md5sig_pool(pool);
> +	tcp_md5sig_pool_populated = true;
>  }
>  
>  bool tcp_alloc_md5sig_pool(void)
>  {
> -	if (unlikely(!tcp_md5sig_pool)) {
> +	if (unlikely(!tcp_md5sig_pool_populated)) {
>  		mutex_lock(&tcp_md5sig_mutex);
>  
> -		if (!tcp_md5sig_pool)
> +		if (!tcp_md5sig_pool_populated)
>  			__tcp_alloc_md5sig_pool();
>  
>  		mutex_unlock(&tcp_md5sig_mutex);
>  	}
> -	return tcp_md5sig_pool != NULL;
> +	return tcp_md5sig_pool_populated;
>  }
>  EXPORT_SYMBOL(tcp_alloc_md5sig_pool);
>  
> @@ -2936,13 +2926,13 @@ EXPORT_SYMBOL(tcp_alloc_md5sig_pool);
>   */
>  struct tcp_md5sig_pool *tcp_get_md5sig_pool(void)
>  {
> -	struct tcp_md5sig_pool __percpu *p;
> -
>  	local_bh_disable();
> -	p = ACCESS_ONCE(tcp_md5sig_pool);
> -	if (p)
> -		return raw_cpu_ptr(p);
>  
> +	if (tcp_md5sig_pool_populated) {
> +		/* coupled with smp_wmb() in __tcp_alloc_md5sig_pool */
> +		smp_rmb();
> +		return this_cpu_read(tcp_md5sig_pool);
> +	}
>  	local_bh_enable();
>  	return NULL;
>  }
> 
> 
> 
> --
> 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
> 

Re: [RFC] tcp md5 use of alloc_percpu

[Crestez Dan Leonard]

On 10/23/2014 08:38 AM, Eric Dumazet wrote:
> On Wed, 2014-10-22 at 22:23 -0700, Eric Dumazet wrote:
> My updated patch would be :
> 
>  net/ipv4/tcp.c |   66 +++++++++++++++++++----------------------------
>  1 file changed, 28 insertions(+), 38 deletions(-)
> 
I can confirm that this patch fixes my original issue.

I am working with a kernel based on 3.10 so I had to integrate 71cea17ed39fdf1c0634f530ddc6a2c2fc601c2b as well.

Regards,
Leonard

Re: [RFC] tcp md5 use of alloc_percpu

[Eric Dumazet]

On Thu, 2014-10-23 at 02:58 -0400, Jonathan Toppins wrote:

> > +		if (!pool) {
> > +			pool = kzalloc_node(sizeof(*pool), GFP_KERNEL,
> GFP_DMA | GFP_KERNEL
> This memory will possibly be used in a DMA correct? (thinking crypto
> hardware offload)

I am not sure this is the case, but this certainly can be added.

Re: [RFC] tcp md5 use of alloc_percpu

[Eric Dumazet]

On Thu, 2014-10-23 at 06:21 -0700, Eric Dumazet wrote:
> On Thu, 2014-10-23 at 02:58 -0400, Jonathan Toppins wrote:
> 
> > > +		if (!pool) {
> > > +			pool = kzalloc_node(sizeof(*pool), GFP_KERNEL,
> > GFP_DMA | GFP_KERNEL
> > This memory will possibly be used in a DMA correct? (thinking crypto
> > hardware offload)
> 
> I am not sure this is the case, but this certainly can be added.
> 

Yes, this is not the case.


The real problem is because sg_set_buf() does the following :


sg_set_page(sg, virt_to_page(buf), buflen, offset_in_page(buf));

So it assumes a memory range is not spanning multiple pages.

So maybe a simpler patch would be :


diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 1bec4e76d88c5852d8ba3392b22aa58d6453ab4d..53e355199437b00836635c5919f2f1a1ae237271 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2886,10 +2886,17 @@ static void __tcp_free_md5sig_pool(struct tcp_md5sig_pool __percpu *pool)
 
 static void __tcp_alloc_md5sig_pool(void)
 {
-	int cpu;
 	struct tcp_md5sig_pool __percpu *pool;
+	size_t sz;
+	int cpu;
 
-	pool = alloc_percpu(struct tcp_md5sig_pool);
+	/* sg_set_buf() assumes a contiguous memory area, but alloc_percpu()
+	 * can use vmalloc(). So make sure we ask an alignment so that
+	 * tcp_md5sig_pool is located in a single page.
+	 */
+	BUILD_BUG_ON(sizeof(struct tcp_md5sig_pool) > PAGE_SIZE);
+	sz = roundup_pow_of_two(sizeof(struct tcp_md5sig_pool));
+	pool = __alloc_percpu(sz, sz);
 	if (!pool)
 		return;
 

Re: [RFC] tcp md5 use of alloc_percpu

[Crestez Dan Leonard]

On Thu, Oct 23, 2014 at 4:21 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2014-10-23 at 02:58 -0400, Jonathan Toppins wrote:
>
>> > +           if (!pool) {
>> > +                   pool = kzalloc_node(sizeof(*pool), GFP_KERNEL,
>> GFP_DMA | GFP_KERNEL
>> This memory will possibly be used in a DMA correct? (thinking crypto
>> hardware offload)
>
> I am not sure this is the case, but this certainly can be added.
As far as I know what GFP_DMA actually does is restrict allocation to
low memory addresses under 24 bits for very old devices. There is also
GFP_DMA32 which restricts to addresses under 32 bytes (for device
which don't support 64 bit addresses).

This kind of stuff only belongs in device drivers where the exact
hardware limitations are known. I don't think crypto devices with this
kind of limitations can be exposed through the crypto API that the
md5sig feature uses.

--
Regards,
Leonard

Re: [RFC] tcp md5 use of alloc_percpu

[Crestez Dan Leonard]

On 10/23/2014 05:43 PM, Eric Dumazet wrote:
> On Thu, 2014-10-23 at 06:21 -0700, Eric Dumazet wrote:
>> On Thu, 2014-10-23 at 02:58 -0400, Jonathan Toppins wrote:
>>
>>>> +		if (!pool) {
>>>> +			pool = kzalloc_node(sizeof(*pool), GFP_KERNEL,
>>> GFP_DMA | GFP_KERNEL
>>> This memory will possibly be used in a DMA correct? (thinking crypto
>>> hardware offload)
>>
>> I am not sure this is the case, but this certainly can be added.
>>
> 
> Yes, this is not the case.
> 
> 
> The real problem is because sg_set_buf() does the following :
> sg_set_page(sg, virt_to_page(buf), buflen, offset_in_page(buf));
> 
> So it assumes a memory range is not spanning multiple pages.
Doesn't virt_to_page also assume that the memory is not from vmalloc?

Making this portable would require checking if is_vmalloc_addr and doing
vmalloc_to_page instead. Easier to just kmalloc instead.

Regards,
Leonard

[PATCH net] tcp: md5: percpu tcp_md5sig_pool must not span pages

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

percpu tcp_md5sig_pool contains memory blobs that ultimately
go through sg_set_buf().

-> sg_set_page(sg, virt_to_page(buf), buflen, offset_in_page(buf));

This requires that whole area is in a physically contiguous portion
of memory.

Given that alloc_percpu() can use vmalloc() areas, we need to make sure
tcp_md5sig_pool is allocated from a single page.


Signed-off-by: Eric Dumazet <edumazet@google.com>
Fixes: 765cf9976e93 ("tcp: md5: remove one indirection level in tcp_md5sig_pool")
Reported-by: Crestez Dan Leonard <cdleonard@gmail.com>
---

Jonathan, David and Crestez, please test this patch fixes
the issue for good. We might in future kernels avoid the dynamic
memory allocations, but a simple fix for stable kernels is better IMO.

Thanks !

 net/ipv4/tcp.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 1bec4e76d88c5852d8ba3392b22aa58d6453ab4d..1f59e4130db99f129279c13f89b3c058ed6167e2 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2886,10 +2886,17 @@ static void __tcp_free_md5sig_pool(struct tcp_md5sig_pool __percpu *pool)
 
 static void __tcp_alloc_md5sig_pool(void)
 {
-	int cpu;
 	struct tcp_md5sig_pool __percpu *pool;
+	size_t align;
+	int cpu;
 
-	pool = alloc_percpu(struct tcp_md5sig_pool);
+	/* sg_set_buf() assumes a contiguous memory area, but alloc_percpu()
+	 * can use vmalloc(). So make sure we request an alignment so that
+	 * whole tcp_md5sig_pool is contained in a single page.
+	 */
+	BUILD_BUG_ON(sizeof(struct tcp_md5sig_pool) > PAGE_SIZE);
+	align = roundup_pow_of_two(sizeof(struct tcp_md5sig_pool));
+	pool = __alloc_percpu(sizeof(struct tcp_md5sig_pool), align);
 	if (!pool)
 		return;
 

Re: [RFC] tcp md5 use of alloc_percpu

[Eric Dumazet]

On Thu, 2014-10-23 at 19:17 +0300, Crestez Dan Leonard wrote:

> Doesn't virt_to_page also assume that the memory is not from vmalloc?
> 
> Making this portable would require checking if is_vmalloc_addr and doing
> vmalloc_to_page instead. Easier to just kmalloc instead.

Oh right, I'll send a v2

Thanks !

Re: [PATCH net] tcp: md5: percpu tcp_md5sig_pool must not span pages

[Eric Dumazet]

On Thu, 2014-10-23 at 09:33 -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> percpu tcp_md5sig_pool contains memory blobs that ultimately
> go through sg_set_buf().
> 
> -> sg_set_page(sg, virt_to_page(buf), buflen, offset_in_page(buf));
> 
> This requires that whole area is in a physically contiguous portion
> of memory.
> 
> Given that alloc_percpu() can use vmalloc() areas, we need to make sure
> tcp_md5sig_pool is allocated from a single page.
> 
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Fixes: 765cf9976e93 ("tcp: md5: remove one indirection level in tcp_md5sig_pool")
> Reported-by: Crestez Dan Leonard <cdleonard@gmail.com>
> ---

Self Nack, we definitely need to avoid vmalloc() in the first place.

[PATCH v2 net] tcp: md5: do not use alloc_percpu()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

percpu tcp_md5sig_pool contains memory blobs that ultimately
go through sg_set_buf().

-> sg_set_page(sg, virt_to_page(buf), buflen, offset_in_page(buf));

This requires that whole area is in a physically contiguous portion
of memory. And that @buf is not backed by vmalloc().

Given that alloc_percpu() can use vmalloc() areas, this does not
fit the requirements.

Replace alloc_percpu() by a static DEFINE_PER_CPU() as tcp_md5sig_pool
is small anyway, there is no gain to dynamically allocate it.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Fixes: 765cf9976e93 ("tcp: md5: remove one indirection level in tcp_md5sig_pool")
Reported-by: Crestez Dan Leonard <cdleonard@gmail.com>
---
 net/ipv4/tcp.c |   59 +++++++++++++++--------------------------------
 1 file changed, 20 insertions(+), 39 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 1bec4e76d88c..39ec0c379545 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2868,61 +2868,42 @@ EXPORT_SYMBOL(compat_tcp_getsockopt);
 #endif
 
 #ifdef CONFIG_TCP_MD5SIG
-static struct tcp_md5sig_pool __percpu *tcp_md5sig_pool __read_mostly;
+static DEFINE_PER_CPU(struct tcp_md5sig_pool, tcp_md5sig_pool);
 static DEFINE_MUTEX(tcp_md5sig_mutex);
-
-static void __tcp_free_md5sig_pool(struct tcp_md5sig_pool __percpu *pool)
-{
-	int cpu;
-
-	for_each_possible_cpu(cpu) {
-		struct tcp_md5sig_pool *p = per_cpu_ptr(pool, cpu);
-
-		if (p->md5_desc.tfm)
-			crypto_free_hash(p->md5_desc.tfm);
-	}
-	free_percpu(pool);
-}
+static bool tcp_md5sig_pool_populated = false;
 
 static void __tcp_alloc_md5sig_pool(void)
 {
 	int cpu;
-	struct tcp_md5sig_pool __percpu *pool;
-
-	pool = alloc_percpu(struct tcp_md5sig_pool);
-	if (!pool)
-		return;
 
 	for_each_possible_cpu(cpu) {
-		struct crypto_hash *hash;
-
-		hash = crypto_alloc_hash("md5", 0, CRYPTO_ALG_ASYNC);
-		if (IS_ERR_OR_NULL(hash))
-			goto out_free;
+		if (!per_cpu(tcp_md5sig_pool, cpu).md5_desc.tfm) {
+			struct crypto_hash *hash;
 
-		per_cpu_ptr(pool, cpu)->md5_desc.tfm = hash;
+			hash = crypto_alloc_hash("md5", 0, CRYPTO_ALG_ASYNC);
+			if (IS_ERR_OR_NULL(hash))
+				return;
+			per_cpu(tcp_md5sig_pool, cpu).md5_desc.tfm = hash;
+		}
 	}
-	/* before setting tcp_md5sig_pool, we must commit all writes
-	 * to memory. See ACCESS_ONCE() in tcp_get_md5sig_pool()
+	/* before setting tcp_md5sig_pool_populated, we must commit all writes
+	 * to memory. See smp_rmb() in tcp_get_md5sig_pool()
 	 */
 	smp_wmb();
-	tcp_md5sig_pool = pool;
-	return;
-out_free:
-	__tcp_free_md5sig_pool(pool);
+	tcp_md5sig_pool_populated = true;
 }
 
 bool tcp_alloc_md5sig_pool(void)
 {
-	if (unlikely(!tcp_md5sig_pool)) {
+	if (unlikely(!tcp_md5sig_pool_populated)) {
 		mutex_lock(&tcp_md5sig_mutex);
 
-		if (!tcp_md5sig_pool)
+		if (!tcp_md5sig_pool_populated)
 			__tcp_alloc_md5sig_pool();
 
 		mutex_unlock(&tcp_md5sig_mutex);
 	}
-	return tcp_md5sig_pool != NULL;
+	return tcp_md5sig_pool_populated;
 }
 EXPORT_SYMBOL(tcp_alloc_md5sig_pool);
 
@@ -2936,13 +2917,13 @@ EXPORT_SYMBOL(tcp_alloc_md5sig_pool);
  */
 struct tcp_md5sig_pool *tcp_get_md5sig_pool(void)
 {
-	struct tcp_md5sig_pool __percpu *p;
-
 	local_bh_disable();
-	p = ACCESS_ONCE(tcp_md5sig_pool);
-	if (p)
-		return raw_cpu_ptr(p);
 
+	if (tcp_md5sig_pool_populated) {
+		/* coupled with smp_wmb() in __tcp_alloc_md5sig_pool() */
+		smp_rmb();
+		return this_cpu_ptr(&tcp_md5sig_pool);
+	}
 	local_bh_enable();
 	return NULL;
 }

Re: [PATCH v2 net] tcp: md5: do not use alloc_percpu()

[David Ahern]

On 10/23/14, 1:58 PM, Eric Dumazet wrote:
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 1bec4e76d88c..39ec0c379545 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2868,61 +2868,42 @@ EXPORT_SYMBOL(compat_tcp_getsockopt);
>   #endif
>
>   #ifdef CONFIG_TCP_MD5SIG
> -static struct tcp_md5sig_pool __percpu *tcp_md5sig_pool __read_mostly;
> +static DEFINE_PER_CPU(struct tcp_md5sig_pool, tcp_md5sig_pool);
>   static DEFINE_MUTEX(tcp_md5sig_mutex);
> -
> -static void __tcp_free_md5sig_pool(struct tcp_md5sig_pool __percpu *pool)
> -{
> -	int cpu;
> -
> -	for_each_possible_cpu(cpu) {
> -		struct tcp_md5sig_pool *p = per_cpu_ptr(pool, cpu);
> -
> -		if (p->md5_desc.tfm)
> -			crypto_free_hash(p->md5_desc.tfm);
> -	}
> -	free_percpu(pool);
> -}
> +static bool tcp_md5sig_pool_populated = false;

global variables do not need to be initialized to 0.

I'll see how this applies to v3.4 and build an image.

Thanks,
David

Re: [PATCH v2 net] tcp: md5: do not use alloc_percpu()

[Eric Dumazet]

On Thu, 2014-10-23 at 14:44 -0600, David Ahern wrote:

> global variables do not need to be initialized to 0.
> 

Compilers are generating the same code nowadays.
They place such zero variables in bss anyway.
They finally got this right, maybe years ago.


$ nm -v net/ipv4/tcp.o | grep populated
0000000000000078 b tcp_md5sig_pool_populated

Re: [PATCH v2 net] tcp: md5: do not use alloc_percpu()

[David Ahern]

On 10/23/14, 4:57 PM, Eric Dumazet wrote:
> On Thu, 2014-10-23 at 14:44 -0600, David Ahern wrote:
>
>> global variables do not need to be initialized to 0.
>>
>
> Compilers are generating the same code nowadays.
> They place such zero variables in bss anyway.
> They finally got this right, maybe years ago.
>
>
> $ nm -v net/ipv4/tcp.o | grep populated
> 0000000000000078 b tcp_md5sig_pool_populated

Right. I was referring to coding standards. As I recall checkpatch 
throws a warning about it.

David

Re: [PATCH v2 net] tcp: md5: do not use alloc_percpu()

[David Ahern]

On 10/23/14, 1:58 PM, Eric Dumazet wrote:
> From: Eric Dumazet<edumazet@google.com>
>
> percpu tcp_md5sig_pool contains memory blobs that ultimately
> go through sg_set_buf().
>
> -> sg_set_page(sg, virt_to_page(buf), buflen, offset_in_page(buf));
>
> This requires that whole area is in a physically contiguous portion
> of memory. And that @buf is not backed by vmalloc().
>
> Given that alloc_percpu() can use vmalloc() areas, this does not
> fit the requirements.
>
> Replace alloc_percpu() by a static DEFINE_PER_CPU() as tcp_md5sig_pool
> is small anyway, there is no gain to dynamically allocate it.
>
> Signed-off-by: Eric Dumazet<edumazet@google.com>
> Fixes: 765cf9976e93 ("tcp: md5: remove one indirection level in tcp_md5sig_pool")
> Reported-by: Crestez Dan Leonard<cdleonard@gmail.com>

Too much time (and too many changes) has passed for this to apply easily 
to v3.4 kernels.

David

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

Re: [PATCH v2 net] tcp: md5: do not use alloc_percpu()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 23 Oct 2014 12:58:58 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> percpu tcp_md5sig_pool contains memory blobs that ultimately
> go through sg_set_buf().
> 
> -> sg_set_page(sg, virt_to_page(buf), buflen, offset_in_page(buf));
> 
> This requires that whole area is in a physically contiguous portion
> of memory. And that @buf is not backed by vmalloc().
> 
> Given that alloc_percpu() can use vmalloc() areas, this does not
> fit the requirements.
> 
> Replace alloc_percpu() by a static DEFINE_PER_CPU() as tcp_md5sig_pool
> is small anyway, there is no gain to dynamically allocate it.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Fixes: 765cf9976e93 ("tcp: md5: remove one indirection level in tcp_md5sig_pool")
> Reported-by: Crestez Dan Leonard <cdleonard@gmail.com>

Applied and queued up for -stable, thanks Eric.

Longer term we should do a proper shash/ahash conversion of the tcp md5
code, using the rules provided by Herbert Xu.

Thanks.