Re: 2.6.0-test9 : bridge freezes

Re: 2.6.0-test9 : bridge freezes

[Linus Torvalds]

On Sat, 22 Nov 2003, SVR Anand wrote:
> 
> The problem is : After 3 to 4 hours of functioning, the bridge stops working 
> and the machine becomes unusable where it doesn't respond to keyboard, and 
> there is no video display.

Sounds like a memory leak somewhere. It would probably be interesting to 
watch /proc/slabinfo every five minutes or so, and see what happens..

		Linus

Re: 2.6.0-test9 : bridge freezes

[David S. Miller]

On Sat, 22 Nov 2003 08:20:40 -0800 (PST)
Linus Torvalds <torvalds@osdl.org> wrote:

> On Sat, 22 Nov 2003, SVR Anand wrote:
> > 
> > The problem is : After 3 to 4 hours of functioning, the bridge stops working 
> > and the machine becomes unusable where it doesn't respond to keyboard, and 
> > there is no video display.
> 
> Sounds like a memory leak somewhere. It would probably be interesting to 
> watch /proc/slabinfo every five minutes or so, and see what happens..

Also, we've certainly fixed some serious networking bugs since test9
came out.

Re: 2.6.0-test9 : bridge freezes

[Steve Hill]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1200 bytes --]


With both conntrack and bridging turned on in the 2.6.0test11 kernel, 
sending fragmented packets over the bridge reveals a memory leak 
(specifically, forwarding packets from any interface to a bridge).  The 
memory that is leaking seems to be being allocated on line 299 on 
net/bridge/br_netfilter.c:

        if ((nf_bridge = nf_bridge_alloc(skb)) == NULL)
                return NF_DROP;

Only the first fragment gets freed later on.

The patch attached fixes the problem by freeing nf_bridge when the 
packets are defragmented, however I am sure this is not the right place 
to do this.  Where would the skb's for the fragments usually get freed?

Bart De Schuymer suggested that they should be freed in 
skbuff.c::skb_release_data(), but having looked at this it seems to do 
this already.  skb_release_data() calls skb_drop_fraglist(), which does 
kfree_skb() on each fragment, and kfree_skb calls nf_bridge_put correctly 
so this isn't the problem.

-- 

- Steve Hill
Senior Software Developer                        Email: steve@navaho.co.uk
Navaho Technologies Ltd.                           Tel: +44-870-7034015

        ... Alcohol and calculus don't mix - Don't drink and derive! ...


