tcp md5 use of alloc_percpu

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:

Quoted 30 lines · 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));
}

Quoted 5 lines

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.

Quoted 36 lines · 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.

Quoted 7 lines

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

Quoted 2 lines

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

Quoted 4 lines

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.

Quoted 3 lines

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:

static int tcp_v4_md5_hash_pseudoheader(struct tcp_md5sig_pool *hp,

net/ipv4/tcp_ipv4.c +1 -1

 					__be32 daddr, __be32 saddr, int nbytes)
 {
-	struct tcp4_pseudohdr *bp;
+	struct tcp4_pseudohdr 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.

Quoted 1 line · Crestez Dan Leonard

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?

Quoted 7 lines · Jonathan Toppins

Quoted 1 line · Crestez Dan Leonard

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.

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:

Quoted 3 lines · 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:

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:

net/ipv4/tcp.c +0 -0

index 1bec4e76d88c..833a676bd4b0 100644

Quoted 21 lines · David Ahern

Quoted 3 lines · 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:

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:

net/ipv4/tcp.c +1 -1

index 1bec4e76d88c..833a676bd4b0 100644
@@ -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)));
 

net/ipv4/tcp.c struct tcp_md5sig_pool *tcp_get_md5sig_pool(void) · line 2947

Quoted 2 lines

net/ipv4/tcp.c +0 -0

     local_bh_enable();
     return NULL;

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

Quoted 22 lines · Eric Dumazet

Quoted 21 lines · David Ahern

Quoted 3 lines · 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:

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:

net/ipv4/tcp.c +1 -1

index 1bec4e76d88c..833a676bd4b0 100644
@@ -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)));
 

Quoted 6 lines

net/ipv4/tcp.c EXPORT_SYMBOL(tcp_alloc_md5sig_pool); · line 2937

Quoted 2 lines

net/ipv4/tcp.c +0 -0

     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(-)

net/ipv4/tcp.c +3 -19

index 1bec4e76d88c..af4dc16b61f6 100644
@@ -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;

net/ipv4/tcp.c +22 -15

+					    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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

net/ipv4/tcp.c EXPORT_SYMBOL(compat_tcp_getsockopt); · line 2885

Quoted 2 lines

net/ipv4/tcp.c +2 -0

+		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)

Quoted 5 lines · Jonathan Toppins

Review

Quoted 2 lines

(quoted diff) +2 -0

+		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.

Quoted 10 lines · Eric Dumazet

Quoted 5 lines · Jonathan Toppins

Review

Quoted 2 lines

(quoted diff) +2 -0

+		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.

From: Eric Dumazet <[email protected]>

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 <[email protected]>

Fixes 765cf9976e93 ("tcp: md5: remove one indirection level in tcp_md5sig_pool")

Reported-by Crestez Dan Leonard <[email protected]>

---

net/ipv4/tcp.c +1 -5

index 1bec4e76d88c..39ec0c379545 100644
@@ -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;

global variables do not need to be initialized to 0.

Quoted 2 lines · David Ahern

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

From: Eric Dumazet<[email protected]>

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<[email protected]>

Fixes 765cf9976e93 ("tcp: md5: remove one indirection level in tcp_md5sig_pool")

Reported-by Crestez Dan Leonard<[email protected]>

From: Eric Dumazet <[email protected]>

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 <[email protected]>

Fixes 765cf9976e93 ("tcp: md5: remove one indirection level in tcp_md5sig_pool")

Reported-by Crestez Dan Leonard <[email protected]>

Quoted 5 lines · Jonathan Toppins

net/ipv4/tcp.c EXPORT_SYMBOL(compat_tcp_getsockopt); · line 2885

Quoted 2 lines

net/ipv4/tcp.c +2 -0

+           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.

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(-)