[0/6] Allow registration/change name notifications to fail

[0/6] Allow registration/change name notifications to fail

[Herbert Xu]

Hi Dave:

This series of patches adds support to let netdev registration
and change name events to fail.  Failures of the former kind
prevents the device from registered while the latter attempts
to roll back the change (which unfortunately can also fail).

If the failure occurs while a netdev is being registered,
then the registration as a whole will fail.  If it occurs
when a protocol is being registered, then that protocol
will fail to register instead.

Having this allows us to flag allocations failures in a 
meaningful way rather than letting the next action that
requires what is allocated to flag the failure.

In particular, this allows the reporting of failures to
allocate the IPv4/IPv6 device objects.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH 1/6] [NET]: Call uninit if necessary in register_netdevice

[Herbert Xu]

[NET]: Call uninit if necessary in register_netdevice

This patch makes register_netdevice call dev->uninit if the regsitration
fails after dev->init has completed successfully.  Very few drivers use
the init/uninit calls but at least one (drivers/net/wan/sealevel.c) may
leak without this change.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 net/core/dev.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index ee40355..a284c9b 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3333,7 +3333,7 @@ int register_netdevice(struct net_device *dev)
 
 	if (!dev_valid_name(dev->name)) {
 		ret = -EINVAL;
-		goto out;
+		goto err_uninit;
 	}
 
 	dev->ifindex = dev_new_index();
@@ -3347,7 +3347,7 @@ int register_netdevice(struct net_device *dev)
 			= hlist_entry(p, struct net_device, name_hlist);
 		if (!strncmp(d->name, dev->name, IFNAMSIZ)) {
 			ret = -EEXIST;
-			goto out;
+			goto err_uninit;
 		}
 	}
 
@@ -3407,7 +3407,7 @@ int register_netdevice(struct net_device *dev)
 
 	ret = netdev_register_sysfs(dev);
 	if (ret)
-		goto out;
+		goto err_uninit;
 	dev->reg_state = NETREG_REGISTERED;
 
 	/*
@@ -3432,6 +3432,11 @@ int register_netdevice(struct net_device *dev)
 
 out:
 	return ret;
+
+err_uninit:
+	if (dev->uninit)
+		dev->uninit(dev);
+	goto out;
 }
 
 /**

Re: [PATCH 1/6] [NET]: Call uninit if necessary in register_netdevice

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 26 Jul 2007 17:09:08 +0800

> [NET]: Call uninit if necessary in register_netdevice
> 
> This patch makes register_netdevice call dev->uninit if the regsitration
> fails after dev->init has completed successfully.  Very few drivers use
> the init/uninit calls but at least one (drivers/net/wan/sealevel.c) may
> leak without this change.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Looks great, applied.

[PATCH 2/6] [NET]: Take dev_base_lock when moving device name hash list entry

[Herbert Xu]

[NET]: Take dev_base_lock when moving device name hash list entry

When we added name-based hashing the dev_base_lock was designated as the
lock to take when changing the name hash list.  Unfortunately, because
it was a preexisting lock that just happened to be taken in the right
spots we neglected to take it in dev_change_name.

The race can affect calles of __dev_get_by_name that do so without taking
the RTNL.  They may end up walking down the wrong hash chain and end up
missing the device that they're looking for.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 net/core/dev.c |    4 ++++
 1 files changed, 4 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index a284c9b..0a35c15 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -839,8 +839,12 @@ int dev_change_name(struct net_device *dev, char *newname)
 		strlcpy(dev->name, newname, IFNAMSIZ);
 
 	device_rename(&dev->dev, dev->name);
+
+	write_lock_bh(&dev_base_lock);
 	hlist_del(&dev->name_hlist);
 	hlist_add_head(&dev->name_hlist, dev_name_hash(dev->name));
+	write_unlock_bh(&dev_base_lock);
+
 	raw_notifier_call_chain(&netdev_chain, NETDEV_CHANGENAME, dev);
 
 	return err;

Re: [PATCH 2/6] [NET]: Take dev_base_lock when moving device name hash list entry

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 26 Jul 2007 17:09:30 +0800

> [NET]: Take dev_base_lock when moving device name hash list entry
> 
> When we added name-based hashing the dev_base_lock was designated as the
> lock to take when changing the name hash list.  Unfortunately, because
> it was a preexisting lock that just happened to be taken in the right
> spots we neglected to take it in dev_change_name.
> 
> The race can affect calles of __dev_get_by_name that do so without taking
> the RTNL.  They may end up walking down the wrong hash chain and end up
> missing the device that they're looking for.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Good catch, applied, thanks Herbert.

[PATCH 3/6] [NET] loopback: Panic if registration fails

[Herbert Xu]

[NET] loopback: Panic if registration fails

Because IPv4 and IPv6 both depend on the presence of the loopback device
to function, failure in registration the loopback device should be fatal.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/net/loopback.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index 6ba6ed2..5106c23 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -229,7 +229,12 @@ struct net_device loopback_dev = {
 /* Setup and register the loopback device. */
 static int __init loopback_init(void)
 {
-	return register_netdev(&loopback_dev);
+	int err = register_netdev(&loopback_dev);
+
+	if (err)
+		panic("loopback: Failed to register netdevice: %d\n", err);
+
+	return err;
 };
 
 module_init(loopback_init);

