diff mbox

bridge netfilter output bug on 2.6.39

Message ID 1306251543.3026.57.camel@edumazet-laptop
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Eric Dumazet May 24, 2011, 3:39 p.m. UTC 
Le mardi 24 mai 2011 à 07:41 -0700, Stephen Hemminger a écrit :
> Got this bug report against 2.6.39.  Looks like ip_fragment() is now
> getting confused when called from bridge netfilter. Probably related to
> the changes to do ip_options_compile for the bridge input path.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=35672
> 
> May 23 02:04:24 lxc kernel: [99498.329036] BUG: unable to handle kernel NULL
> pointer dereference at 00000004
> May 23 02:04:24 lxc kernel: [99498.330017] IP: [<c143d6bf>] dst_mtu+0xb/0x1c
> May 23 02:04:24 lxc kernel: [99498.330017] *pdpt = 000000001fb55001 *pde =
> 0000000000000000
> May 23 02:04:24 lxc kernel: [99498.330017] Oops: 0000 [#1] SMP
> May 23 02:04:24 lxc kernel: [99498.330017] last sysfs file:
> /sys/devices/virtual/vc/vcsa8/uevent
> May 23 02:04:24 lxc kernel: [99498.330017] Modules linked in: lp ppdev
> parport_pc parport fuse firewire_ohci firewire_core crc_itu_t intel_agp
> intel_gtt
> May 23 02:04:24 lxc kernel: [99498.330017]
> May 23 02:04:24 lxc kernel: [99498.330017] Pid: 0, comm: swapper Not tainted
> 2.6.39-lxc #2 .   .  /IP35 Pro XE(Intel P35-ICH9R)
> May 23 02:04:24 lxc kernel: [99498.330017] EIP: 0060:[<c143d6bf>] EFLAGS:
> 00010246 CPU: 0
> May 23 02:04:24 lxc kernel: [99498.330017] EIP is at dst_mtu+0xb/0x1c
> May 23 02:04:24 lxc kernel: [99498.330017] EAX: 00000000 EBX: e90b6b40 ECX:
> effc981c EDX: effc9000
> May 23 02:04:24 lxc kernel: [99498.330017] ESI: c1a0d84e EDI: dda6331e EBP:
> f080bb44 ESP: f080bb44
> May 23 02:04:24 lxc kernel: [99498.330017]  DS: 007b ES: 007b FS: 00d8 GS: 0000
> SS: 0068
> May 23 02:04:24 lxc kernel: [99498.330017] Process swapper (pid: 0, ti=f080a000
> task=c172b7e0 task.ti=c1724000)
> May 23 02:04:24 lxc kernel: [99498.330017] Stack:
> May 23 02:04:24 lxc kernel: [99498.330017]  f080bb8c c143e20d 00000004 f080bb88
> c141aab2 c14b46db effc9000 00000014
> May 23 02:04:24 lxc kernel: [99498.330017]  c14b8a44 effc9000 e90b6b40 00000014
> effc981c e90b6b58 cd472800 e90b6b40
> May 23 02:04:24 lxc kernel: [99498.330017]  c14b8a44 dda6331e f080bb98 c14b8aa0
> e90b6b40 f080bba8 c14b881a e90b6b40
> May 23 02:04:24 lxc kernel: [99498.330017] Call Trace:
> May 23 02:04:24 lxc kernel: [99498.330017]  [<c143e20d>] ip_fragment+0xb5/0x66c
> May 23 02:04:24 lxc kernel: [99498.330017]  [<c141aab2>] ?
> nf_hook_slow+0x43/0xd1
> May 23 02:04:24 lxc kernel: [99498.330017]  [<c14b46db>] ? br_flood+0x83/0x83
> May 23 02:04:24 lxc kernel: [99498.330017]  [<c14b8a44>] ?
> br_parse_ip_options+0x1b0/0x1b0
> May 23 02:04:24 lxc kernel: [99498.330017]  [<c14b8a44>] ?
> br_parse_ip_options+0x1b0/0x1b0
> May 23 02:04:24 lxc kernel: [99498.330017]  [<c14b8aa0>]
> br_nf_dev_queue_xmit+0x5c/0x68
> --

I would say its more likely a problem with dst metrics changes

In this crash, we dereference a NULL dst->_metrics 'pointer' in
dst_metric_raw(dst, RTAX_MTU);

Hmm, it seems __dst_destroy_metrics_generic() doesnt add the
DST_METRICS_READ_ONLY flag ?

[PATCH] net: fix __dst_destroy_metrics_generic()

dst_default_metrics is readonly, we dont want to kfree() it later.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Stephen Hemminger <shemminger@vyatta.com>
CC: Herbert Xu <herbert@gondor.apana.org.au>
---
 net/core/dst.c |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)



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

Comments

Eric Dumazet May 24, 2011, 4:27 p.m. UTC | #1 
Le mardi 24 mai 2011 à 17:39 +0200, Eric Dumazet a écrit :

> I would say its more likely a problem with dst metrics changes
> 
> In this crash, we dereference a NULL dst->_metrics 'pointer' in
> dst_metric_raw(dst, RTAX_MTU);

It seems bridge code uses one fake_rtable

You probably want to properly init its _metric field.

I can do the patch in one hour eventually, if nobody beats me.



--
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
David Miller May 24, 2011, 5:30 p.m. UTC | #2 
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 24 May 2011 17:39:03 +0200

> [PATCH] net: fix __dst_destroy_metrics_generic()
> 
> dst_default_metrics is readonly, we dont want to kfree() it later.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Good catch, applied, thanks.
--
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
David Miller May 24, 2011, 5:31 p.m. UTC | #3 
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 24 May 2011 18:27:37 +0200

> It seems bridge code uses one fake_rtable

Ugh, forgot about this turd. :-/

All route objects should be dynamically allocated, otherwise we'll
constantly have to attend to these static route instances when we make
any changes to dst_alloc() or similar.

I'll apply the fix you posted for now, but long term this code
needs to accomplish it's goals in a different way.
--
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
diff mbox

Patch

diff --git a/net/core/dst.c b/net/core/dst.c
index 81a4fa1..1badc98 100644
--- a/net/core/dst.c
+++ b/net/core/dst.c
@@ -315,7 +315,7 @@  void __dst_destroy_metrics_generic(struct dst_entry *dst, unsigned long old)
 {
 	unsigned long prev, new;
 
-	new = (unsigned long) dst_default_metrics;
+	new = ((unsigned long) dst_default_metrics) | DST_METRICS_READ_ONLY;
 	prev = cmpxchg(&dst->_metrics, old, new);
 	if (prev == old)
 		kfree(__DST_METRICS_PTR(old));