[PATCH] netfilter: use krealloc() in nf_conntrack_extend.c

[PATCH] netfilter: use krealloc() in nf_conntrack_extend.c

[Pekka J Enberg]

From: Pekka Enberg <penberg@cs.helsinki.fi>

The ksize() API is going away because it is being abused and it doesn't even
work consistenly across different allocators. Therefore, convert the use 
of ksize() in net/netfilter/nf_conntrack_extend.c to krealloc() that is 
open-coded there.

Cc: Matt Mackall <mpm@selenic.com>
Cc: Christoph Lameter <clameter@sgi.com>
Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
 net/netfilter/nf_conntrack_extend.c |   31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

Index: slab-2.6/net/netfilter/nf_conntrack_extend.c
===================================================================
--- slab-2.6.orig/net/netfilter/nf_conntrack_extend.c	2008-05-22 21:38:14.000000000 +0300
+++ slab-2.6/net/netfilter/nf_conntrack_extend.c	2008-05-22 21:38:21.000000000 +0300
@@ -88,27 +88,22 @@
 	newlen = newoff + t->len;
 	rcu_read_unlock();
 
-	if (newlen >= ksize(ct->ext)) {
-		new = kmalloc(newlen, gfp);
-		if (!new)
-			return NULL;
+	new = krealloc(ct->ext, newlen, gfp);
+	if (!new)
+		return NULL;
 
-		memcpy(new, ct->ext, ct->ext->len);
-
-		for (i = 0; i < NF_CT_EXT_NUM; i++) {
-			if (!nf_ct_ext_exist(ct, i))
-				continue;
+	for (i = 0; i < NF_CT_EXT_NUM; i++) {
+		if (!nf_ct_ext_exist(ct, i))
+			continue;
 
-			rcu_read_lock();
-			t = rcu_dereference(nf_ct_ext_types[i]);
-			if (t && t->move)
-				t->move((void *)new + new->offset[i],
-					(void *)ct->ext + ct->ext->offset[i]);
-			rcu_read_unlock();
-		}
-		kfree(ct->ext);
-		ct->ext = new;
+		rcu_read_lock();
+		t = rcu_dereference(nf_ct_ext_types[i]);
+		if (t && t->move)
+			t->move((void *)new + new->offset[i],
+				(void *)ct->ext + ct->ext->offset[i]);
+		rcu_read_unlock();
 	}
+	ct->ext = new;
 
 	ct->ext->offset[id] = newoff;
 	ct->ext->len = newlen;

[PATCH] netfilter: use krealloc() in nf_conntrack_extend.c V2

[Pekka J Enberg]

To: kaber@trash.net
[PATCH] netfilter: use krealloc() in nf_conntrack_extend.c V2
From: Pekka Enberg <penberg@cs.helsinki.fi>

The ksize() API is going away because it is being abused and it doesn't even
work consistenly across different allocators. Therefore, convert
net/netfilter/nf_conntrack_extend.c to use krealloc().

Cc: <netfilter-devel@vger.kernel.org>
Cc: <netdev@vger.kernel.org>
Cc: Matt Mackall <mpm@selenic.com>
Cc: Christoph Lameter <clameter@sgi.com>
Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
Patrick, please use this patch instead. The previous one did the moving
unconditionally which is wrong. This one moves entries around only if
krealloc() allocated a new buffer.

 net/netfilter/nf_conntrack_extend.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

Index: slab-2.6/net/netfilter/nf_conntrack_extend.c
===================================================================
--- slab-2.6.orig/net/netfilter/nf_conntrack_extend.c	2008-05-22 21:52:12.000000000 +0300
+++ slab-2.6/net/netfilter/nf_conntrack_extend.c	2008-05-22 21:52:26.000000000 +0300
@@ -88,13 +88,11 @@
 	newlen = newoff + t->len;
 	rcu_read_unlock();
 
-	if (newlen >= ksize(ct->ext)) {
-		new = kmalloc(newlen, gfp);
-		if (!new)
-			return NULL;
-
-		memcpy(new, ct->ext, ct->ext->len);
+	new = krealloc(ct->ext, newlen, gfp);
+	if (!new)
+		return NULL;
 
+	if (new != ct->ext) {
 		for (i = 0; i < NF_CT_EXT_NUM; i++) {
 			if (!nf_ct_ext_exist(ct, i))
 				continue;

Re: [PATCH] netfilter: use krealloc() in nf_conntrack_extend.c V2

[Patrick McHardy]

Pekka J Enberg wrote:
> The ksize() API is going away because it is being abused and it doesn't even
> work consistenly across different allocators. Therefore, convert
> net/netfilter/nf_conntrack_extend.c to use krealloc().
>
> Cc: <netfilter-devel@vger.kernel.org>
> Cc: <netdev@vger.kernel.org>
> Cc: Matt Mackall <mpm@selenic.com>
> Cc: Christoph Lameter <clameter@sgi.com>
> Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
> ---
> Patrick, please use this patch instead. The previous one did the moving
> unconditionally which is wrong. This one moves entries around only if
> krealloc() allocated a new buffer.
>
>  net/netfilter/nf_conntrack_extend.c |   10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> Index: slab-2.6/net/netfilter/nf_conntrack_extend.c
> ===================================================================
> --- slab-2.6.orig/net/netfilter/nf_conntrack_extend.c	2008-05-22 21:52:12.000000000 +0300
> +++ slab-2.6/net/netfilter/nf_conntrack_extend.c	2008-05-22 21:52:26.000000000 +0300
> @@ -88,13 +88,11 @@
>  	newlen = newoff + t->len;
>  	rcu_read_unlock();
>  
> -	if (newlen >= ksize(ct->ext)) {
> -		new = kmalloc(newlen, gfp);
> -		if (!new)
> -			return NULL;
> -
> -		memcpy(new, ct->ext, ct->ext->len);
> +	new = krealloc(ct->ext, newlen, gfp);

Unfortunately this means we'll always have to reallocate, even
if there's still some room left from the previous allocation.
Any chance to avoid that?

Re: [PATCH] netfilter: use krealloc() in nf_conntrack_extend.c V2

[Pekka Enberg]

Patrick McHardy wrote:
> Unfortunately this means we'll always have to reallocate, even
> if there's still some room left from the previous allocation.
> Any chance to avoid that?

No, no, krealloc() avoids reallocation if there's enough room in the 
buffer to fit newlen.

Re: [PATCH] netfilter: use krealloc() in nf_conntrack_extend.c V2

[Patrick McHardy]

Pekka Enberg wrote:
> Patrick McHardy wrote:
>> Unfortunately this means we'll always have to reallocate, even
>> if there's still some room left from the previous allocation.
>> Any chance to avoid that?
>
> No, no, krealloc() avoids reallocation if there's enough room in the 
> buffer to fit newlen.

Great, thanks for the explanation. Is this patch targeted
at 2.6.26 or 2.6.27?

Re: [PATCH] netfilter: use krealloc() in nf_conntrack_extend.c V2

[Pekka Enberg]

Patrick McHardy wrote:
> Great, thanks for the explanation. Is this patch targeted
> at 2.6.26 or 2.6.27?

I don't think we will be removing ksize() in 2.6.26 anyway so 2.6.27 is 
fine for this patch. Thanks!

		Pekka

Re: [PATCH] netfilter: use krealloc() in nf_conntrack_extend.c V2

[Patrick McHardy]

Pekka J Enberg wrote:
> To: kaber@trash.net
> [PATCH] netfilter: use krealloc() in nf_conntrack_extend.c V2
> From: Pekka Enberg <penberg@cs.helsinki.fi>
>
> The ksize() API is going away because it is being abused and it doesn't even
> work consistenly across different allocators. Therefore, convert
> net/netfilter/nf_conntrack_extend.c to use krealloc().
>
> Cc: <netfilter-devel@vger.kernel.org>
> Cc: <netdev@vger.kernel.org>
> Cc: Matt Mackall <mpm@selenic.com>
> Cc: Christoph Lameter <clameter@sgi.com>
> Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
> ---
> Patrick, please use this patch instead. The previous one did the moving
> unconditionally which is wrong. This one moves entries around only if
> krealloc() allocated a new buffer.

Applied, thanks.

Re: [PATCH] netfilter: use krealloc() in nf_conntrack_extend.c V2

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 669 bytes --]

Pekka Enberg wrote:
> Patrick McHardy wrote:
>> Great, thanks for the explanation. Is this patch targeted
>> at 2.6.26 or 2.6.27?
> 
> I don't think we will be removing ksize() in 2.6.26 anyway so 2.6.27 is 
> fine for this patch. Thanks!

Your patch introduced a use-after-free and double-free.
krealloc() frees the old pointer, but it is still used
for the ->move operations, then freed again.

To fix this I think we need a __krealloc() that doesn't
free the old memory, especially since it must not be
freed immediately because it may still be used in a RCU
read side (see the last part in the patch attached to
this mail (based on a kernel without your patch)).



[-- Attachment #2: 01.diff --]
[-- Type: text/x-diff, Size: 3306 bytes --]

netfilter: nf_nat: fix RCU races

Fix three ct_extend/NAT extension related races:

- When cleaning up the extension area and removing it from the bysource hash,
  the nat->ct pointer must not be set to NULL since it may still be used in
  a RCU read side

- When replacing a NAT extension area in the bysource hash, the nat->ct
  pointer must be assigned before performing the replacement

- When reallocating extension storage in ct_extend, the old memory must
  not be freed immediately since it may still be used by a RCU read side

Possibly fixes https://bugzilla.redhat.com/show_bug.cgi?id=449315
and/or http://bugzilla.kernel.org/show_bug.cgi?id=10875

Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit 8d4c178a5e17c19cf7a781b0e5e416c4e22b1ff2
tree 2c4651788906d120cb7636006e2178dbd7a283c4
parent ec0a196626bd12e0ba108d7daa6d95a4fb25c2c5
author Patrick McHardy <kaber@trash.net> Sat, 14 Jun 2008 12:42:45 +0200
committer Patrick McHardy <kaber@trash.net> Sat, 14 Jun 2008 12:42:45 +0200

 include/net/netfilter/nf_conntrack_extend.h |    1 +
 net/ipv4/netfilter/nf_nat_core.c            |    3 +--
 net/netfilter/nf_conntrack_extend.c         |    9 ++++++++-
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_extend.h b/include/net/netfilter/nf_conntrack_extend.h
index f736e84..f80c0ed 100644
--- a/include/net/netfilter/nf_conntrack_extend.h
+++ b/include/net/netfilter/nf_conntrack_extend.h
@@ -15,6 +15,7 @@ enum nf_ct_ext_id
 
 /* Extensions: optional stuff which isn't permanently in struct. */
 struct nf_ct_ext {
+	struct rcu_head rcu;
 	u8 offset[NF_CT_EXT_NUM];
 	u8 len;
 	char data[0];
diff --git a/net/ipv4/netfilter/nf_nat_core.c b/net/ipv4/netfilter/nf_nat_core.c
index 0457859..d2a887f 100644
--- a/net/ipv4/netfilter/nf_nat_core.c
+++ b/net/ipv4/netfilter/nf_nat_core.c
@@ -556,7 +556,6 @@ static void nf_nat_cleanup_conntrack(struct nf_conn *ct)
 
 	spin_lock_bh(&nf_nat_lock);
 	hlist_del_rcu(&nat->bysource);
-	nat->ct = NULL;
 	spin_unlock_bh(&nf_nat_lock);
 }
 
@@ -570,8 +569,8 @@ static void nf_nat_move_storage(void *new, void *old)
 		return;
 
 	spin_lock_bh(&nf_nat_lock);
-	hlist_replace_rcu(&old_nat->bysource, &new_nat->bysource);
 	new_nat->ct = ct;
+	hlist_replace_rcu(&old_nat->bysource, &new_nat->bysource);
 	spin_unlock_bh(&nf_nat_lock);
 }
 
diff --git a/net/netfilter/nf_conntrack_extend.c b/net/netfilter/nf_conntrack_extend.c
index bcc19fa..8a3f8b3 100644
--- a/net/netfilter/nf_conntrack_extend.c
+++ b/net/netfilter/nf_conntrack_extend.c
@@ -59,12 +59,19 @@ nf_ct_ext_create(struct nf_ct_ext **ext, enum nf_ct_ext_id id, gfp_t gfp)
 	if (!*ext)
 		return NULL;
 
+	INIT_RCU_HEAD(&(*ext)->rcu);
 	(*ext)->offset[id] = off;
 	(*ext)->len = len;
 
 	return (void *)(*ext) + off;
 }
 
+static void __nf_ct_ext_free_rcu(struct rcu_head *head)
+{
+	struct nf_ct_ext *ext = container_of(head, struct nf_ct_ext, rcu);
+	kfree(ext);
+}
+
 void *__nf_ct_ext_add(struct nf_conn *ct, enum nf_ct_ext_id id, gfp_t gfp)
 {
 	struct nf_ct_ext *new;
@@ -106,7 +113,7 @@ void *__nf_ct_ext_add(struct nf_conn *ct, enum nf_ct_ext_id id, gfp_t gfp)
 					(void *)ct->ext + ct->ext->offset[i]);
 			rcu_read_unlock();
 		}
-		kfree(ct->ext);
+		call_rcu(&ct->ext->rcu, __nf_ct_ext_free_rcu);
 		ct->ext = new;
 	}