[PATCH 0/6] Bonding simplifications and netns support

[PATCH 0/6] Bonding simplifications and netns support

[Eric W. Biederman]

I recently had it pointed out to me that the bonding driver does not
work in a network namespace.  So I have simplified the bonding driver
a bit, added support for ip link add and ip link del, and finally made
the bonding driver work in multiple network namespaces.

The most note worthy change in the patchset is the addition of support
in the networking core for registering a sysfs group for a device.

Using this in the bonding driver simplifies the code and removes a
userspace race between actions triggered by the netlink event and the
bonding sysfs attributes appearing.

Eric

[PATCH 1/6] net: Allow devices to specify a device specific sysfs group.

[Eric W. Biederman]

From: Eric W. Biederman <ebiederm@aristanetworks.com>

This isn't beautifully abstracted, but it is simple,
simplifies uses and so far is only needed for the bonding driver.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 include/linux/netdevice.h |    4 ++--
 net/core/net-sysfs.c      |    5 ++++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index ffc3106..b37f01e 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -899,8 +899,8 @@ struct net_device
 
 	/* class/net/name entry */
 	struct device		dev;
-	/* space for optional statistics and wireless sysfs groups */
-	const struct attribute_group *sysfs_groups[3];
+	/* space for optional device, statistics, and wireless sysfs groups */
+	const struct attribute_group *sysfs_groups[4];
 
 	/* rtnetlink link ops */
 	const struct rtnl_link_ops *rtnl_link_ops;
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 89de182..157645c 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -544,8 +544,11 @@ int netdev_register_kobject(struct net_device *net)
 	dev_set_name(dev, "%s", net->name);
 
 #ifdef CONFIG_SYSFS
-	*groups++ = &netstat_group;
+	/* Allow for a device specific group */
+	if (*groups)
+		groups++;
 
+	*groups++ = &netstat_group;
 #ifdef CONFIG_WIRELESS_EXT_SYSFS
 	if (net->ieee80211_ptr)
 		*groups++ = &wireless_group;
-- 
1.6.3.1.54.g99dd.dirty

[PATCH 2/6] bond: Simply bond sysfs group creation

[Eric W. Biederman]

From: Eric W. Biederman <ebiederm@aristanetworks.com>

This patch delegates the work of creating the sysfs groups
to the netdev layer and ultimately to the device layer.  This
closes races between uevents.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 drivers/net/bonding/bond_main.c  |   11 +----------
 drivers/net/bonding/bond_sysfs.c |   20 ++------------------
 drivers/net/bonding/bonding.h    |    3 +--
 3 files changed, 4 insertions(+), 30 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 8c5ebfb..f73d2de 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2011,7 +2011,6 @@ static void bond_uninit(struct net_device *bond_dev)
 	struct bonding *bond = netdev_priv(bond_dev);
 
 	bond_deinit(bond_dev);
-	bond_destroy_sysfs_entry(bond);
 
 	if (bond->wq)
 		destroy_workqueue(bond->wq);
@@ -3457,9 +3456,6 @@ static int bond_event_changename(struct bonding *bond)
 	bond_remove_proc_entry(bond);
 	bond_create_proc_entry(bond);
 
-	bond_destroy_sysfs_entry(bond);
-	bond_create_sysfs_entry(bond);
-
 	return NOTIFY_DONE;
 }
 
@@ -5078,6 +5074,7 @@ static int bond_init(struct net_device *bond_dev)
 	bond_create_proc_entry(bond);
 	list_add_tail(&bond->bond_list, &bond_dev_list);
 
+	bond_prepare_sysfs_group(bond);
 	return 0;
 }
 
@@ -5120,15 +5117,9 @@ int bond_create(const char *name)
 	if (res < 0)
 		goto out_bond;
 
-	res = bond_create_sysfs_entry(netdev_priv(bond_dev));
-	if (res < 0)
-		goto out_unreg;
-
 	rtnl_unlock();
 	return 0;
 
-out_unreg:
-	unregister_netdevice(bond_dev);
 out_bond:
 	bond_deinit(bond_dev);
 out_netdev:
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index dca7d82..f924a0b 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -1616,24 +1616,8 @@ void bond_destroy_sysfs(void)
  * Initialize sysfs for each bond.  This sets up and registers
  * the 'bondctl' directory for each individual bond under /sys/class/net.
  */
-int bond_create_sysfs_entry(struct bonding *bond)
+void bond_prepare_sysfs_group(struct bonding *bond)
 {
-	struct net_device *dev = bond->dev;
-	int err;
-
-	err = sysfs_create_group(&(dev->dev.kobj), &bonding_group);
-	if (err)
-		pr_emerg("eek! didn't create group!\n");
-
-	return err;
-}
-/*
- * Remove sysfs entries for each bond.
- */
-void bond_destroy_sysfs_entry(struct bonding *bond)
-{
-	struct net_device *dev = bond->dev;
-
-	sysfs_remove_group(&(dev->dev.kobj), &bonding_group);
+	bond->dev->sysfs_groups[0] = &bonding_group;
 }
 
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 9b520b0..013be29 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -331,8 +331,7 @@ int bond_create(const char *name);
 int  bond_release_and_destroy(struct net_device *bond_dev, struct net_device *slave_dev);
 int bond_create_sysfs(void);
 void bond_destroy_sysfs(void);
-void bond_destroy_sysfs_entry(struct bonding *bond);
-int bond_create_sysfs_entry(struct bonding *bond);
+void bond_prepare_sysfs_group(struct bonding *bond);
 int bond_create_slave_symlinks(struct net_device *master, struct net_device *slave);
 void bond_destroy_slave_symlinks(struct net_device *master, struct net_device *slave);
 int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev);
-- 
1.6.3.1.54.g99dd.dirty

[PATCH 3/6] bond: Simplify bond_create.

[Eric W. Biederman]

From: Eric W. Biederman <ebiederm@aristanetworks.com>

Stop calling dev_get_by_name to see if the bond device already
exists.  register_netdevice already does that.

Stop calling bond_deinit if register_netdevice fails as bond_uninit
is guaranteed to be called if bond_init succeeds.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 drivers/net/bonding/bond_main.c |   22 ++++------------------
 1 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index f73d2de..3ce31e7 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5089,14 +5089,6 @@ int bond_create(const char *name)
 	int res;
 
 	rtnl_lock();
-	/* Check to see if the bond already exists. */
-	/* FIXME: pass netns from caller */
-	if (name && __dev_get_by_name(&init_net, name)) {
-		pr_err(DRV_NAME ": cannot add bond %s; already exists\n",
-		       name);
-		res = -EEXIST;
-		goto out_rtnl;
-	}
 
 	bond_dev = alloc_netdev(sizeof(struct bonding), name ? name : "",
 				bond_setup);
@@ -5104,7 +5096,7 @@ int bond_create(const char *name)
 		pr_err(DRV_NAME ": %s: eek! can't alloc netdev!\n",
 		       name);
 		res = -ENOMEM;
-		goto out_rtnl;
+		goto out;
 	}
 
 	if (!name) {
@@ -5114,19 +5106,13 @@ int bond_create(const char *name)
 	}
 
 	res = register_netdevice(bond_dev);
-	if (res < 0)
-		goto out_bond;
 
+out:
 	rtnl_unlock();
-	return 0;
-
-out_bond:
-	bond_deinit(bond_dev);
+	return res;
 out_netdev:
 	free_netdev(bond_dev);
-out_rtnl:
-	rtnl_unlock();
-	return res;
+	goto out;
 }
 
 static int __init bonding_init(void)
-- 
1.6.3.1.54.g99dd.dirty

[PATCH 4/6] bond: Simplify bond device destruction

[Eric W. Biederman]

From: Eric W. Biederman <ebiederm@aristanetworks.com>

Manually inline the code from bond_deinit to bond_uninit.  bond_uninit
is the only caller and it is short.

