bridge netfilter output bug on 2.6.39

bridge netfilter output bug on 2.6.39

[Stephen Hemminger]

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

Re: bridge netfilter output bug on 2.6.39

[Eric Dumazet]

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

Re: bridge netfilter output bug on 2.6.39

[Eric Dumazet]

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.

Re: bridge netfilter output bug on 2.6.39

[Eric Dumazet]

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

Re: bridge netfilter output bug on 2.6.39

[David Miller]

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.

Re: bridge netfilter output bug on 2.6.39

[David Miller]

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.

Re: bridge netfilter output bug on 2.6.39

[Stephen Hemminger]

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

Re: bridge netfilter output bug on 2.6.39

[David Miller]

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.