Re: [PATCH 3/6] [NET] loopback: Panic if registration fails

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 26 Jul 2007 17:09:33 +0800

> [NET] loopback: Panic if registration fails
> 
> Because IPv4 and IPv6 both depend on the presence of the loopback device
> to function, failure in registration the loopback device should be fatal.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied.

It might be nice to tightly integrate the loopback driver into
net/core/dev.c because that is what is happening anyways and it would
allow us to cons the thing up by hand and deal with these dependencies
more reliably.

[PATCH 4/6] [NET]: Allow netdev REGISTER/CHANGENAME events to fail

[Herbert Xu]

[NET]: Allow netdev REGISTER/CHANGENAME events to fail

This patch adds code to allow errors to be passed up from event
handlers of NETDEV_REGISTER and NETDEV_CHANGENAME.  It also adds
the notifier_from_errno/notifier_to_errnor helpers to pass the
errno value up to the notifier caller.

If an error is detected when a device is registered, it causes
that operation to fail.  A NETDEV_UNREGISTER will be sent to
all event handlers.

Similarly if NETDEV_CHANGENAME fails the original name is restored
and a new NETDEV_CHANGENAME event is sent.

As such all event handlers must be idempotent with respect to
these events.

When an event handler is registered NETDEV_REGISTER events are
sent for all devices currently registered.  Should any of them
fail, we will send NETDEV_GOING_DOWN/NETDEV_DOWN/NETDEV_UNREGISTER
events to that handler for the devices which have already been
registered with it.  The handler registration itself will fail.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 include/linux/notifier.h |   13 +++++++++
 net/core/dev.c           |   62 +++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 65 insertions(+), 10 deletions(-)

