* bridge netfilter output bug on 2.6.39 @ 2011-05-24 14:41 Stephen Hemminger 2011-05-24 15:39 ` Eric Dumazet 0 siblings, 1 reply; 8+ messages in thread From: Stephen Hemminger @ 2011-05-24 14:41 UTC (permalink / raw) To: Herbert Xu; +Cc: netdev 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: bridge netfilter output bug on 2.6.39 2011-05-24 14:41 bridge netfilter output bug on 2.6.39 Stephen Hemminger @ 2011-05-24 15:39 ` Eric Dumazet 2011-05-24 16:27 ` Eric Dumazet 2011-05-24 17:30 ` David Miller 0 siblings, 2 replies; 8+ messages in thread From: Eric Dumazet @ 2011-05-24 15:39 UTC (permalink / raw) To: Stephen Hemminger, David Miller; +Cc: Herbert Xu, netdev 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(-) 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)); ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: bridge netfilter output bug on 2.6.39 2011-05-24 15:39 ` Eric Dumazet @ 2011-05-24 16:27 ` Eric Dumazet 2011-05-24 16:46 ` Eric Dumazet 2011-05-24 17:31 ` David Miller 2011-05-24 17:30 ` David Miller 1 sibling, 2 replies; 8+ messages in thread From: Eric Dumazet @ 2011-05-24 16:27 UTC (permalink / raw) To: Stephen Hemminger; +Cc: David Miller, Herbert Xu, netdev 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: bridge netfilter output bug on 2.6.39 2011-05-24 16:27 ` Eric Dumazet @ 2011-05-24 16:46 ` Eric Dumazet 2011-05-24 17:40 ` Stephen Hemminger 2011-05-24 17:31 ` David Miller 1 sibling, 1 reply; 8+ messages in thread From: Eric Dumazet @ 2011-05-24 16:46 UTC (permalink / raw) To: Stephen Hemminger; +Cc: David Miller, Herbert Xu, netdev Le mardi 24 mai 2011 à 18:27 +0200, Eric Dumazet a écrit : > 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. > > Here is the patch : [PATCH] bridge: initialize fake_rtable metrics bridge netfilter code uses a fake_rtable, and we must init its _metric field or risk NULL dereference later. Ref: https://bugzilla.kernel.org/show_bug.cgi?id=35672 Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> CC: Stephen Hemminger <shemminger@vyatta.com> CC: Herbert Xu <herbert@gondor.apana.org.au> --- net/bridge/br_netfilter.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletion(-) diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c index e1f5ec7..3fa1231 100644 --- a/net/bridge/br_netfilter.c +++ b/net/bridge/br_netfilter.c @@ -117,6 +117,10 @@ static struct dst_ops fake_dst_ops = { * ipt_REJECT needs it. Future netfilter modules might * require us to fill additional fields. */ +static const u32 br_dst_default_metrics[RTAX_MAX] = { + [RTAX_MTU - 1] = 1500, +}; + void br_netfilter_rtable_init(struct net_bridge *br) { struct rtable *rt = &br->fake_rtable; @@ -124,7 +128,7 @@ void br_netfilter_rtable_init(struct net_bridge *br) atomic_set(&rt->dst.__refcnt, 1); rt->dst.dev = br->dev; rt->dst.path = &rt->dst; - dst_metric_set(&rt->dst, RTAX_MTU, 1500); + dst_init_metrics(&rt->dst, br_dst_default_metrics, true); rt->dst.flags = DST_NOXFRM; rt->dst.ops = &fake_dst_ops; } ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: bridge netfilter output bug on 2.6.39 2011-05-24 16:46 ` Eric Dumazet @ 2011-05-24 17:40 ` Stephen Hemminger 2011-05-24 17:49 ` David Miller 0 siblings, 1 reply; 8+ messages in thread From: Stephen Hemminger @ 2011-05-24 17:40 UTC (permalink / raw) To: Eric Dumazet; +Cc: David Miller, Herbert Xu, netdev On Tue, 24 May 2011 18:46:27 +0200 Eric Dumazet <eric.dumazet@gmail.com> wrote: > Le mardi 24 mai 2011 à 18:27 +0200, Eric Dumazet a écrit : > > 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. > > > > > > Here is the patch : > > [PATCH] bridge: initialize fake_rtable metrics > > bridge netfilter code uses a fake_rtable, and we must init its _metric > field or risk NULL dereference later. > > Ref: https://bugzilla.kernel.org/show_bug.cgi?id=35672 > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > CC: Stephen Hemminger <shemminger@vyatta.com> > CC: Herbert Xu <herbert@gondor.apana.org.au> > --- > net/bridge/br_netfilter.c | 6 +++++- > 1 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c > index e1f5ec7..3fa1231 100644 > --- a/net/bridge/br_netfilter.c > +++ b/net/bridge/br_netfilter.c > @@ -117,6 +117,10 @@ static struct dst_ops fake_dst_ops = { > * ipt_REJECT needs it. Future netfilter modules might > * require us to fill additional fields. > */ > +static const u32 br_dst_default_metrics[RTAX_MAX] = { > + [RTAX_MTU - 1] = 1500, > +}; > + > void br_netfilter_rtable_init(struct net_bridge *br) > { > struct rtable *rt = &br->fake_rtable; > @@ -124,7 +128,7 @@ void br_netfilter_rtable_init(struct net_bridge *br) > atomic_set(&rt->dst.__refcnt, 1); > rt->dst.dev = br->dev; > rt->dst.path = &rt->dst; > - dst_metric_set(&rt->dst, RTAX_MTU, 1500); > + dst_init_metrics(&rt->dst, br_dst_default_metrics, true); > rt->dst.flags = DST_NOXFRM; > rt->dst.ops = &fake_dst_ops; > } This part is fine. Acked-by: Stephen Hemminger <shemminger@vyatta.com> I think there should be BUG_ON any calls to dst_metric_set where dst has no metrics available. [PATCH] dst: catch uninitialized metrics Catch cases where dst_metric_set() and other functions are called but _metrics is NULL. Signed-off-by: Stephen Hemminger <shemminger@vyatta.com> --- a/include/net/dst.h 2011-05-24 10:36:07.597962703 -0700 +++ b/include/net/dst.h 2011-05-24 10:36:54.382509111 -0700 @@ -111,6 +111,8 @@ static inline u32 *dst_metrics_write_ptr { unsigned long p = dst->_metrics; + BUG_ON(!p); + if (p & DST_METRICS_READ_ONLY) return dst->ops->cow_metrics(dst, p); return __DST_METRICS_PTR(p); ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: bridge netfilter output bug on 2.6.39 2011-05-24 17:40 ` Stephen Hemminger @ 2011-05-24 17:49 ` David Miller 0 siblings, 0 replies; 8+ messages in thread From: David Miller @ 2011-05-24 17:49 UTC (permalink / raw) To: shemminger; +Cc: eric.dumazet, herbert, netdev From: Stephen Hemminger <shemminger@vyatta.com> Date: Tue, 24 May 2011 10:40:22 -0700 > I think there should be BUG_ON any calls to dst_metric_set where > dst has no metrics available. Yeah I'll add this too, we can kill it later after we're sure that we've snuffed out all of these kinds of bugs. Thanks. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: bridge netfilter output bug on 2.6.39 2011-05-24 16:27 ` Eric Dumazet 2011-05-24 16:46 ` Eric Dumazet @ 2011-05-24 17:31 ` David Miller 1 sibling, 0 replies; 8+ messages in thread From: David Miller @ 2011-05-24 17:31 UTC (permalink / raw) To: eric.dumazet; +Cc: shemminger, herbert, netdev 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: bridge netfilter output bug on 2.6.39 2011-05-24 15:39 ` Eric Dumazet 2011-05-24 16:27 ` Eric Dumazet @ 2011-05-24 17:30 ` David Miller 1 sibling, 0 replies; 8+ messages in thread From: David Miller @ 2011-05-24 17:30 UTC (permalink / raw) To: eric.dumazet; +Cc: shemminger, herbert, netdev 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-05-24 17:49 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-05-24 14:41 bridge netfilter output bug on 2.6.39 Stephen Hemminger 2011-05-24 15:39 ` Eric Dumazet 2011-05-24 16:27 ` Eric Dumazet 2011-05-24 16:46 ` Eric Dumazet 2011-05-24 17:40 ` Stephen Hemminger 2011-05-24 17:49 ` David Miller 2011-05-24 17:31 ` David Miller 2011-05-24 17:30 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).