Move the call of bond_release_all from the netdev notifier into
bond_uninit.  The call site is effectively the same and performing
the call explicitly allows all the paths for destroying a
bonding device to behave the same way.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 drivers/net/bonding/bond_main.c |   45 +++++++++++++-------------------------
 1 files changed, 16 insertions(+), 29 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 3ce31e7..7a37ecf 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -227,7 +227,7 @@ struct bond_parm_tbl ad_select_tbl[] = {
 
 static void bond_send_gratuitous_arp(struct bonding *bond);
 static int bond_init(struct net_device *bond_dev);
-static void bond_deinit(struct net_device *bond_dev);
+static void bond_uninit(struct net_device *bond_dev);
 
 /*---------------------------- General routines -----------------------------*/
 
@@ -2003,24 +2003,6 @@ int bond_release(struct net_device *bond_dev, struct net_device *slave_dev)
 }
 
 /*
-* Destroy a bonding device.
-* Must be under rtnl_lock when this function is called.
-*/
-static void bond_uninit(struct net_device *bond_dev)
-{
-	struct bonding *bond = netdev_priv(bond_dev);
-
-	bond_deinit(bond_dev);
-
-	if (bond->wq)
-		destroy_workqueue(bond->wq);
-
-	netif_addr_lock_bh(bond_dev);
-	bond_mc_list_destroy(bond);
-	netif_addr_unlock_bh(bond_dev);
-}
-
-/*
 * First release a slave and than destroy the bond if no more slaves are left.
 * Must be under rtnl_lock when this function is called.
 */
@@ -3467,9 +3449,6 @@ static int bond_master_netdev_event(unsigned long event,
 	switch (event) {
 	case NETDEV_CHANGENAME:
 		return bond_event_changename(event_bond);
-	case NETDEV_UNREGISTER:
-		bond_release_all(event_bond->dev);
-		break;
 	default:
 		break;
 	}
@@ -4608,18 +4587,29 @@ static void bond_work_cancel_all(struct bonding *bond)
 		cancel_delayed_work(&bond->ad_work);
 }
 
-/* De-initialize device specific data.
- * Caller must hold rtnl_lock.
- */
-static void bond_deinit(struct net_device *bond_dev)
+/*
+* Destroy a bonding device.
+* Must be under rtnl_lock when this function is called.
+*/
+static void bond_uninit(struct net_device *bond_dev)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
 
+	/* Release the bonded slaves */
+	bond_release_all(bond_dev);
+
 	list_del(&bond->bond_list);
 
 	bond_work_cancel_all(bond);
 
 	bond_remove_proc_entry(bond);
+
+	if (bond->wq)
+		destroy_workqueue(bond->wq);
+
+	netif_addr_lock_bh(bond_dev);
+	bond_mc_list_destroy(bond);
+	netif_addr_unlock_bh(bond_dev);
 }
 
 /* Unregister and free all bond devices.
@@ -4632,9 +4622,6 @@ static void bond_free_all(void)
 	list_for_each_entry_safe(bond, nxt, &bond_dev_list, bond_list) {
 		struct net_device *bond_dev = bond->dev;
 
-		bond_work_cancel_all(bond);
-		/* Release the bonded slaves */
-		bond_release_all(bond_dev);
 		unregister_netdevice(bond_dev);
 	}
 
-- 
1.6.3.1.54.g99dd.dirty

[PATCH 5/6] bond: Implement a basic set of rtnl link ops

[Eric W. Biederman]

From: Eric W. Biederman <ebiederm@aristanetworks.com>

This implements a basic set of rtnl link ops and takes advantage of
the fact that rtnl_link_unregister kills all of the surviving
devices to all us to kill bond_free_all.  A module alias
is added so ip link add can pull in the bonding module.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 drivers/net/bonding/bond_main.c |   55 +++++++++++++++++++++-----------------
 1 files changed, 30 insertions(+), 25 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 7a37ecf..6da2a82 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4612,22 +4612,6 @@ static void bond_uninit(struct net_device *bond_dev)
 	netif_addr_unlock_bh(bond_dev);
 }
 
-/* Unregister and free all bond devices.
- * Caller must hold rtnl_lock.
- */
-static void bond_free_all(void)
-{
-	struct bonding *bond, *nxt;
-
-	list_for_each_entry_safe(bond, nxt, &bond_dev_list, bond_list) {
-		struct net_device *bond_dev = bond->dev;
-
-		unregister_netdevice(bond_dev);
-	}
-
-	bond_destroy_proc_dir();
-}
-
 /*------------------------- Module initialization ---------------------------*/
 
 /*
@@ -5065,6 +5049,23 @@ static int bond_init(struct net_device *bond_dev)
 	return 0;
 }
 
+static int bond_validate(struct nlattr *tb[], struct nlattr *data[])
+{
+	if (tb[IFLA_ADDRESS]) {
+		if (nla_len(tb[IFLA_ADDRESS]) != ETH_ALEN)
+			return -EINVAL;
+		if (!is_valid_ether_addr(nla_data(tb[IFLA_ADDRESS])))
+			return -EADDRNOTAVAIL;
+	}
+	return 0;
+}
+
+static struct rtnl_link_ops bond_link_ops __read_mostly = {
+	.kind		= "bond",
+	.setup		= bond_setup,
+	.validate	= bond_validate,
+};
+
 /* Create a new bond based on the specified name and bonding parameters.
  * If name is NULL, obtain a suitable "bond%d" name for us.
  * Caller must NOT hold rtnl_lock; we need to release it here before we
@@ -5086,6 +5087,8 @@ int bond_create(const char *name)
 		goto out;
 	}
 
+	bond_dev->rtnl_link_ops = &bond_link_ops;
+
 	if (!name) {
 		res = dev_alloc_name(bond_dev, "bond%d");
 		if (res < 0)
@@ -5115,6 +5118,10 @@ static int __init bonding_init(void)
 
 	bond_create_proc_dir();
 
+	res = rtnl_link_register(&bond_link_ops);
+	if (res)
+		goto err;
+
 	for (i = 0; i < max_bonds; i++) {
 		res = bond_create(NULL);
 		if (res)
@@ -5128,14 +5135,12 @@ static int __init bonding_init(void)
 	register_netdevice_notifier(&bond_netdev_notifier);
 	register_inetaddr_notifier(&bond_inetaddr_notifier);
 	bond_register_ipv6_notifier();
-
-	goto out;
-err:
-	rtnl_lock();
-	bond_free_all();
-	rtnl_unlock();
 out:
 	return res;
+err:
+	rtnl_link_unregister(&bond_link_ops);
+	bond_destroy_proc_dir();
+	goto out;
 
 }
 
@@ -5147,9 +5152,8 @@ static void __exit bonding_exit(void)
 
 	bond_destroy_sysfs();
 
-	rtnl_lock();
-	bond_free_all();
-	rtnl_unlock();
+	rtnl_link_unregister(&bond_link_ops);
+	bond_destroy_proc_dir();
 }
 
 module_init(bonding_init);
@@ -5158,3 +5162,4 @@ MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_VERSION);
 MODULE_DESCRIPTION(DRV_DESCRIPTION ", v" DRV_VERSION);
 MODULE_AUTHOR("Thomas Davis, tadavis@lbl.gov and many others");
+MODULE_ALIAS_RTNL_LINK("bond");
-- 
1.6.3.1.54.g99dd.dirty

Re: [PATCH 5/6] bond: Implement a basic set of rtnl link ops

[Patrick McHardy]

Eric W. Biederman wrote:
> @@ -5115,6 +5118,10 @@ static int __init bonding_init(void)
>  
>  	bond_create_proc_dir();
>  
> +	res = rtnl_link_register(&bond_link_ops);
> +	if (res)
> +		goto err;

The error handling looks wrong, at err: you call rtnl_link_unregister()
after rtnl_link_register() failed.

> +
>  	for (i = 0; i < max_bonds; i++) {
>  		res = bond_create(NULL);
>  		if (res)
> @@ -5128,14 +5135,12 @@ static int __init bonding_init(void)
>  	register_netdevice_notifier(&bond_netdev_notifier);
>  	register_inetaddr_notifier(&bond_inetaddr_notifier);
>  	bond_register_ipv6_notifier();
> -
> -	goto out;
> -err:
> -	rtnl_lock();
> -	bond_free_all();
> -	rtnl_unlock();
>  out:
>  	return res;
> +err:
> +	rtnl_link_unregister(&bond_link_ops);
> +	bond_destroy_proc_dir();
> +	goto out;
>  
>  }

Re: [PATCH 5/6] bond: Implement a basic set of rtnl link ops

[Patrick McHardy]

Eric W. Biederman wrote:
> +static struct rtnl_link_ops bond_link_ops __read_mostly = {
> +	.kind		= "bond",
> +	.setup		= bond_setup,
> +	.validate	= bond_validate,
> +};

One more thing - you need to initialize .priv_size here so
the devices created through rtnl_link have enough private
room allocated.

Re: [PATCH 5/6] bond: Implement a basic set of rtnl link ops

[Eric W. Biederman]

Patrick McHardy <kaber@trash.net> writes:

> Eric W. Biederman wrote:
>> +static struct rtnl_link_ops bond_link_ops __read_mostly = {
>> +	.kind		= "bond",
>> +	.setup		= bond_setup,
>> +	.validate	= bond_validate,
>> +};
>
> One more thing - you need to initialize .priv_size here so
> the devices created through rtnl_link have enough private
> room allocated.

Wow and the code works when I test it without that ouch!

As for rtnl_link_register it always succeeds so let's just
remove the return code and call it good.

Eric

Re: [PATCH 5/6] bond: Implement a basic set of rtnl link ops

[Patrick McHardy]

Eric W. Biederman wrote:
> Patrick McHardy <kaber@trash.net> writes:
> 
>> Eric W. Biederman wrote:
>>> +static struct rtnl_link_ops bond_link_ops __read_mostly = {
>>> +	.kind		= "bond",
>>> +	.setup		= bond_setup,
>>> +	.validate	= bond_validate,
>>> +};
>> One more thing - you need to initialize .priv_size here so
>> the devices created through rtnl_link have enough private
>> room allocated.
> 
> Wow and the code works when I test it without that ouch!
> 
> As for rtnl_link_register it always succeeds so let's just
> remove the return code and call it good.

You need unroll anyways for the other failure conditions, so
why not simply add an err1/2 and be safe for future changes?

[PATCH 7/6] bond: Get the rtnl_link_ops support correct

[Eric W. Biederman]

- Don't call rtnl_link_unregister if rtnl_link_register fails
- Set .priv_size so we aren't stomping on uninitialized memory
  when we use netdev_priv, on bond devices created with
  ip link add type bond.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 drivers/net/bonding/bond_main.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index fed44e6..0ef051c 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5004,6 +5004,7 @@ static int bond_validate(struct nlattr *tb[], struct nlattr *data[])
 
 static struct rtnl_link_ops bond_link_ops __read_mostly = {
 	.kind		= "bond",
+	.priv_size	= sizeof(struct bonding),
 	.setup		= bond_setup,
 	.validate	= bond_validate,
 };
@@ -5105,7 +5106,7 @@ static int __init bonding_init(void)
 
 	res = rtnl_link_register(&bond_link_ops);
 	if (res)
-		goto err;
+		goto err_link;
 
 	for (i = 0; i < max_bonds; i++) {
 		res = bond_create(&init_net, NULL);
@@ -5124,6 +5125,7 @@ out:
 	return res;
 err:
 	rtnl_link_unregister(&bond_link_ops);
+err_link:
 	unregister_pernet_gen_subsys(bond_net_id, &bond_net_ops);
 	goto out;
 
-- 
1.6.3.1.54.g99dd.dirty

Re: [PATCH 5/6] bond: Implement a basic set of rtnl link ops

[Eric W. Biederman]

Patrick McHardy <kaber@trash.net> writes:

> Eric W. Biederman wrote:
>> Patrick McHardy <kaber@trash.net> writes:
>> 
>>> Eric W. Biederman wrote:
>>>> +static struct rtnl_link_ops bond_link_ops __read_mostly = {
>>>> +	.kind		= "bond",
>>>> +	.setup		= bond_setup,
>>>> +	.validate	= bond_validate,
>>>> +};
>>> One more thing - you need to initialize .priv_size here so
>>> the devices created through rtnl_link have enough private
>>> room allocated.
>> 
>> Wow and the code works when I test it without that ouch!
>> 
>> As for rtnl_link_register it always succeeds so let's just
>> remove the return code and call it good.
>
> You need unroll anyways for the other failure conditions, so
> why not simply add an err1/2 and be safe for future changes?

Not a real problem.  I was just thinking of things like the
dummy driver that have this same issue and the fact that since
rtnl_link_register never fails we never test the error path.
So it would be much less error prone and less code to remove
the possibility of rtnl_link_register failing.

Eric

Re: [PATCH 5/6] bond: Implement a basic set of rtnl link ops

[Patrick McHardy]

Eric W. Biederman wrote:
> Patrick McHardy <kaber@trash.net> writes:
> 
>>> As for rtnl_link_register it always succeeds so let's just
>>> remove the return code and call it good.
>> You need unroll anyways for the other failure conditions, so
>> why not simply add an err1/2 and be safe for future changes?
> 
> Not a real problem.  I was just thinking of things like the
> dummy driver that have this same issue and the fact that since
> rtnl_link_register never fails we never test the error path.
> So it would be much less error prone and less code to remove
> the possibility of rtnl_link_register failing.

Mhh good point, I think I added the broken dummy code myself :)

The main reason for not returning void from rtnl_link_register()
was so new drivers that are written with rtnl_link in mind from
the beginning (and thus usually don't do anything like default
device creation, sysfs registrations etc.) can simply do
"return rtnl_link_register(&ops)" in their init function. But
that's admittedly not a very strong argument :)

[PATCH 6/6] bond: Add support for multiple network namespaces

[Eric W. Biederman]

From: Eric W. Biederman <ebiederm@aristanetworks.com>

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 drivers/net/bonding/bond_3ad.c   |    3 -
 drivers/net/bonding/bond_alb.c   |    3 -
 drivers/net/bonding/bond_ipv6.c  |    7 +--
 drivers/net/bonding/bond_main.c  |  111 +++++++++++++++++++++++++-------------
 drivers/net/bonding/bond_sysfs.c |   19 ++++---
 drivers/net/bonding/bonding.h    |   14 ++++--
 6 files changed, 99 insertions(+), 58 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 3cd8153..1d05819 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -2445,9 +2445,6 @@ int bond_3ad_lacpdu_recv(struct sk_buff *skb, struct net_device *dev, struct pac
 	struct slave *slave = NULL;
 	int ret = NET_RX_DROP;
 
-	if (dev_net(dev) != &init_net)
-		goto out;
-
 	if (!(dev->flags & IFF_MASTER))
 		goto out;
 
diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 9b5936f..0d30d1e 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -355,9 +355,6 @@ static int rlb_arp_recv(struct sk_buff *skb, struct net_device *bond_dev, struct
 	struct arp_pkt *arp = (struct arp_pkt *)skb->data;
 	int res = NET_RX_DROP;
 
-	if (dev_net(bond_dev) != &init_net)
-		goto out;
-
 	while (bond_dev->priv_flags & IFF_802_1Q_VLAN)
 		bond_dev = vlan_dev_real_dev(bond_dev);
 
diff --git a/drivers/net/bonding/bond_ipv6.c b/drivers/net/bonding/bond_ipv6.c
index 83921ab..b72e1dc 100644
--- a/drivers/net/bonding/bond_ipv6.c
+++ b/drivers/net/bonding/bond_ipv6.c
@@ -25,6 +25,7 @@
 #include <net/ipv6.h>
 #include <net/ndisc.h>
 #include <net/addrconf.h>
+#include <net/netns/generic.h>
 #include "bonding.h"
 
 /*
@@ -152,11 +153,9 @@ static int bond_inet6addr_event(struct notifier_block *this,
 	struct net_device *vlan_dev, *event_dev = ifa->idev->dev;
 	struct bonding *bond;
 	struct vlan_entry *vlan;
+	struct bond_net *bn = net_generic(dev_net(event_dev), bond_net_id);
 
-	if (dev_net(event_dev) != &init_net)
-		return NOTIFY_DONE;
-
-	list_for_each_entry(bond, &bond_dev_list, bond_list) {
+	list_for_each_entry(bond, &bn->dev_list, bond_list) {
 		if (bond->dev == event_dev) {
 			switch (event) {
 			case NETDEV_UP:
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 6da2a82..568609e 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -75,6 +75,7 @@
 #include <linux/jiffies.h>
 #include <net/route.h>
 #include <net/net_namespace.h>
+#include <net/netns/generic.h>
 #include "bonding.h"
 #include "bond_3ad.h"
 #include "bond_alb.h"
@@ -157,11 +158,7 @@ MODULE_PARM_DESC(fail_over_mac, "For active-backup, do not set all slaves to the
 static const char * const version =
 	DRV_DESCRIPTION ": v" DRV_VERSION " (" DRV_RELDATE ")\n";
 
-LIST_HEAD(bond_dev_list);
-
-#ifdef CONFIG_PROC_FS
-static struct proc_dir_entry *bond_proc_dir;
-#endif
+int bond_net_id;
 
 static __be32 arp_target[BOND_MAX_ARP_TARGETS];
 static int arp_ip_count;
@@ -2586,7 +2583,7 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 		fl.fl4_dst = targets[i];
 		fl.fl4_tos = RTO_ONLINK;
 
-		rv = ip_route_output_key(&init_net, &rt, &fl);
+		rv = ip_route_output_key(dev_net(bond->dev), &rt, &fl);
 		if (rv) {
 			if (net_ratelimit()) {
 				pr_warning(DRV_NAME
@@ -2694,9 +2691,6 @@ static int bond_arp_rcv(struct sk_buff *skb, struct net_device *dev, struct pack
 	unsigned char *arp_ptr;
 	__be32 sip, tip;
 
-	if (dev_net(dev) != &init_net)
-		goto out;
-
 	if (!(dev->priv_flags & IFF_BONDING) || !(dev->flags & IFF_MASTER))
 		goto out;
 
@@ -3359,10 +3353,11 @@ static const struct file_operations bond_info_fops = {
 static void bond_create_proc_entry(struct bonding *bond)
 {
 	struct net_device *bond_dev = bond->dev;
+	struct bond_net *bn = net_generic(dev_net(bond_dev), bond_net_id);
 
-	if (bond_proc_dir) {
+	if (bn->proc_dir) {
 		bond->proc_entry = proc_create_data(bond_dev->name,
-						    S_IRUGO, bond_proc_dir,
+						    S_IRUGO, bn->proc_dir,
 						    &bond_info_fops, bond);
 		if (bond->proc_entry == NULL)
 			pr_warning(DRV_NAME
@@ -3375,8 +3370,11 @@ static void bond_create_proc_entry(struct bonding *bond)
 
 static void bond_remove_proc_entry(struct bonding *bond)
 {
-	if (bond_proc_dir && bond->proc_entry) {
-		remove_proc_entry(bond->proc_file_name, bond_proc_dir);
+	struct net_device *bond_dev = bond->dev;
+	struct bond_net *bn = net_generic(dev_net(bond_dev), bond_net_id);
+
+	if (bn->proc_dir && bond->proc_entry) {
+		remove_proc_entry(bond->proc_file_name, bn->proc_dir);
 		memset(bond->proc_file_name, 0, IFNAMSIZ);
 		bond->proc_entry = NULL;
 	}
@@ -3385,11 +3383,11 @@ static void bond_remove_proc_entry(struct bonding *bond)
 /* Create the bonding directory under /proc/net, if doesn't exist yet.
  * Caller must hold rtnl_lock.
  */
