Re: bridge netfilter output bug on 2.6.39

[Stephen Hemminger <shemminger@vyatta.com>]

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