[-- Attachment #2: Type: TEXT/PLAIN, Size: 565 bytes --]

diff -urN linux-2.6.0-test11.vanilla/net/ipv4/ip_fragment.c linux-2.6.0-test11/net/ipv4/ip_fragment.c
--- linux-2.6.0-test11.vanilla/net/ipv4/ip_fragment.c	2003-12-12 19:27:07.000000000 +0000
+++ linux-2.6.0-test11/net/ipv4/ip_fragment.c	2003-12-15 08:49:01.000000000 +0000
@@ -592,6 +592,9 @@
 	atomic_sub(head->truesize, &ip_frag_mem);
 
 	for (fp=head->next; fp; fp = fp->next) {
+#ifdef CONFIG_BRIDGE_NETFILTER
+		nf_bridge_put(fp->nf_bridge);
+#endif
 		head->data_len += fp->len;
 		head->len += fp->len;
 		if (head->ip_summed != fp->ip_summed)

Re: 2.6.0-test9 : bridge freezes

[David S. Miller]

On Mon, 15 Dec 2003 13:15:44 +0000 (GMT)
Steve Hill <steve@navaho.co.uk> wrote:

> The memory that is leaking seems to be being allocated on line 299 on
> net/bridge/br_netfilter.c:
> 
>         if ((nf_bridge = nf_bridge_alloc(skb)) == NULL)
>                 return NF_DROP;
> 
> Only the first fragment gets freed later on.

I see.

> The patch attached fixes the problem by freeing nf_bridge when the 
> packets are defragmented, however I am sure this is not the right place 
> to do this.  Where would the skb's for the fragments usually get freed?
> 
> Bart De Schuymer suggested that they should be freed in 
> skbuff.c::skb_release_data(), but having looked at this it seems to do 
> this already.  skb_release_data() calls skb_drop_fraglist(), which does 
> kfree_skb() on each fragment, and kfree_skb calls nf_bridge_put correctly 
> so this isn't the problem.

There must be something in particular that the IPV4 fragmentation code
is doing that makes these fragment reference drops get forgotten.  Hmmm...

I just noticed that both bridge netfilter and IPV4 fragmentation make much
use of the skb->cb[] control block, this may be the true source of the
troubles.

In fact, since bridge netfilter expects pointers to be there, I'm surprised
this does not cause a crash.

Re: 2.6.0-test9 : bridge freezes

[Bart De Schuymer]

On Tuesday 16 December 2003 02:17, David S. Miller wrote:
> There must be something in particular that the IPV4 fragmentation code
> is doing that makes these fragment reference drops get forgotten.  Hmmm...
>
> I just noticed that both bridge netfilter and IPV4 fragmentation make much
> use of the skb->cb[] control block, this may be the true source of the
> troubles.
>
> In fact, since bridge netfilter expects pointers to be there, I'm surprised
> this does not cause a crash.

It only expects a pointer in br_nf_forward_finish() for ARP traffic. I
checked and the ARP code doesn't use the control buffer.
For IP traffic, it uses the control buffer just before and just after
the call to the IP PRE_ROUTING hook.
OK, I just looked at the ip_fragment.c code and it uses the control buffer
too. You are truly amazing. I'll use skbuff.c::nf_bridge_info instead.

Steve, does this patch fix things? Of course, first remove your code from
ip_fragment.c. I haven't tested this patch yet, this will have to wait
until this evening.
Dave, I'll cook up a slightly different patch for you later, I think
nf_bridge->hh is now a bad name, I'll change it into nf_bridge->data.

thanks,
Bart

--- linux-2.6.0-test11-bk10/net/bridge/br_netfilter.c.old	2003-12-16 08:33:35.000000000 +0100
+++ linux-2.6.0-test11-bk10/net/bridge/br_netfilter.c	2003-12-16 08:34:12.000000000 +0100
@@ -38,11 +38,9 @@
 
 
 #define skb_origaddr(skb)	 (((struct bridge_skb_cb *) \
-				 (skb->cb))->daddr.ipv4)
+				 (skb->nf_bridge->hh))->daddr.ipv4)
 #define store_orig_dstaddr(skb)	 (skb_origaddr(skb) = (skb)->nh.iph->daddr)
 #define dnat_took_place(skb)	 (skb_origaddr(skb) != (skb)->nh.iph->daddr)
-#define clear_cb(skb)		 (memset(&skb_origaddr(skb), 0, \
-				 sizeof(struct bridge_skb_cb)))
 
 #define has_bridge_parent(device)	((device)->br_port != NULL)
 #define bridge_parent(device)		((device)->br_port->br->dev)
@@ -203,7 +201,6 @@ bridged_dnat:
 				 */
 				nf_bridge->mask |= BRNF_BRIDGED_DNAT;
 				skb->dev = nf_bridge->physindev;
-				clear_cb(skb);
 				if (skb->protocol ==
 				    __constant_htons(ETH_P_8021Q)) {
 					skb_push(skb, VLAN_HLEN);
@@ -224,7 +221,6 @@ bridged_dnat:
 		dst_hold(skb->dst);
 	}
 