-static void bond_create_proc_dir(void)
+static void bond_create_proc_dir(struct bond_net *bn)
 {
-	if (!bond_proc_dir) {
-		bond_proc_dir = proc_mkdir(DRV_NAME, init_net.proc_net);
-		if (!bond_proc_dir)
+	if (!bn->proc_dir) {
+		bn->proc_dir = proc_mkdir(DRV_NAME, bn->net->proc_net);
+		if (!bn->proc_dir)
 			pr_warning(DRV_NAME
 				": Warning: cannot create /proc/net/%s\n",
 				DRV_NAME);
@@ -3399,11 +3397,11 @@ static void bond_create_proc_dir(void)
 /* Destroy the bonding directory under /proc/net, if empty.
  * Caller must hold rtnl_lock.
  */
-static void bond_destroy_proc_dir(void)
+static void bond_destroy_proc_dir(struct bond_net *bn)
 {
-	if (bond_proc_dir) {
-		remove_proc_entry(DRV_NAME, init_net.proc_net);
-		bond_proc_dir = NULL;
+	if (bn->proc_dir) {
+		remove_proc_entry(DRV_NAME, bn->net->proc_net);
+		bn->proc_dir = NULL;
 	}
 }
 
@@ -3417,11 +3415,11 @@ static void bond_remove_proc_entry(struct bonding *bond)
 {
 }
 
-static void bond_create_proc_dir(void)
+static void bond_create_proc_dir(struct bond_net *bn)
 {
 }
 
-static void bond_destroy_proc_dir(void)
+static void bond_destroy_proc_dir(struct bond_net *bn)
 {
 }
 
@@ -3540,9 +3538,6 @@ static int bond_netdev_event(struct notifier_block *this,
 {
 	struct net_device *event_dev = (struct net_device *)ptr;
 
-	if (dev_net(event_dev) != &init_net)
-		return NOTIFY_DONE;
-
 	pr_debug("event_dev: %s, event: %lx\n",
 		(event_dev ? event_dev->name : "None"),
 		event);
@@ -3575,13 +3570,11 @@ static int bond_inetaddr_event(struct notifier_block *this, unsigned long event,
 {
 	struct in_ifaddr *ifa = ptr;
 	struct net_device *vlan_dev, *event_dev = ifa->ifa_dev->dev;
+	struct bond_net *bn = net_generic(dev_net(event_dev), bond_net_id);
 	struct bonding *bond;
 	struct vlan_entry *vlan;
 
-	if (dev_net(ifa->ifa_dev->dev) != &init_net)
-		return NOTIFY_DONE;
-
-	list_for_each_entry(bond, &bond_dev_list, bond_list) {
+	list_for_each_entry(bond, &bn->dev_list, bond_list) {
 		if (bond->dev == event_dev) {
 			switch (event) {
 			case NETDEV_UP:
@@ -3950,7 +3943,7 @@ static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd
 	if (!capable(CAP_NET_ADMIN))
 		return -EPERM;
 
-	slave_dev = dev_get_by_name(&init_net, ifr->ifr_slave);
+	slave_dev = dev_get_by_name(dev_net(bond_dev), ifr->ifr_slave);
 
 	pr_debug("slave_dev=%p: \n", slave_dev);
 
@@ -5031,6 +5024,7 @@ static void bond_set_lockdep_class(struct net_device *dev)
 static int bond_init(struct net_device *bond_dev)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
+	struct bond_net *bn = net_generic(dev_net(bond_dev), bond_net_id);
 
 	pr_debug("Begin bond_init for %s\n", bond_dev->name);
 
@@ -5043,7 +5037,7 @@ static int bond_init(struct net_device *bond_dev)
 	netif_carrier_off(bond_dev);
 
 	bond_create_proc_entry(bond);
-	list_add_tail(&bond->bond_list, &bond_dev_list);
+	list_add_tail(&bond->bond_list, &bn->dev_list);
 
 	bond_prepare_sysfs_group(bond);
 	return 0;
@@ -5071,7 +5065,7 @@ static struct rtnl_link_ops bond_link_ops __read_mostly = {
  * Caller must NOT hold rtnl_lock; we need to release it here before we
  * set up our sysfs entries.
  */
-int bond_create(const char *name)
+int bond_create(struct net *net, const char *name)
 {
 	struct net_device *bond_dev;
 	int res;
@@ -5087,6 +5081,7 @@ int bond_create(const char *name)
 		goto out;
 	}
 
+	dev_net_set(bond_dev, net);
 	bond_dev->rtnl_link_ops = &bond_link_ops;
 
 	if (!name) {
@@ -5105,6 +5100,46 @@ out_netdev:
 	goto out;
 }
 
+static int bond_net_init(struct net *net)
+{
+	struct bond_net *bn;
+	int err;
+
+	err = -ENOMEM;
+	bn = kzalloc(sizeof(struct bond_net), GFP_KERNEL);
+	if (bn == NULL)
+		goto out;
+
+	bn->net = net;
+	INIT_LIST_HEAD(&bn->dev_list);
+
+	err = net_assign_generic(net, bond_net_id, bn);
+	if (err)
+		goto out_free;
+
+	bond_create_proc_dir(bn);
+out:
+	return err;
+out_free:
+	kfree(bn);
+	goto out;
+}
+
+static void bond_net_exit(struct net *net)
+{
+	struct bond_net *bn;
+
+	bn = net_generic(net, bond_net_id);
+
+	bond_destroy_proc_dir(bn);
+	kfree(bn);
+}
+
+static struct pernet_operations bond_net_ops = {
+	.init = bond_net_init,
+	.exit = bond_net_exit,
+};
+
 static int __init bonding_init(void)
 {
 	int i;
@@ -5116,14 +5151,16 @@ static int __init bonding_init(void)
 	if (res)
 		goto out;
 
-	bond_create_proc_dir();
+	res = register_pernet_gen_subsys(&bond_net_id, &bond_net_ops);
+	if (res)
+		goto out;
 
 	res = rtnl_link_register(&bond_link_ops);
 	if (res)
 		goto err;
 
 	for (i = 0; i < max_bonds; i++) {
-		res = bond_create(NULL);
+		res = bond_create(&init_net, NULL);
 		if (res)
 			goto err;
 	}
@@ -5139,7 +5176,7 @@ out:
 	return res;
 err:
 	rtnl_link_unregister(&bond_link_ops);
-	bond_destroy_proc_dir();
+	unregister_pernet_gen_subsys(bond_net_id, &bond_net_ops);
 	goto out;
 
 }
@@ -5153,7 +5190,7 @@ static void __exit bonding_exit(void)
 	bond_destroy_sysfs();
 
 	rtnl_link_unregister(&bond_link_ops);
-	bond_destroy_proc_dir();
+	unregister_pernet_gen_subsys(bond_net_id, &bond_net_ops);
 }
 
 module_init(bonding_init);
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index f924a0b..a59094f 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -35,6 +35,8 @@
 #include <linux/rtnetlink.h>
 #include <linux/etherdevice.h>
 #include <net/net_namespace.h>
+#include <net/netns/generic.h>
+#include <linux/nsproxy.h>
 
 #include "bonding.h"
 
@@ -47,12 +49,14 @@
  */
 static ssize_t bonding_show_bonds(struct class *cls, char *buf)
 {
+	struct net *net = current->nsproxy->net_ns;
+	struct bond_net *bn = net_generic(net, bond_net_id);
 	int res = 0;
 	struct bonding *bond;
 
 	rtnl_lock();
 
-	list_for_each_entry(bond, &bond_dev_list, bond_list) {
+	list_for_each_entry(bond, &bn->dev_list, bond_list) {
 		if (res > (PAGE_SIZE - IFNAMSIZ)) {
 			/* not enough space for another interface name */
 			if ((PAGE_SIZE - res) > 10)
@@ -69,11 +73,12 @@ static ssize_t bonding_show_bonds(struct class *cls, char *buf)
 	return res;
 }
 
-static struct net_device *bond_get_by_name(const char *ifname)
+static struct net_device *bond_get_by_name(struct net *net, const char *ifname)
 {
+	struct bond_net *bn = net_generic(net, bond_net_id);
 	struct bonding *bond;
 
-	list_for_each_entry(bond, &bond_dev_list, bond_list) {
+	list_for_each_entry(bond, &bn->dev_list, bond_list) {
 		if (strncmp(bond->dev->name, ifname, IFNAMSIZ) == 0)
 			return bond->dev;
 	}
@@ -91,6 +96,7 @@ static struct net_device *bond_get_by_name(const char *ifname)
 static ssize_t bonding_store_bonds(struct class *cls,
 				   const char *buffer, size_t count)
 {
+	struct net *net = current->nsproxy->net_ns;
 	char command[IFNAMSIZ + 1] = {0, };
 	char *ifname;
 	int rv, res = count;
@@ -104,7 +110,7 @@ static ssize_t bonding_store_bonds(struct class *cls,
 	if (command[0] == '+') {
 		pr_info(DRV_NAME
 			": %s is being created...\n", ifname);
-		rv = bond_create(ifname);
+		rv = bond_create(net, ifname);
 		if (rv) {
 			pr_info(DRV_NAME ": Bond creation failed.\n");
 			res = rv;
@@ -113,7 +119,7 @@ static ssize_t bonding_store_bonds(struct class *cls,
 		struct net_device *bond_dev;
 
 		rtnl_lock();
-		bond_dev = bond_get_by_name(ifname);
+		bond_dev = bond_get_by_name(net, ifname);
 		if (bond_dev) {
 			pr_info(DRV_NAME ": %s is being deleted...\n",
 				ifname);
@@ -238,8 +244,7 @@ static ssize_t bonding_store_slaves(struct device *d,
 		/* Got a slave name in ifname.  Is it already in the list? */
 		found = 0;
 
-		/* FIXME: get netns from sysfs object */
-		dev = __dev_get_by_name(&init_net, ifname);
+		dev = __dev_get_by_name(dev_net(bond->dev), ifname);
 		if (!dev) {
 			pr_info(DRV_NAME
 			       ": %s: Interface %s does not exist!\n",
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 013be29..a51ae7d 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -30,8 +30,6 @@
 
 #define BOND_MAX_ARP_TARGETS	16
 
-extern struct list_head bond_dev_list;
-
 #define IS_UP(dev)					   \
 	      ((((dev)->flags & IFF_UP) == IFF_UP)	&& \
 	       netif_running(dev)			&& \
@@ -327,7 +325,7 @@ static inline void bond_unset_master_alb_flags(struct bonding *bond)
 
 struct vlan_entry *bond_next_vlan(struct bonding *bond, struct vlan_entry *curr);
 int bond_dev_queue_xmit(struct bonding *bond, struct sk_buff *skb, struct net_device *slave_dev);
-int bond_create(const char *name);
+int bond_create(struct net *net, const char *name);
 int  bond_release_and_destroy(struct net_device *bond_dev, struct net_device *slave_dev);
 int bond_create_sysfs(void);
 void bond_destroy_sysfs(void);
@@ -346,8 +344,16 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active);
 void bond_register_arp(struct bonding *);
 void bond_unregister_arp(struct bonding *);
 
+struct bond_net {
+	struct net *		net;	/* Associated network namespace */
+	struct list_head	dev_list;
+#ifdef CONFIG_PROC_FS
+	struct proc_dir_entry *	proc_dir;
+#endif
+};
+
 /* exported from bond_main.c */
-extern struct list_head bond_dev_list;
+extern int bond_net_id;
 extern const struct bond_parm_tbl bond_lacp_tbl[];
 extern const struct bond_parm_tbl bond_mode_tbl[];
 extern const struct bond_parm_tbl xmit_hashtype_tbl[];
-- 
1.6.3.1.54.g99dd.dirty

Re: [PATCH 0/6] Bonding simplifications and netns support

[David Miller]

From: ebiederm@xmission.com (Eric W. Biederman)
Date: Thu, 29 Oct 2009 17:16:54 -0700

> I recently had it pointed out to me that the bonding driver does not
> work in a network namespace.  So I have simplified the bonding driver
> a bit, added support for ip link add and ip link del, and finally made
> the bonding driver work in multiple network namespaces.
> 
> The most note worthy change in the patchset is the addition of support
> in the networking core for registering a sysfs group for a device.
> 
> Using this in the bonding driver simplifies the code and removes a
> userspace race between actions triggered by the netlink event and the
> bonding sysfs attributes appearing.

I have no objections to these patches, but I'd like the bonding
folks to have a chance to look at it before I apply to net-next-2.6

One question though, are you sure this clever extra slot scheme
in patch #1 works for, f.e., a bond of wireless devices?  It seems
like it would work out, but I wanted to ask to make sure you
considered that case.

Thanks.

Re: [PATCH 0/6] Bonding simplifications and netns support

[Eric Dumazet]

David Miller a écrit :
> From: ebiederm@xmission.com (Eric W. Biederman)
> Date: Thu, 29 Oct 2009 17:16:54 -0700
> 
>> I recently had it pointed out to me that the bonding driver does not
>> work in a network namespace.  So I have simplified the bonding driver
>> a bit, added support for ip link add and ip link del, and finally made
>> the bonding driver work in multiple network namespaces.
>>
>> The most note worthy change in the patchset is the addition of support
>> in the networking core for registering a sysfs group for a device.
>>
>> Using this in the bonding driver simplifies the code and removes a
>> userspace race between actions triggered by the netlink event and the
>> bonding sysfs attributes appearing.
> 
> I have no objections to these patches, but I'd like the bonding
> folks to have a chance to look at it before I apply to net-next-2.6
> 
> One question though, are you sure this clever extra slot scheme
> in patch #1 works for, f.e., a bond of wireless devices?  It seems
> like it would work out, but I wanted to ask to make sure you
> considered that case.
> 

I have patches against bond driver for unregister_netdevice_queue() stuff,
I'll wait for Eric work being committed first.

Thanks

Re: [PATCH 0/6] Bonding simplifications and netns support

[David Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Fri, 30 Oct 2009 07:38:33 +0100

> I have patches against bond driver for unregister_netdevice_queue() stuff,
> I'll wait for Eric work being committed first.

Ok, thanks for the heads up.

Re: [PATCH 0/6] Bonding simplifications and netns support

[Eric W. Biederman]

David Miller <davem@davemloft.net> writes:

> From: ebiederm@xmission.com (Eric W. Biederman)
> Date: Thu, 29 Oct 2009 17:16:54 -0700
>
>> I recently had it pointed out to me that the bonding driver does not
>> work in a network namespace.  So I have simplified the bonding driver
>> a bit, added support for ip link add and ip link del, and finally made
>> the bonding driver work in multiple network namespaces.
>> 
>> The most note worthy change in the patchset is the addition of support
>> in the networking core for registering a sysfs group for a device.
>> 
>> Using this in the bonding driver simplifies the code and removes a
>> userspace race between actions triggered by the netlink event and the
>> bonding sysfs attributes appearing.
>
> I have no objections to these patches, but I'd like the bonding
> folks to have a chance to look at it before I apply to net-next-2.6

Sure.

> One question though, are you sure this clever extra slot scheme
> in patch #1 works for, f.e., a bond of wireless devices?  It seems
> like it would work out, but I wanted to ask to make sure you
> considered that case.

I have not explicitly tested wireless devices.  But I did make certain
we have enough slots in the array.  I did write the code so that a
device driver can use at most one slot (the next slot gets
unconditionally stomped).  Other that it is just shifting of where
sysfs_create_group and sysfs_remove_group are called.  So I would
be totally stunned if bonded wireless devices started failing from
this change.

Sometime when I have sufficient ambition I intend to reorganize all
callers of sysfs_create_group, sysfs_create_file so that device_add
does all of the work, allowing userspace that responds to hotplug
events to count on everything being there.  The current situation is
inherently racy which is a unnecessary pain.

Eric

Re: [PATCH 0/6] Bonding simplifications and netns support

[Eric W. Biederman]

ebiederm@xmission.com (Eric W. Biederman) writes:

> David Miller <davem@davemloft.net> writes:
>
>> From: ebiederm@xmission.com (Eric W. Biederman)
>> Date: Thu, 29 Oct 2009 17:16:54 -0700
>>
>>> I recently had it pointed out to me that the bonding driver does not
>>> work in a network namespace.  So I have simplified the bonding driver
>>> a bit, added support for ip link add and ip link del, and finally made
>>> the bonding driver work in multiple network namespaces.
>>> 
>>> The most note worthy change in the patchset is the addition of support
>>> in the networking core for registering a sysfs group for a device.
>>> 
>>> Using this in the bonding driver simplifies the code and removes a
>>> userspace race between actions triggered by the netlink event and the
>>> bonding sysfs attributes appearing.
>>
>> I have no objections to these patches, but I'd like the bonding
>> folks to have a chance to look at it before I apply to net-next-2.6
>
> Sure.
>
>> One question though, are you sure this clever extra slot scheme
>> in patch #1 works for, f.e., a bond of wireless devices?  It seems
>> like it would work out, but I wanted to ask to make sure you
>> considered that case.
>
> I have not explicitly tested wireless devices.  But I did make certain
> we have enough slots in the array.  I did write the code so that a
> device driver can use at most one slot (the next slot gets
> unconditionally stomped).  Other that it is just shifting of where
> sysfs_create_group and sysfs_remove_group are called.  So I would
> be totally stunned if bonded wireless devices started failing from
> this change.

Bah.  The argument is better than that.  The bond_group that I am
messing with only applies to the virtual bonding devices.  The virtual
bond device is never a wireless device.  So we will never see in
practice all three groups on the same network device.

Eric

Re: [PATCH 0/6] Bonding simplifications and netns support

[David Miller]

From: ebiederm@xmission.com (Eric W. Biederman)
Date: Thu, 29 Oct 2009 17:16:54 -0700

> I recently had it pointed out to me that the bonding driver does not
> work in a network namespace.  So I have simplified the bonding driver
> a bit, added support for ip link add and ip link del, and finally made
> the bonding driver work in multiple network namespaces.
> 
> The most note worthy change in the patchset is the addition of support
> in the networking core for registering a sysfs group for a device.
> 
> Using this in the bonding driver simplifies the code and removes a
> userspace race between actions triggered by the netlink event and the
> bonding sysfs attributes appearing.

I've tossed patches 1-7 into net-next-2.6, thanks Eric.

Re: [PATCH 0/6] Bonding simplifications and netns support

[Jay Vosburgh]

David Miller <davem@davemloft.net> wrote:

>From: ebiederm@xmission.com (Eric W. Biederman)
>Date: Thu, 29 Oct 2009 17:16:54 -0700
>
>> I recently had it pointed out to me that the bonding driver does not
>> work in a network namespace.  So I have simplified the bonding driver
>> a bit, added support for ip link add and ip link del, and finally made
>> the bonding driver work in multiple network namespaces.
>> 
>> The most note worthy change in the patchset is the addition of support
>> in the networking core for registering a sysfs group for a device.
>> 
>> Using this in the bonding driver simplifies the code and removes a
>> userspace race between actions triggered by the netlink event and the
>> bonding sysfs attributes appearing.
>
>I've tossed patches 1-7 into net-next-2.6, thanks Eric.

	I put patches 1-7 on a recent net-next-2.6, and from a simple
"insmod bonding.ko; rmmod bonding" I'm seeing the following:

------------[ cut here ]------------
WARNING: at fs/proc/generic.c:847 remove_proc_entry+0x1a8/0x1c7()
Hardware name: IBM eserver xSeries 220 -[8645]-
remove_proc_entry: removing non-empty directory 'net/bonding', leaking at least 
'bond0'
Modules linked in: bonding(-) ipv6 microcode loop ppdev sworks_agp parport_pc tg
3 e100 agpgart parport mii libphy e1000 edd pata_serverworks [last unloaded: spe
edstep_lib]
Pid: 6216, comm: rmmod Not tainted 2.6.32-rc3-devel #19
Call Trace:
 [<c012ec9d>] warn_slowpath_common+0x60/0x90
 [<c012ed01>] warn_slowpath_fmt+0x24/0x27
 [<c01e3e55>] remove_proc_entry+0x1a8/0x1c7
 [<e0906435>] ? bond_net_exit+0x0/0xa3 [bonding]
 [<e09064c3>] bond_net_exit+0x8e/0xa3 [bonding]
 [<c02e6ee8>] unregister_pernet_gen_subsys+0x23/0x3d
 [<e0910baa>] bonding_exit+0x3a/0x66 [bonding]
 [<c015ccf3>] sys_delete_module+0x191/0x1f1
 [<c0147a30>] ? up_read+0x16/0x2a
 [<c0102a18>] ? restore_all_notrace+0x0/0x18
 [<c0353357>] ? do_page_fault+0x0/0x393
 [<c0102904>] sysenter_do_call+0x12/0x32
---[ end trace 8f3eaeee682a572c ]---

	Any thoughts?  I have not as yet investigated further.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH 0/6] Bonding simplifications and netns support

[Eric W. Biederman]

Jay Vosburgh <fubar@us.ibm.com> writes:

> David Miller <davem@davemloft.net> wrote:
>
>>From: ebiederm@xmission.com (Eric W. Biederman)
>>Date: Thu, 29 Oct 2009 17:16:54 -0700
>>
>>> I recently had it pointed out to me that the bonding driver does not
>>> work in a network namespace.  So I have simplified the bonding driver
>>> a bit, added support for ip link add and ip link del, and finally made
>>> the bonding driver work in multiple network namespaces.
>>> 
>>> The most note worthy change in the patchset is the addition of support
>>> in the networking core for registering a sysfs group for a device.
>>> 
>>> Using this in the bonding driver simplifies the code and removes a
>>> userspace race between actions triggered by the netlink event and the
>>> bonding sysfs attributes appearing.
>>
>>I've tossed patches 1-7 into net-next-2.6, thanks Eric.
>
> 	I put patches 1-7 on a recent net-next-2.6, and from a simple
> "insmod bonding.ko; rmmod bonding" I'm seeing the following:
>
> ------------[ cut here ]------------
> WARNING: at fs/proc/generic.c:847 remove_proc_entry+0x1a8/0x1c7()
> Hardware name: IBM eserver xSeries 220 -[8645]-
> remove_proc_entry: removing non-empty directory 'net/bonding', leaking at least 
> 'bond0'
> Modules linked in: bonding(-) ipv6 microcode loop ppdev sworks_agp parport_pc tg
> 3 e100 agpgart parport mii libphy e1000 edd pata_serverworks [last unloaded: spe
> edstep_lib]
> Pid: 6216, comm: rmmod Not tainted 2.6.32-rc3-devel #19
> Call Trace:
>  [<c012ec9d>] warn_slowpath_common+0x60/0x90
>  [<c012ed01>] warn_slowpath_fmt+0x24/0x27
>  [<c01e3e55>] remove_proc_entry+0x1a8/0x1c7
>  [<e0906435>] ? bond_net_exit+0x0/0xa3 [bonding]
>  [<e09064c3>] bond_net_exit+0x8e/0xa3 [bonding]
>  [<c02e6ee8>] unregister_pernet_gen_subsys+0x23/0x3d
>  [<e0910baa>] bonding_exit+0x3a/0x66 [bonding]
>  [<c015ccf3>] sys_delete_module+0x191/0x1f1
>  [<c0147a30>] ? up_read+0x16/0x2a
>  [<c0102a18>] ? restore_all_notrace+0x0/0x18
>  [<c0353357>] ? do_page_fault+0x0/0x393
>  [<c0102904>] sysenter_do_call+0x12/0x32
> ---[ end trace 8f3eaeee682a572c ]---
>
> 	Any thoughts?  I have not as yet investigated further.

Weird.

We have already run:
rtnl_link_unregister. 
  rtnl_kill_links
    dellink(bond0)
      unregister_netdevice(bond0)
          bond_uninit
             bond_remove_proc_entry


So the proc entry should no longer be there.  I'm a little nervous about
the new unregister_netdevice_many but I don't see any obvious problems with
that code.

Were there by any chance any earlier errors that could have prevented the uninit?
You weren't inserting multiple copies of the bonding driver?

Eric

Re: [PATCH 0/6] Bonding simplifications and netns support

[Jay Vosburgh]

Eric W. Biederman <ebiederm@xmission.com> wrote:

>Jay Vosburgh <fubar@us.ibm.com> writes:
>
>> David Miller <davem@davemloft.net> wrote:
>>
>>>From: ebiederm@xmission.com (Eric W. Biederman)
>>>Date: Thu, 29 Oct 2009 17:16:54 -0700
>>>
>>>> I recently had it pointed out to me that the bonding driver does not
>>>> work in a network namespace.  So I have simplified the bonding driver
>>>> a bit, added support for ip link add and ip link del, and finally made
>>>> the bonding driver work in multiple network namespaces.
>>>> 
>>>> The most note worthy change in the patchset is the addition of support
>>>> in the networking core for registering a sysfs group for a device.
>>>> 
>>>> Using this in the bonding driver simplifies the code and removes a
>>>> userspace race between actions triggered by the netlink event and the
>>>> bonding sysfs attributes appearing.
>>>
>>>I've tossed patches 1-7 into net-next-2.6, thanks Eric.
>>
>> 	I put patches 1-7 on a recent net-next-2.6, and from a simple
>> "insmod bonding.ko; rmmod bonding" I'm seeing the following:
>>
>> ------------[ cut here ]------------
>> WARNING: at fs/proc/generic.c:847 remove_proc_entry+0x1a8/0x1c7()
>> Hardware name: IBM eserver xSeries 220 -[8645]-
>> remove_proc_entry: removing non-empty directory 'net/bonding', leaking at least 
>> 'bond0'
>> Modules linked in: bonding(-) ipv6 microcode loop ppdev sworks_agp parport_pc tg
>> 3 e100 agpgart parport mii libphy e1000 edd pata_serverworks [last unloaded: spe
>> edstep_lib]
>> Pid: 6216, comm: rmmod Not tainted 2.6.32-rc3-devel #19
>> Call Trace:
>>  [<c012ec9d>] warn_slowpath_common+0x60/0x90
>>  [<c012ed01>] warn_slowpath_fmt+0x24/0x27
>>  [<c01e3e55>] remove_proc_entry+0x1a8/0x1c7
>>  [<e0906435>] ? bond_net_exit+0x0/0xa3 [bonding]
>>  [<e09064c3>] bond_net_exit+0x8e/0xa3 [bonding]
>>  [<c02e6ee8>] unregister_pernet_gen_subsys+0x23/0x3d
>>  [<e0910baa>] bonding_exit+0x3a/0x66 [bonding]
>>  [<c015ccf3>] sys_delete_module+0x191/0x1f1
>>  [<c0147a30>] ? up_read+0x16/0x2a
>>  [<c0102a18>] ? restore_all_notrace+0x0/0x18
>>  [<c0353357>] ? do_page_fault+0x0/0x393
>>  [<c0102904>] sysenter_do_call+0x12/0x32
>> ---[ end trace 8f3eaeee682a572c ]---
>>
>> 	Any thoughts?  I have not as yet investigated further.
>
>Weird.
>
>We have already run:
>rtnl_link_unregister. 
>  rtnl_kill_links
>    dellink(bond0)
>      unregister_netdevice(bond0)
>          bond_uninit
>             bond_remove_proc_entry
>
>
>So the proc entry should no longer be there.  I'm a little nervous about
>the new unregister_netdevice_many but I don't see any obvious problems with
>that code.
>
>Were there by any chance any earlier errors that could have prevented the uninit?
>You weren't inserting multiple copies of the bonding driver?

	No, to both questions.  Also, if I back out the 7 bonding
patches, the same insmod / rmmod does not panic.  

	I just set it up and did it again.  Fresh boot of the system
(which doesn't load bonding); "insmod drivers/net/bonding/bonding.ko;
rmmod bonding" and blammo.

	A little bisect action reveals that the problem first appears
after applying the fifth patch (below).  Does a basic insmod / rmmod
cycle work ok for you?  I'm specifying no options to bonding.

	-J

Subject: [PATCH 5/6] bond: Implement a basic set of rtnl link ops
[...]
This implements a basic set of rtnl link ops and takes advantage of
the fact that rtnl_link_unregister kills all of the surviving
devices to all us to kill bond_free_all.  A module alias
is added so ip link add can pull in the bonding module.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 drivers/net/bonding/bond_main.c |   55 +++++++++++++++++++++-----------------
 1 files changed, 30 insertions(+), 25 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 7a37ecf..6da2a82 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4612,22 +4612,6 @@ static void bond_uninit(struct net_device *bond_dev)
 	netif_addr_unlock_bh(bond_dev);
 }

-/* Unregister and free all bond devices.
- * Caller must hold rtnl_lock.
- */
-static void bond_free_all(void)
-{
-	struct bonding *bond, *nxt;
-
-	list_for_each_entry_safe(bond, nxt, &bond_dev_list, bond_list) {
-		struct net_device *bond_dev = bond->dev;
-
-		unregister_netdevice(bond_dev);
-	}
-
-	bond_destroy_proc_dir();
-}
-
 /*------------------------- Module initialization ---------------------------*/

 /*
@@ -5065,6 +5049,23 @@ static int bond_init(struct net_device *bond_dev)
 	return 0;
 }

+static int bond_validate(struct nlattr *tb[], struct nlattr *data[])
+{
+	if (tb[IFLA_ADDRESS]) {
+		if (nla_len(tb[IFLA_ADDRESS]) != ETH_ALEN)
+			return -EINVAL;
+		if (!is_valid_ether_addr(nla_data(tb[IFLA_ADDRESS])))
+			return -EADDRNOTAVAIL;
+	}
+	return 0;
+}
+
+static struct rtnl_link_ops bond_link_ops __read_mostly = {
+	.kind		= "bond",
+	.setup		= bond_setup,
+	.validate	= bond_validate,
+};
+
 /* Create a new bond based on the specified name and bonding parameters.
  * If name is NULL, obtain a suitable "bond%d" name for us.
  * Caller must NOT hold rtnl_lock; we need to release it here before we
@@ -5086,6 +5087,8 @@ int bond_create(const char *name)
 		goto out;
 	}

+	bond_dev->rtnl_link_ops = &bond_link_ops;
+
 	if (!name) {
 		res = dev_alloc_name(bond_dev, "bond%d");
 		if (res < 0)
@@ -5115,6 +5118,10 @@ static int __init bonding_init(void)

 	bond_create_proc_dir();

+	res = rtnl_link_register(&bond_link_ops);
+	if (res)
+		goto err;
+
 	for (i = 0; i < max_bonds; i++) {
 		res = bond_create(NULL);
 		if (res)
@@ -5128,14 +5135,12 @@ static int __init bonding_init(void)
 	register_netdevice_notifier(&bond_netdev_notifier);
 	register_inetaddr_notifier(&bond_inetaddr_notifier);
 	bond_register_ipv6_notifier();
-
-	goto out;
-err:
-	rtnl_lock();
-	bond_free_all();
-	rtnl_unlock();
 out:
 	return res;
+err:
+	rtnl_link_unregister(&bond_link_ops);
+	bond_destroy_proc_dir();
+	goto out;

 }

@@ -5147,9 +5152,8 @@ static void __exit bonding_exit(void)

 	bond_destroy_sysfs();

-	rtnl_lock();
-	bond_free_all();
-	rtnl_unlock();
+	rtnl_link_unregister(&bond_link_ops);
+	bond_destroy_proc_dir();
 }

 module_init(bonding_init);
@@ -5158,3 +5162,4 @@ MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_VERSION);
 MODULE_DESCRIPTION(DRV_DESCRIPTION ", v" DRV_VERSION);
 MODULE_AUTHOR("Thomas Davis, tadavis@lbl.gov and many others");
+MODULE_ALIAS_RTNL_LINK("bond");
-- 
1.6.3.1.54.g99dd.dirty


---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH 0/6] Bonding simplifications and netns support

[Eric W. Biederman]

Jay Vosburgh <fubar@us.ibm.com> writes:

> 	No, to both questions.  Also, if I back out the 7 bonding
> patches, the same insmod / rmmod does not panic.  
>
> 	I just set it up and did it again.  Fresh boot of the system
> (which doesn't load bonding); "insmod drivers/net/bonding/bonding.ko;
> rmmod bonding" and blammo.
>
> 	A little bisect action reveals that the problem first appears
> after applying the fifth patch (below).  Does a basic insmod / rmmod
> cycle work ok for you?  I'm specifying no options to bonding.

It works here.  The only issue I found was that veth wasn't quite
working.   I am wondering if there was some version of the tree
where rtnl_link_unregister is broken and you applied the patches to that.

I tested the net-next tree with my patches at the top:

There are some other differences like I am running a 64bit kernel but
I don't expect that would make a difference in practice.

Eric

commit 6639104bd826e0b1388c69a6b7564fffc636c8a8
Author: Eric W. Biederman <ebiederm@xmission.com>
Date:   Thu Oct 29 23:58:54 2009 +0000

    bond: Get the rtnl_link_ops support correct
    
    - Don't call rtnl_link_unregister if rtnl_link_register fails
    - Set .priv_size so we aren't stomping on uninitialized memory
      when we use netdev_priv, on bond devices created with
      ip link add type bond.
    
    Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Re: [PATCH 0/6] Bonding simplifications and netns support

[Jay Vosburgh]

Eric W. Biederman <ebiederm@xmission.com> wrote:

>Jay Vosburgh <fubar@us.ibm.com> writes:
>
>> 	No, to both questions.  Also, if I back out the 7 bonding
>> patches, the same insmod / rmmod does not panic.  
>>
>> 	I just set it up and did it again.  Fresh boot of the system
>> (which doesn't load bonding); "insmod drivers/net/bonding/bonding.ko;
>> rmmod bonding" and blammo.
>>
>> 	A little bisect action reveals that the problem first appears
>> after applying the fifth patch (below).  Does a basic insmod / rmmod
>> cycle work ok for you?  I'm specifying no options to bonding.
>
>It works here.  The only issue I found was that veth wasn't quite
>working.   I am wondering if there was some version of the tree
>where rtnl_link_unregister is broken and you applied the patches to that.

	Must have been, I did a pull of net-next-2.6 and it seems to
work ok now.  Not sure what it was; I was only a day or so behind.
Anyway, it doesn't panic now; I'll give it some further testing next
week.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH 0/6] Bonding simplifications and netns support

[Eric W. Biederman]

Jay Vosburgh <fubar@us.ibm.com> writes:

> 	I put patches 1-7 on a recent net-next-2.6, and from a simple
> "insmod bonding.ko; rmmod bonding" I'm seeing the following:
>

> 	Any thoughts?  I have not as yet investigated further.

I just tried to reproduce this, without luck.  My best guess is that there is
something odd particular to your setup. 

A simple while :; modprobe bonding ; rmmod bonding ; done runs without problems
for me.

Eric