[2.6.30-git21] Network Namespace test failure

Quoting Serge E. Hallyn (serue@us.ibm.com):
> Quoting Sachin Sant (sachinp@in.ibm.com):
> > Serge E. Hallyn wrote:
> >> Precise kernel version and .config?
> >>   
> > Kernel version is 2.6.30-git21 (626f380d0b264a1e40237f5a2a3dffc5d14f256e)
> 
> Thanks.  I bisected it to commit
> ae0e8e82205c903978a79ebf5e31c670b61fa5b4 : "veth: prevent oops caused by
> netdev destructor".  That moves the free_percpu(priv->stats) from
> veth_dev_free to veth_close().  Since it gets allocated at
> veth_dev_init, and dveth_xmit uses it unconditionally, that seems like a
> likely cause of the oops?

Indeed the following patch fixes it on my end.  Sachin can you give
this one a shot?

thanks,
-serge

From 7193023ad09dbc4b57909c0204c19ed93472cd9e Mon Sep 17 00:00:00 2001
From: root <root@elm3b203.beaverton.ibm.com>
Date: Wed, 24 Jun 2009 20:26:17 -0700
Subject: [PATCH 1/1] veth: don't free priv->status until dev->destructor

Since commit ae0e8e82205c903978a79ebf5e31c670b61fa5b4, priv->status
has been freed at veth_close().  But that causes a NULL deref at
veth_xmit.  This patch moves priv->status free back to the device
destructor.

Signed-off-by: Serge Hallyn <serue@us.ibm.com>
---
 drivers/net/veth.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

Quoting Serge E. Hallyn (serue@us.ibm.com):
> Quoting Serge E. Hallyn (serue@us.ibm.com):
> > Quoting Sachin Sant (sachinp@in.ibm.com):
> > > Serge E. Hallyn wrote:
> > >> Precise kernel version and .config?
> > >>   
> > > Kernel version is 2.6.30-git21 (626f380d0b264a1e40237f5a2a3dffc5d14f256e)
> > 
> > Thanks.  I bisected it to commit
> > ae0e8e82205c903978a79ebf5e31c670b61fa5b4 : "veth: prevent oops caused by
> > netdev destructor".  That moves the free_percpu(priv->stats) from
> > veth_dev_free to veth_close().  Since it gets allocated at
> > veth_dev_init, and dveth_xmit uses it unconditionally, that seems like a
> > likely cause of the oops?
> 
> Indeed the following patch fixes it on my end.  Sachin can you give
> this one a shot?

BTW - according to the original patch, my patch is not a proper fix,
bc the destructor can't point to code in the module.

I'm not sure offhand what is a proper fix, though, so this patch seemed
ok for having Sachin test but is not intended as a mergeable fix.

> thanks,
> -serge
> 
> >From 7193023ad09dbc4b57909c0204c19ed93472cd9e Mon Sep 17 00:00:00 2001
> From: root <root@elm3b203.beaverton.ibm.com>
> Date: Wed, 24 Jun 2009 20:26:17 -0700
> Subject: [PATCH 1/1] veth: don't free priv->status until dev->destructor
> 
> Since commit ae0e8e82205c903978a79ebf5e31c670b61fa5b4, priv->status
> has been freed at veth_close().  But that causes a NULL deref at
> veth_xmit.  This patch moves priv->status free back to the device
> destructor.
> 
> Signed-off-by: Serge Hallyn <serue@us.ibm.com>
> ---
>  drivers/net/veth.c |   13 ++++++++++---
>  1 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index 8e56fcf..6000aae 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -259,8 +259,6 @@ static int veth_close(struct net_device *dev)
>  	netif_carrier_off(dev);
>  	netif_carrier_off(priv->peer);
> 
> -	free_percpu(priv->stats);
> -	priv->stats = NULL;
>  	return 0;
>  }
> 
> @@ -301,6 +299,15 @@ static const struct net_device_ops veth_netdev_ops = {
>  	.ndo_set_mac_address = eth_mac_addr,
>  };
> 
> +static void veth_dev_free(struct net_device *dev)
> +{
> +	struct veth_priv *priv;
> +
> +	priv = netdev_priv(dev);
> +	free_percpu(priv->stats);
> +	free_netdev(dev);
> +}
> +
>  static void veth_setup(struct net_device *dev)
>  {
>  	ether_setup(dev);
> @@ -308,7 +315,7 @@ static void veth_setup(struct net_device *dev)
>  	dev->netdev_ops = &veth_netdev_ops;
>  	dev->ethtool_ops = &veth_ethtool_ops;
>  	dev->features |= NETIF_F_LLTX;
> -	dev->destructor = free_netdev;
> +	dev->destructor = veth_dev_free;
>  }
> 
>  /*
> -- 
> 1.6.2.3
--
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

Serge E. Hallyn wrote:
> Indeed the following patch fixes it on my end.  Sachin can you give
> this one a shot?
>
> thanks,
> -serge
>
> From 7193023ad09dbc4b57909c0204c19ed93472cd9e Mon Sep 17 00:00:00 2001
> From: root <root@elm3b203.beaverton.ibm.com>
> Date: Wed, 24 Jun 2009 20:26:17 -0700
> Subject: [PATCH 1/1] veth: don't free priv->status until dev->destructor
>
> Since commit ae0e8e82205c903978a79ebf5e31c670b61fa5b4, priv->status
> has been freed at veth_close().  But that causes a NULL deref at
> veth_xmit.  This patch moves priv->status free back to the device
> destructor.
>
> Signed-off-by: Serge Hallyn <serue@us.ibm.com>
> ---
Yes this fixed the problem for me.

Thanks
-Sachin

From: "Serge E. Hallyn" <serue@us.ibm.com>
Date: Wed, 24 Jun 2009 22:38:47 -0500

>>From 7193023ad09dbc4b57909c0204c19ed93472cd9e Mon Sep 17 00:00:00 2001
> From: root <root@elm3b203.beaverton.ibm.com>
> Date: Wed, 24 Jun 2009 20:26:17 -0700
> Subject: [PATCH 1/1] veth: don't free priv->status until dev->destructor
> 
> Since commit ae0e8e82205c903978a79ebf5e31c670b61fa5b4, priv->status
> has been freed at veth_close().  But that causes a NULL deref at
> veth_xmit.  This patch moves priv->status free back to the device
> destructor.
> 
> Signed-off-by: Serge Hallyn <serue@us.ibm.com>

I think ae0e8e82205c903978a79ebf5e31c670b61fa5b4 is worse than
the problem it's trying to cure.  It introduces two regressions:

1) This OOPS we are discussing.

2) statistics are not "remembered" across ifdown/ifup and that
   disagrees with how every other network device works

I'm going to revert that change, and the dev->destructor pointing
to module code problem will need to be fixed in another way.

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