-	clear_cb(skb);
 	skb->dev = nf_bridge->physindev;
 	if (skb->protocol == __constant_htons(ETH_P_8021Q)) {
 		skb_push(skb, VLAN_HLEN);

Re: 2.6.0-test9 : bridge freezes

[David S. Miller]

On Tue, 16 Dec 2003 08:43:58 +0100
Bart De Schuymer <bdschuym@pandora.be> wrote:

> You are truly amazing.

Don't make me blush in public :)

> Dave, I'll cook up a slightly different patch for you later, I think
> nf_bridge->hh is now a bad name, I'll change it into nf_bridge->data.

Great, I hope we've got this one nailed.

Re: 2.6.0-test9 : bridge freezes

[Steve Hill]

On Tue, 16 Dec 2003, Bart De Schuymer wrote:

> Steve, does this patch fix things? Of course, first remove your code from
> ip_fragment.c. I haven't tested this patch yet, this will have to wait
> until this evening.

No, it still leaks I'm afraid :(

-- 

- Steve Hill
Senior Software Developer                        Email: steve@navaho.co.uk
Navaho Technologies Ltd.                           Tel: +44-870-7034015

        ... Alcohol and calculus don't mix - Don't drink and derive! ...

Re: 2.6.0-test9 : bridge freezes

[Bart De Schuymer]

On Tuesday 16 December 2003 10:00, Steve Hill wrote:
> On Tue, 16 Dec 2003, Bart De Schuymer wrote:
> > Steve, does this patch fix things? Of course, first remove your code from
> > ip_fragment.c. I haven't tested this patch yet, this will have to wait
> > until this evening.
>
> No, it still leaks I'm afraid :(

OK, I think the patch below should fix it, my previous patch is still valid.
I'll send a combined patch once it's confirmed this fixes the bug.

Steve, please test this patch.

cheers,
Bart

--- linux-2.6.0-test11-bk10/net/ipv4/ip_output.c.old	2003-12-16 22:05:02.000000000 +0100
+++ linux-2.6.0-test11-bk10/net/ipv4/ip_output.c	2003-12-16 22:36:02.000000000 +0100
@@ -417,6 +417,7 @@ static void ip_copy_metadata(struct sk_b
 	to->nfct = from->nfct;
 	nf_conntrack_get(to->nfct);
 #ifdef CONFIG_BRIDGE_NETFILTER
+	nf_bridge_put(to->nf_bridge);
 	to->nf_bridge = from->nf_bridge;
 	nf_bridge_get(to->nf_bridge);
 #endif

Re: 2.6.0-test9 : bridge freezes

[David S. Miller]

On Tue, 16 Dec 2003 22:46:45 +0100
Bart De Schuymer <bdschuym@pandora.be> wrote:

>  #ifdef CONFIG_BRIDGE_NETFILTER
> +	nf_bridge_put(to->nf_bridge);
>  	to->nf_bridge = from->nf_bridge;
>  	nf_bridge_get(to->nf_bridge);
>  #endif

Now this change makes a LOT of sense, great that you discovered it.

Steve, we eagerly wait your test results :)

Re: 2.6.0-test9 : bridge freezes

[Steve Hill]

On Tue, 16 Dec 2003, Bart De Schuymer wrote:

> OK, I think the patch below should fix it, my previous patch is still valid.
> I'll send a combined patch once it's confirmed this fixes the bug.

Yep, this seems to fix the problem.  Thanks.

-- 

- Steve Hill
Senior Software Developer                        Email: steve@navaho.co.uk
Navaho Technologies Ltd.                           Tel: +44-870-7034015

        ... Alcohol and calculus don't mix - Don't drink and derive! ...

Re: 2.6.0-test9 : bridge freezes

[Bart De Schuymer]

On Wednesday 17 December 2003 09:36, Steve Hill wrote:
> On Tue, 16 Dec 2003, Bart De Schuymer wrote:
> > OK, I think the patch below should fix it, my previous patch is still
> > valid. I'll send a combined patch once it's confirmed this fixes the bug.
>
> Yep, this seems to fix the problem.  Thanks.

Cool,

Dave, here's the patch, nf_bridge_info.hh is renamed to nf_bridge_info.data,
the bridge-nf IP code no longer uses the control buffer and a very
necessary call of nf_bridge_put is added.

cheers,
Bart


--- linux-2.6.0-test11-bk10/include/linux/skbuff.h.old	2003-12-17 08:12:12.000000000 +0100
+++ linux-2.6.0-test11-bk10/include/linux/skbuff.h	2003-12-17 08:12:38.000000000 +0100
@@ -107,7 +107,7 @@ struct nf_bridge_info {
 	struct net_device *netoutdev;
 #endif
 	unsigned int mask;
-	unsigned long hh[32 / sizeof(unsigned long)];
+	unsigned long data[32 / sizeof(unsigned long)];
 };
 #endif
 
--- linux-2.6.0-test11-bk10/include/linux/netfilter_bridge.h.old	2003-12-17 08:08:21.000000000 +0100
+++ linux-2.6.0-test11-bk10/include/linux/netfilter_bridge.h	2003-12-17 08:09:19.000000000 +0100
@@ -73,11 +73,11 @@ void nf_bridge_maybe_copy_header(struct 
 	if (skb->nf_bridge) {
 #if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 		if (skb->protocol == __constant_htons(ETH_P_8021Q)) {
-			memcpy(skb->data - 18, skb->nf_bridge->hh, 18);
+			memcpy(skb->data - 18, skb->nf_bridge->data, 18);
 			skb_push(skb, 4);
 		} else
 #endif
-			memcpy(skb->data - 16, skb->nf_bridge->hh, 16);
+			memcpy(skb->data - 16, skb->nf_bridge->data, 16);
 	}
 }
 