diff --git a/include/linux/notifier.h b/include/linux/notifier.h
index be3f2bb..fad7ff1 100644
--- a/include/linux/notifier.h
+++ b/include/linux/notifier.h
@@ -157,6 +157,19 @@ extern int __srcu_notifier_call_chain(struct srcu_notifier_head *nh,
  */
 #define NOTIFY_STOP		(NOTIFY_OK|NOTIFY_STOP_MASK)
 
+/* Encapsulate (negative) errno value (in particular, NOTIFY_BAD <=> EPERM). */
+static inline int notifier_from_errno(int err)
+{
+	return NOTIFY_STOP_MASK | (NOTIFY_OK - err);
+}
+
+/* Restore (negative) errno value from notify return value. */
+static inline int notifier_to_errno(int ret)
+{
+	ret &= ~NOTIFY_STOP_MASK;
+	return ret > NOTIFY_OK ? NOTIFY_OK - ret : 0;
+}
+
 /*
  *	Declared notifiers so far. I can imagine quite a few more chains
  *	over time (eg laptop power reset chains, reboot chain (to clean 
diff --git a/net/core/dev.c b/net/core/dev.c
index 0a35c15..d94b178 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -817,7 +817,9 @@ int dev_alloc_name(struct net_device *dev, const char *name)
  */
 int dev_change_name(struct net_device *dev, char *newname)
 {
+	char oldname[IFNAMSIZ];
 	int err = 0;
+	int ret;
 
 	ASSERT_RTNL();
 
@@ -827,6 +829,8 @@ int dev_change_name(struct net_device *dev, char *newname)
 	if (!dev_valid_name(newname))
 		return -EINVAL;
 
+	memcpy(oldname, dev->name, IFNAMSIZ);
+
 	if (strchr(newname, '%')) {
 		err = dev_alloc_name(dev, newname);
 		if (err < 0)
@@ -838,6 +842,7 @@ int dev_change_name(struct net_device *dev, char *newname)
 	else
 		strlcpy(dev->name, newname, IFNAMSIZ);
 
+rollback:
 	device_rename(&dev->dev, dev->name);
 
 	write_lock_bh(&dev_base_lock);
@@ -845,7 +850,20 @@ int dev_change_name(struct net_device *dev, char *newname)
 	hlist_add_head(&dev->name_hlist, dev_name_hash(dev->name));
 	write_unlock_bh(&dev_base_lock);
 
-	raw_notifier_call_chain(&netdev_chain, NETDEV_CHANGENAME, dev);
+	ret = raw_notifier_call_chain(&netdev_chain, NETDEV_CHANGENAME, dev);
+	ret = notifier_to_errno(ret);
+
+	if (ret) {
+		if (err) {
+			printk(KERN_ERR
+			       "%s: name change rollback failed: %d.\n",
+			       dev->name, ret);
+		} else {
+			err = ret;
+			memcpy(dev->name, oldname, IFNAMSIZ);
+			goto rollback;
+		}
+	}
 
 	return err;
 }
@@ -1058,20 +1076,43 @@ int dev_close(struct net_device *dev)
 int register_netdevice_notifier(struct notifier_block *nb)
 {
 	struct net_device *dev;
+	struct net_device *last;
 	int err;
 
 	rtnl_lock();
 	err = raw_notifier_chain_register(&netdev_chain, nb);
-	if (!err) {
-		for_each_netdev(dev) {
-			nb->notifier_call(nb, NETDEV_REGISTER, dev);
+	if (err)
+		goto unlock;
 
-			if (dev->flags & IFF_UP)
-				nb->notifier_call(nb, NETDEV_UP, dev);
-		}
+	for_each_netdev(dev) {
+		err = nb->notifier_call(nb, NETDEV_REGISTER, dev);
+		err = notifier_to_errno(err);
+		if (err)
+			goto rollback;
+
+		if (!(dev->flags & IFF_UP))
+			continue;
+
+		nb->notifier_call(nb, NETDEV_UP, dev);
 	}
+
+unlock:
 	rtnl_unlock();
 	return err;
+
+rollback:
+	last = dev;
+	for_each_netdev(dev) {
+		if (dev == last)
+			break;
+
+		if (dev->flags & IFF_UP) {
+			nb->notifier_call(nb, NETDEV_GOING_DOWN, dev);
+			nb->notifier_call(nb, NETDEV_DOWN, dev);
+		}
+		nb->notifier_call(nb, NETDEV_UNREGISTER, dev);
+	}
+	goto unlock;
 }
 
 /**
@@ -3430,9 +3471,10 @@ int register_netdevice(struct net_device *dev)
 	write_unlock_bh(&dev_base_lock);
 
 	/* Notify protocols, that a new device appeared. */
-	raw_notifier_call_chain(&netdev_chain, NETDEV_REGISTER, dev);
-
-	ret = 0;
+	ret = raw_notifier_call_chain(&netdev_chain, NETDEV_REGISTER, dev);
+	ret = notifier_to_errno(ret);
+	if (ret)
+		unregister_netdevice(dev);
 
 out:
 	return ret;

Re: [PATCH 4/6] [NET]: Allow netdev REGISTER/CHANGENAME events to fail

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 26 Jul 2007 17:09:36 +0800

> [NET]: Allow netdev REGISTER/CHANGENAME events to fail
> 
> This patch adds code to allow errors to be passed up from event
> handlers of NETDEV_REGISTER and NETDEV_CHANGENAME.  It also adds
> the notifier_from_errno/notifier_to_errnor helpers to pass the
> errno value up to the notifier caller.
> 
> If an error is detected when a device is registered, it causes
> that operation to fail.  A NETDEV_UNREGISTER will be sent to
> all event handlers.
> 
> Similarly if NETDEV_CHANGENAME fails the original name is restored
> and a new NETDEV_CHANGENAME event is sent.
> 
> As such all event handlers must be idempotent with respect to
> these events.
> 
> When an event handler is registered NETDEV_REGISTER events are
> sent for all devices currently registered.  Should any of them
> fail, we will send NETDEV_GOING_DOWN/NETDEV_DOWN/NETDEV_UNREGISTER
> events to that handler for the devices which have already been
> registered with it.  The handler registration itself will fail.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied.

Interesting encoding scheme :-)

[PATCH 5/6] [IPV4/IPV6]: Fail registration if inet device construction fails

[Herbert Xu]

[IPV4/IPV6]: Fail registration if inet device construction fails

Now that netdev notifications can fail, we can use this to signal
errors during registration for IPv4/IPv6.  In particular, if we
fail to allocate memory for the inet device, we can fail the netdev
registration.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 net/ipv4/devinet.c  |    5 ++---
 net/ipv6/addrconf.c |    8 +++++---
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index abf6352..5b77bda 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1056,10 +1056,9 @@ static int inetdev_event(struct notifier_block *this, unsigned long event,
 	if (!in_dev) {
 		if (event == NETDEV_REGISTER) {
 			in_dev = inetdev_init(dev);
+			if (!in_dev)
+				return notifier_from_errno(-ENOMEM);
 			if (dev == &loopback_dev) {
-				if (!in_dev)
-					panic("devinet: "
-					      "Failed to create loopback\n");
 				IN_DEV_CONF_SET(in_dev, NOXFRM, 1);
 				IN_DEV_CONF_SET(in_dev, NOPOLICY, 1);
 			}
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 0601292..91ef3be 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2256,14 +2256,14 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
 	struct net_device *dev = (struct net_device *) data;
 	struct inet6_dev *idev = __in6_dev_get(dev);
 	int run_pending = 0;
+	int err;
 
 	switch(event) {
 	case NETDEV_REGISTER:
 		if (!idev && dev->mtu >= IPV6_MIN_MTU) {
 			idev = ipv6_add_dev(dev);
 			if (!idev)
-				printk(KERN_WARNING "IPv6: add_dev failed for %s\n",
-					dev->name);
+				return notifier_from_errno(-ENOMEM);
 		}
 		break;
 	case NETDEV_UP:
@@ -2373,7 +2373,9 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
 					      NULL);
 			addrconf_sysctl_register(idev, &idev->cnf);
 #endif
-			snmp6_register_dev(idev);
+			err = snmp6_register_dev(idev);
+			if (err)
+				return notifier_from_errno(err);
 		}
 		break;
 	}

Re: [PATCH 5/6] [IPV4/IPV6]: Fail registration if inet device construction fails

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 26 Jul 2007 17:09:38 +0800

> [IPV4/IPV6]: Fail registration if inet device construction fails
> 
> Now that netdev notifications can fail, we can use this to signal
> errors during registration for IPv4/IPv6.  In particular, if we
> fail to allocate memory for the inet device, we can fail the netdev
> registration.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied.

[PATCH 6/6] [IPV6]: Remove circular dependency on if_inet6.h

[Herbert Xu]

[IPV6]: Remove circular dependency on if_inet6.h

net/if_inet6.h includes linux/ipv6.h which also tries to include
net/if_inet6.h.  Since the latter only needs it for forward
declarations, we can fix this by adding the declarations.

A number of files are implicitly including net/if_inet6.h through
linux/ipv6.h.  They also use net/ipv6.h so this patch includes
net/if_inet6.h there.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 include/linux/ipv6.h |    5 ++++-
 include/net/ipv6.h   |    1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
index 97983dc..4ca60c3 100644
--- a/include/linux/ipv6.h
+++ b/include/linux/ipv6.h
@@ -219,7 +219,6 @@ enum {
 #include <linux/tcp.h>
 #include <linux/udp.h>
 
-#include <net/if_inet6.h>       /* struct ipv6_mc_socklist */
 #include <net/inet_sock.h>
 
 static inline struct ipv6hdr *ipv6_hdr(const struct sk_buff *skb)
@@ -273,6 +272,10 @@ struct tcp6_request_sock {
 	struct inet6_request_sock tcp6rsk_inet6;
 };
 
+struct ipv6_mc_socklist;
+struct ipv6_ac_socklist;
+struct ipv6_fl_socklist;
+
 /**
  * struct ipv6_pinfo - ipv6 private area
  *
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 46b9dce..9059e0e 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -17,6 +17,7 @@
 
 #include <linux/ipv6.h>
 #include <linux/hardirq.h>
+#include <net/if_inet6.h>
 #include <net/ndisc.h>
 #include <net/flow.h>
 #include <net/snmp.h>

Re: [PATCH 6/6] [IPV6]: Remove circular dependency on if_inet6.h

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 26 Jul 2007 17:09:40 +0800

> [IPV6]: Remove circular dependency on if_inet6.h
> 
> net/if_inet6.h includes linux/ipv6.h which also tries to include
> net/if_inet6.h.  Since the latter only needs it for forward
> declarations, we can fix this by adding the declarations.
> 
> A number of files are implicitly including net/if_inet6.h through
> linux/ipv6.h.  They also use net/ipv6.h so this patch includes
> net/if_inet6.h there.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Also applied, thanks a lot.

Re: [0/6] Allow registration/change name notifications to fail

[Stephen Hemminger]

On Thu, 26 Jul 2007 17:07:25 +0800
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> Hi Dave:
> 
> This series of patches adds support to let netdev registration
> and change name events to fail.  Failures of the former kind
> prevents the device from registered while the latter attempts
> to roll back the change (which unfortunately can also fail).
> 
> If the failure occurs while a netdev is being registered,
> then the registration as a whole will fail.  If it occurs
> when a protocol is being registered, then that protocol
> will fail to register instead.
> 
> Having this allows us to flag allocations failures in a 
> meaningful way rather than letting the next action that
> requires what is allocated to flag the failure.
> 
> In particular, this allows the reporting of failures to
> allocate the IPv4/IPv6 device objects.
> 
> Cheers,

Thanks for looking at this, are there bugs this fixes so it should
go to stable?

Re: [0/6] Allow registration/change name notifications to fail

[Herbert Xu]

On Thu, Jul 26, 2007 at 11:37:15AM +0100, Stephen Hemminger wrote:
>
> Thanks for looking at this, are there bugs this fixes so it should
> go to stable?

Well it does fix bugs but I'm a bit anxious about pushing it
to stable straight away because the likelihood of it breaking
something else is just as great.

Perhaps once it's been tested a while then we could push it back.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt