[PATCH] (1/5) replay netdev notifier events on registration

[PATCH] (1/5) replay netdev notifier events on registration

[Stephen Hemminger]

Several protocols register for network device notification to detect new new network devices;
but then have to walk the device list to capture the devices that are already up.
This leaves a exposed window between when the notifier is registered and when the list walk
occurs. Also, in several cases, there is a different code path for the pre-existing and
new devices which leads to bug exposure.

The solution is to replay the registration and up events for existing devices into the
new notifier.

All notifiers in 2.6.1 have been audited and the other patches fix places
where protocols were doing there own registered device discovery loop.

Thanks to Al Viro for the suggestion.

diff -Nru a/net/core/dev.c b/net/core/dev.c
--- a/net/core/dev.c	Thu Dec 18 15:32:11 2003
+++ b/net/core/dev.c	Thu Dec 18 15:32:11 2003
@@ -946,11 +946,29 @@
  *	The notifier passed is linked into the kernel structures and must
  *	not be reused until it has been unregistered. A negative errno code
  *	is returned on a failure.
+ *
+ * 	When registered all registration and up events are replayed
+ *	to the new notifier to allow device to have a race free 
+ *	view of the network device list.
  */
 
 int register_netdevice_notifier(struct notifier_block *nb)
 {
-	return notifier_chain_register(&netdev_chain, nb);
+	struct net_device *dev;
+	int err;
+
+	rtnl_lock();
+	err = notifier_chain_register(&netdev_chain, nb);
+	if (!err) {
+		for (dev = dev_base; dev; dev = dev->next) {
+			nb->notifier_call(nb, NETDEV_REGISTER, dev);
+
+			if (dev->flags & IFF_UP) 
+				nb->notifier_call(nb, NETDEV_UP, dev);
+		}
+	}
+	rtnl_unlock();
+	return err;
 }
 
 /**

Re: [PATCH] (1/5) replay netdev notifier events on registration

[David S. Miller]

On Tue, 13 Jan 2004 10:58:43 -0800
Stephen Hemminger <shemminger@osdl.org> wrote:

> Several protocols register for network device notification to detect new new network devices;
> but then have to walk the device list to capture the devices that are already up.
> This leaves a exposed window between when the notifier is registered and when the list walk
> occurs. Also, in several cases, there is a different code path for the pre-existing and
> new devices which leads to bug exposure.
> 
> The solution is to replay the registration and up events for existing devices into the
> new notifier.
> 
> All notifiers in 2.6.1 have been audited and the other patches fix places
> where protocols were doing there own registered device discovery loop.
> 
> Thanks to Al Viro for the suggestion.

Looks good.... are you absolutely sure no remaining notifiers will
barf if they get a register for an already existing device?  I know up
events should be ok...

Anyways, I applied all 5 patches, thanks.

[PATCH] decnet initialization race

[Stephen Hemminger]

Decnet exposes itself to proc and packets before it has finished initializing.
This was always a race, but the notifier replay might expose it worse.

diff -Nru a/net/decnet/af_decnet.c b/net/decnet/af_decnet.c
--- a/net/decnet/af_decnet.c	Wed Jan 14 16:27:05 2004
+++ b/net/decnet/af_decnet.c	Wed Jan 14 16:27:05 2004
@@ -2363,17 +2363,16 @@
 	if (!dn_sk_cachep)
 		return -ENOMEM;
 
-	sock_register(&dn_family_ops);
-	dev_add_pack(&dn_dix_packet_type);
-	register_netdevice_notifier(&dn_dev_notifier);
-
-	proc_net_fops_create("decnet", S_IRUGO, &dn_socket_seq_fops);
-
 	dn_neigh_init();
 	dn_dev_init();
 	dn_route_init();
 	dn_fib_init();
 
+	sock_register(&dn_family_ops);
+	dev_add_pack(&dn_dix_packet_type);
+	register_netdevice_notifier(&dn_dev_notifier);
+
+	proc_net_fops_create("decnet", S_IRUGO, &dn_socket_seq_fops);
 	dn_register_sysctl();
 
 	return 0;

Re: [PATCH] decnet initialization race

[David S. Miller]

On Wed, 14 Jan 2004 16:40:45 -0800
Stephen Hemminger <shemminger@osdl.org> wrote:

> Decnet exposes itself to proc and packets before it has finished initializing.
> This was always a race, but the notifier replay might expose it worse.

Applied.

Although, Arnaldo is not the listed DECNET maintainer last time I checked,
or is he being CC:'d for another reason? :-)

[PATH] atm/clip device discovery on init not needed

[Stephen Hemminger]

Atm/clip driver was doing device list walk to find interfaces.
This will no longer be needed now that events are replayed when
register_netdevice_notifier is called (see earlier patch)

diff -Nru a/net/atm/clip.c b/net/atm/clip.c
--- a/net/atm/clip.c	Wed Jan 14 16:27:05 2004
+++ b/net/atm/clip.c	Wed Jan 14 16:27:05 2004
@@ -754,9 +754,6 @@
 		printk(KERN_ERR "register_netdevice_notifier failed\n");
 	if (register_inetaddr_notifier(&clip_inet_notifier))
 		printk(KERN_ERR "register_inetaddr_notifier failed\n");
-	for (dev = clip_devs; dev; dev = PRIV(dev)->next)
-		if (dev->flags & IFF_UP)
-			(void) to_atmarpd(act_up,PRIV(dev)->number,0);
 	return 0;
 }
 

Re: [PATH] atm/clip device discovery on init not needed

[David S. Miller]

On Wed, 14 Jan 2004 16:43:38 -0800
Stephen Hemminger <shemminger@osdl.org> wrote:

> Atm/clip driver was doing device list walk to find interfaces.
> This will no longer be needed now that events are replayed when
> register_netdevice_notifier is called (see earlier patch)

Applied, thanks Stephen.

Re: [PATH] atm/clip device discovery on init not needed

[Stephen Hemminger]

Forgot to resulting unused declaration.
diff -Nru a/net/atm/clip.c b/net/atm/clip.c
--- a/net/atm/clip.c	Thu Jan 15 14:49:45 2004
+++ b/net/atm/clip.c	Thu Jan 15 14:49:45 2004
@@ -731,8 +731,6 @@
 
 static int atm_init_atmarp(struct atm_vcc *vcc)
 {
-	struct net_device *dev;
-
 	if (atmarpd) return -EADDRINUSE;
 	if (start_timer) {
 		start_timer = 0;

Re: [PATH] atm/clip device discovery on init not needed

[David S. Miller]

On Thu, 15 Jan 2004 14:59:25 -0800
Stephen Hemminger <shemminger@osdl.org> wrote:

> Forgot to resulting unused declaration.

Applied, thanks Stephen.

Re: [PATCH] (1/5) replay netdev notifier events on registration

[Stephen Hemminger]

> Looks good.... are you absolutely sure no remaining notifiers will
> barf if they get a register for an already existing device?  I know up
> events should be ok...

Certainty is impossible; but here is what I saw..

Possible problems:
qeth: s390 driver -- bug, code is narcissistic and thinks it only gets
	notified about it's own devices.

atm/mpc: only looks for "lec" devices, don't know if they could exist
	before it starts.

Unrelated problems:
ddp: registers for notifier before it is initialized
ipmr: no locking for add/delete
ipfwadm: no module owner on /proc interface


The following are okay:

bonding: since must be master or slave can't exist yet
bpqether: patched, was doing replay already
ppoe: only cares about down
dlci: only cares about unregister
lapbether: patched, was doing replay already
vlan: only cares about groups that can't exist yet
aarp: only down
ddp: only cares about down 
atm/clip: see additional patch
ax25: only cares about ax25 devices can't exist yet
bridge: only cares about bridged devices can't exist yet
dst: only down
rtnetlink: ok no sockets can be open yet
decnet: see additional patch about startup order
dn_rules: looks okay
econet: only unregister
arp: only address changes
devinet: only inet devices can't exist yet
fib_frontend: looks okay happens before devices anyway
fib_rules: looks okay happens before devices anyway
ipmr: only unregister
ipqueue: only down
ipfwadm: only for chains which can't exist yet
ip6_sockglue: okay
ndisc: patched
ip6: down only
ipx: only for ipx devices which can't exist at initial
irsyms: harmless, whole notify could be removed
netrom: down only
packet: only cares about sockets which can't exist at initial
rose: down only
wanpipe: only cares about sockets which can't exist at initial
x25: only cares about x25 which can't exist yet
xfrm_policy: down only

Re: [PATCH] (1/5) replay netdev notifier events on registration

[David S. Miller]

On Wed, 14 Jan 2004 16:44:16 -0800
Stephen Hemminger <shemminger@osdl.org> wrote:

> Possible problems:
> qeth: s390 driver -- bug, code is narcissistic and thinks it only gets
> 	notified about it's own devices.
> 
> atm/mpc: only looks for "lec" devices, don't know if they could exist
> 	before it starts.

These are both hard errors and potential bogus pointer derefences, they both
assume the type of dev->priv.  The atm/mpc case has a netdev->name==NULL
test which is a funny relic :-)

Both these cases ought to be fixed.  However, the atm/mpc case poses an issue,
how to identify "my" devices?  We've established that the textual name is
basically arbitrary and not a reliable key.  Currently I see only two
possible reliable solutions, but I like neither of them:

1) Device driver doing this needs to keep own list of net devices it
   has created.  Then it's notifiier code does something like:

	if (!find_in_mpoa_devlist(dev))
		return NOTIFY_DONE;

2) Add a type cookie or similar to the generic netdev, only devices
   which need to identify themselves in these generic kind of cases
   add identifier values there, so currently that would be MPOA and
   QETH, then the code goes:

	if (dev->type_cookie == NETDEV_TYPE_MPOA)
		return NOTIFY_DONE;

But as stated I think both of these ideas absolutely stink.

... wait...

Ok, I have an idea, consider this.  We add a netdev->notifier()
method.  We create a new routine to net/core/dev.c:

static void run_netdev_notifiers(int event, struct net_device *dev)
{
	notifier_call_chain(&netdev_chain, event, dev);

	if (dev->notifier)
		dev->notifier(dev, event);
}

Then replace all the notifier_call_chain(&netdev_chain, ...) calls
in net/core/dev.c with invocations of run_netdev_notifiers().

I believe we can (and thus should) add an ASSERT_RTNL() to this new
run_netdev_notifiers() functions, although I'm not %100 sure.

What do you think Stephen?

> Unrelated problems:
> ddp: registers for notifier before it is initialized

Just moving it down to the end of atalk_init() should fix this?
Actually, I don't really see any potential problem here.

> ipmr: no locking for add/delete

Not a problem, RTNL semaphore is held.

> ipfwadm: no module owner on /proc interface

Please elaborate.  I don't see the ipfwadm netdev event notifier
messing with procfs stuff, or is this happen at a level or two of
indirection somewhere else?

> The following are okay:

Thanks for the audit.

Re: [PATCH] (1/5) replay netdev notifier events on registration

[Stephen Hemminger]

On Thu, 15 Jan 2004 00:42:55 -0800
"David S. Miller" <davem@redhat.com> wrote:

> On Wed, 14 Jan 2004 16:44:16 -0800
> Stephen Hemminger <shemminger@osdl.org> wrote:
> 
> > Possible problems:
> > qeth: s390 driver -- bug, code is narcissistic and thinks it only gets
> > 	notified about it's own devices.
> > 
> > atm/mpc: only looks for "lec" devices, don't know if they could exist
> > 	before it starts.
> 
> These are both hard errors and potential bogus pointer derefences, they both
> assume the type of dev->priv.  The atm/mpc case has a netdev->name==NULL
> test which is a funny relic :-)
> 
> Both these cases ought to be fixed.  However, the atm/mpc case poses an issue,
> how to identify "my" devices?  We've established that the textual name is
> basically arbitrary and not a reliable key.  Currently I see only two
> possible reliable solutions, but I like neither of them:
> 
> 1) Device driver doing this needs to keep own list of net devices it
>    has created.  Then it's notifiier code does something like:
> 
> 	if (!find_in_mpoa_devlist(dev))
> 		return NOTIFY_DONE;

This is done other places, but your right it scales poorly

> 2) Add a type cookie or similar to the generic netdev, only devices
>    which need to identify themselves in these generic kind of cases
>    add identifier values there, so currently that would be MPOA and
>    QETH, then the code goes:
> 
> 	if (dev->type_cookie == NETDEV_TYPE_MPOA)
> 		return NOTIFY_DONE;

faster, but uglier.

> But as stated I think both of these ideas absolutely stink.

Well, we could switch to an object language with RTTI ;-(

> ... wait...
> 
> Ok, I have an idea, consider this.  We add a netdev->notifier()
> method.  We create a new routine to net/core/dev.c:
> 
> static void run_netdev_notifiers(int event, struct net_device *dev)
> {
> 	notifier_call_chain(&netdev_chain, event, dev);
> 
> 	if (dev->notifier)
> 		dev->notifier(dev, event);
> }
> 
> Then replace all the notifier_call_chain(&netdev_chain, ...) calls
> in net/core/dev.c with invocations of run_netdev_notifiers().
> 
> I believe we can (and thus should) add an ASSERT_RTNL() to this new
> run_netdev_notifiers() functions, although I'm not %100 sure.
> 
> What do you think Stephen?

Feeling stupid this morning, how wold this help?  Would device set
dev->notifier and not register for other notifications?
Rather than a single notifier why not add a dev->notify_chain and
do:
	notifier_call_chain(&netdev_chain, event, dev);
	notifier_call_chain(dev->notify_chain, event, dev);

But the whole programming model of responding to callbacks seems bassackwards
in these cases, because the device can process the same events (up/down)
on the front side (open/close) rather than getting callbacks.  At least in the
qeth case it seems like a messed up design.  


> > Unrelated problems:
> > ddp: registers for notifier before it is initialized
> 
> Just moving it down to the end of atalk_init() should fix this?
I'll test a patch for it.

> Actually, I don't really see any potential problem here.
> 
> > ipmr: no locking for add/delete
> 
> Not a problem, RTNL semaphore is held.
> 
> > ipfwadm: no module owner on /proc interface

> Please elaborate.  I don't see the ipfwadm netdev event notifier
> messing with procfs stuff, or is this happen at a level or two of
> indirection somewhere else?

Just never looked there before... patch coming.

Re: [PATCH] (1/5) replay netdev notifier events on registration

[David S. Miller]

On Thu, 15 Jan 2004 10:16:17 -0800
Stephen Hemminger <shemminger@osdl.org> wrote:

> On Thu, 15 Jan 2004 00:42:55 -0800
> "David S. Miller" <davem@redhat.com> wrote:
> 
> > Ok, I have an idea, consider this.  We add a netdev->notifier()
> > method.  We create a new routine to net/core/dev.c:
> > 
> > static void run_netdev_notifiers(int event, struct net_device *dev)
> > {
> > 	notifier_call_chain(&netdev_chain, event, dev);
> > 
> > 	if (dev->notifier)
> > 		dev->notifier(dev, event);
> > }
> > 
> > Then replace all the notifier_call_chain(&netdev_chain, ...) calls
> > in net/core/dev.c with invocations of run_netdev_notifiers().
> > 
> > I believe we can (and thus should) add an ASSERT_RTNL() to this new
> > run_netdev_notifiers() functions, although I'm not %100 sure.
> > 
> > What do you think Stephen?
> 
> Feeling stupid this morning, how wold this help?  Would device set
> dev->notifier and not register for other notifications?

That's correct.  This eliminates the "am I a type FOO device", because
this netdev->notifier() call would be implication only run on the correct
device types.

> Rather than a single notifier why not add a dev->notify_chain and
> do:
> 	notifier_call_chain(&netdev_chain, event, dev);
> 	notifier_call_chain(dev->notify_chain, event, dev);
> 
> But the whole programming model of responding to callbacks seems bassackwards
> in these cases, because the device can process the same events (up/down)
> on the front side (open/close) rather than getting callbacks.  At least in the
> qeth case it seems like a messed up design.  

qeth is a mess period, it tries to be overly clever because of the things it is
trying to achieve and as a result it's an abominable piece of complexity.

I don't see how a "dev->notify_chain" like scheme could work...
Oh I see, this way the driver can register multiple private device-type specific
notifiers.  Yes, this looks like a fine way to do this too.

But really, the driver too could do all of it's "notifiers" in the one dev->notifier()
method.

I'm not overly picky about using one scheme over another.

Re: [PATCH] (1/5) replay netdev notifier events on registration

[chas williams]

In message <20040115004255.62dc8b95.davem@redhat.com>,"David S. Miller" writes:
>2) Add a type cookie or similar to the generic netdev, only devices
>   which need to identify themselves in these generic kind of cases
>   add identifier values there, so currently that would be MPOA and
>   QETH, then the code goes:
>
>	if (dev->type_cookie == NETDEV_TYPE_MPOA)
>		return NOTIFY_DONE;

while just as ugly it would probably suffice to check dev->start_xmit()
to make sure its the routine you know it should be.  since the number 
of drivers that need to do this seems small this might be simpler.