@@ -90,7 +90,7 @@ void nf_bridge_save_header(struct sk_buf
 	if (skb->protocol == __constant_htons(ETH_P_8021Q))
 		header_size = 18;
 #endif
-	memcpy(skb->nf_bridge->hh, skb->data - header_size, header_size);
+	memcpy(skb->nf_bridge->data, skb->data - header_size, header_size);
 }
 
 struct bridge_skb_cb {
--- linux-2.6.0-test11-bk10/net/bridge/br_netfilter.c.old	2003-12-16 08:33:35.000000000 +0100
+++ linux-2.6.0-test11-bk10/net/bridge/br_netfilter.c	2003-12-17 08:08:08.000000000 +0100
@@ -38,11 +38,9 @@
 
 
 #define skb_origaddr(skb)	 (((struct bridge_skb_cb *) \
-				 (skb->cb))->daddr.ipv4)
+				 (skb->nf_bridge->data))->daddr.ipv4)
 #define store_orig_dstaddr(skb)	 (skb_origaddr(skb) = (skb)->nh.iph->daddr)
 #define dnat_took_place(skb)	 (skb_origaddr(skb) != (skb)->nh.iph->daddr)
-#define clear_cb(skb)		 (memset(&skb_origaddr(skb), 0, \
-				 sizeof(struct bridge_skb_cb)))
 
 #define has_bridge_parent(device)	((device)->br_port != NULL)
 #define bridge_parent(device)		((device)->br_port->br->dev)
@@ -203,7 +201,6 @@ bridged_dnat:
 				 */
 				nf_bridge->mask |= BRNF_BRIDGED_DNAT;
 				skb->dev = nf_bridge->physindev;
-				clear_cb(skb);
 				if (skb->protocol ==
 				    __constant_htons(ETH_P_8021Q)) {
 					skb_push(skb, VLAN_HLEN);
@@ -224,7 +221,6 @@ bridged_dnat:
 		dst_hold(skb->dst);
 	}
 
-	clear_cb(skb);
 	skb->dev = nf_bridge->physindev;
 	if (skb->protocol == __constant_htons(ETH_P_8021Q)) {
 		skb_push(skb, VLAN_HLEN);
--- linux-2.6.0-test11-bk10/net/ipv4/ip_output.c.old	2003-12-16 22:05:02.000000000 +0100
+++ linux-2.6.0-test11-bk10/net/ipv4/ip_output.c	2003-12-16 22:36:02.000000000 +0100
@@ -417,6 +417,7 @@ static void ip_copy_metadata(struct sk_b
 	to->nfct = from->nfct;
 	nf_conntrack_get(to->nfct);
 #ifdef CONFIG_BRIDGE_NETFILTER
+	nf_bridge_put(to->nf_bridge);
 	to->nf_bridge = from->nf_bridge;
 	nf_bridge_get(to->nf_bridge);
 #endif