[PATCH v3 0/2] Fix uevent race in register_netdevice()

[PATCH v3 0/2] Fix uevent race in register_netdevice()

[Kalle Valo]

I'm trying to fix a race in register_netdevice(). The problem is that
there's a uevent to userspace before the netdevice is ready for use. The
problem is described here:

https://bugzilla.kernel.org/show_bug.cgi?id=15606

I have sent few different ways to fix this, but none of them have been
really usable. Now I came up with a way which changes the driver core
to make it possible send the uevent in a separate call. This is a clean
and safe way to fix the race. Downside is that two new functions are
added to the driver core interface.

Please comment.

---

Kalle Valo (2):
      driver core: add device_add_noevent() and device_uevent()
      net: postpone net device uevent to fix a race


 drivers/base/core.c    |   76 +++++++++++++++++++++++++++++++++++++-----------
 include/linux/device.h |    2 +
 net/core/dev.c         |    3 ++
 net/core/net-sysfs.c   |    2 +
 4 files changed, 65 insertions(+), 18 deletions(-)

[PATCH v3 1/2] driver core: add device_add_noevent() and device_uevent()

[Kalle Valo]

From: Kalle Valo <kalle.valo@atheros.com>

To make it possible to postpone uevent after the device is registered
create the two new functions: device_add_noevent() and device_uevent().
This is needed by register_netdevice() to fix a race where the netdevice
is not ready until the initialisation has finished.

Signed-off-by: Kalle Valo <kalle.valo@atheros.com>
---
 drivers/base/core.c    |   76 +++++++++++++++++++++++++++++++++++++-----------
 include/linux/device.h |    2 +
 2 files changed, 61 insertions(+), 17 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 81b78ed..54208e0 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -872,22 +872,7 @@ int device_private_init(struct device *dev)
 	return 0;
 }
 
-/**
- * device_add - add device to device hierarchy.
- * @dev: device.
- *
- * This is part 2 of device_register(), though may be called
- * separately _iff_ device_initialize() has been called separately.
- *
- * This adds @dev to the kobject hierarchy via kobject_add(), adds it
- * to the global and sibling lists for the device, then
- * adds it to the other relevant subsystems of the driver model.
- *
- * NOTE: _Never_ directly free @dev after calling this function, even
- * if it returned an error! Always use put_device() to give up your
- * reference instead.
- */
-int device_add(struct device *dev)
+static int __device_add(struct device *dev, bool event)
 {
 	struct device *parent = NULL;
 	struct class_interface *class_intf;
@@ -974,7 +959,9 @@ int device_add(struct device *dev)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 					     BUS_NOTIFY_ADD_DEVICE, dev);
 
-	kobject_uevent(&dev->kobj, KOBJ_ADD);
+	if (event)
+		kobject_uevent(&dev->kobj, KOBJ_ADD);
+
 	bus_probe_device(dev);
 	if (parent)
 		klist_add_tail(&dev->p->knode_parent,
@@ -1026,6 +1013,61 @@ name_error:
 }
 
 /**
+ * device_add - add device to device hierarchy.
+ * @dev: device.
+ *
+ * This is part 2 of device_register(), though may be called
+ * separately _iff_ device_initialize() has been called separately.
+ *
+ * This adds @dev to the kobject hierarchy via kobject_add(), adds it
+ * to the global and sibling lists for the device, then
+ * adds it to the other relevant subsystems of the driver model.
+ *
+ * NOTE: _Never_ directly free @dev after calling this function, even
+ * if it returned an error! Always use put_device() to give up your
+ * reference instead.
+ */
+int device_add(struct device *dev)
+{
+	return __device_add(dev, true);
+}
+
+/**
+ * device_add - add device to device hierarchy but omit uevent
+ * @dev: device.
+ *
+ * This is part 2 of device_register(), though may be called
+ * separately _iff_ device_initialize() has been called separately.
+ *
+ * This adds @dev to the kobject hierarchy via kobject_add(), adds it
+ * to the global and sibling lists for the device, then
+ * adds it to the other relevant subsystems of the driver model.
+ *
+ * This version differs from device_add() so that it does not emit uevent.
+ * Instead call device_uevent() separately.
+ *
+ * NOTE: _Never_ directly free @dev after calling this function, even
+ * if it returned an error! Always use put_device() to give up your
+ * reference instead.
+ */
+int device_add_noevent(struct device *dev)
+{
+	return __device_add(dev, false);
+}
+
+/**
+ * device_uevent - emit uevent for device addition
+ * @dev: device.
+ *
+ * This is part 2 of device_add_noevent(). It emits the signal which was
+ * not sent when device_add_noevent() was called.
+ */
+void device_uevent(struct device *dev)
+{
+	kobject_uevent(&dev->kobj, KOBJ_ADD);
+}
+
+/**
  * device_register - register a device with the system.
  * @dev: pointer to the device structure
  *
diff --git a/include/linux/device.h b/include/linux/device.h
index ab8dfc0..408d4f0 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -546,6 +546,8 @@ extern int __must_check device_register(struct device *dev);
 extern void device_unregister(struct device *dev);
 extern void device_initialize(struct device *dev);
 extern int __must_check device_add(struct device *dev);
+extern int __must_check device_add_noevent(struct device *dev);
+extern void device_uevent(struct device *dev);
 extern void device_del(struct device *dev);
 extern int device_for_each_child(struct device *dev, void *data,
 		     int (*fn)(struct device *dev, void *data));

[PATCH v3 1/2] driver core: add device_add_noevent() and device_uevent()

[Kalle Valo]

From: Kalle Valo <kalle.valo-DlyHzToyqoxBDgjK7y7TUQ@public.gmane.org>

To make it possible to postpone uevent after the device is registered
create the two new functions: device_add_noevent() and device_uevent().
This is needed by register_netdevice() to fix a race where the netdevice
is not ready until the initialisation has finished.

Signed-off-by: Kalle Valo <kalle.valo-DlyHzToyqoxBDgjK7y7TUQ@public.gmane.org>
---
 drivers/base/core.c    |   76 +++++++++++++++++++++++++++++++++++++-----------
 include/linux/device.h |    2 +
 2 files changed, 61 insertions(+), 17 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 81b78ed..54208e0 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -872,22 +872,7 @@ int device_private_init(struct device *dev)
 	return 0;
 }
 
-/**
- * device_add - add device to device hierarchy.
- * @dev: device.
- *
- * This is part 2 of device_register(), though may be called
- * separately _iff_ device_initialize() has been called separately.
- *
- * This adds @dev to the kobject hierarchy via kobject_add(), adds it
- * to the global and sibling lists for the device, then
- * adds it to the other relevant subsystems of the driver model.
- *
- * NOTE: _Never_ directly free @dev after calling this function, even
- * if it returned an error! Always use put_device() to give up your
- * reference instead.
- */
-int device_add(struct device *dev)
+static int __device_add(struct device *dev, bool event)
 {
 	struct device *parent = NULL;
 	struct class_interface *class_intf;
@@ -974,7 +959,9 @@ int device_add(struct device *dev)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 					     BUS_NOTIFY_ADD_DEVICE, dev);
 
-	kobject_uevent(&dev->kobj, KOBJ_ADD);
+	if (event)
+		kobject_uevent(&dev->kobj, KOBJ_ADD);
+
 	bus_probe_device(dev);
 	if (parent)
 		klist_add_tail(&dev->p->knode_parent,
@@ -1026,6 +1013,61 @@ name_error:
 }
 
 /**
+ * device_add - add device to device hierarchy.
+ * @dev: device.
+ *
+ * This is part 2 of device_register(), though may be called
+ * separately _iff_ device_initialize() has been called separately.
+ *
+ * This adds @dev to the kobject hierarchy via kobject_add(), adds it
+ * to the global and sibling lists for the device, then
+ * adds it to the other relevant subsystems of the driver model.
+ *
+ * NOTE: _Never_ directly free @dev after calling this function, even
+ * if it returned an error! Always use put_device() to give up your
+ * reference instead.
+ */
+int device_add(struct device *dev)
+{
+	return __device_add(dev, true);
+}
+
+/**
+ * device_add - add device to device hierarchy but omit uevent
+ * @dev: device.
+ *
+ * This is part 2 of device_register(), though may be called
+ * separately _iff_ device_initialize() has been called separately.
+ *
+ * This adds @dev to the kobject hierarchy via kobject_add(), adds it
+ * to the global and sibling lists for the device, then
+ * adds it to the other relevant subsystems of the driver model.
+ *
+ * This version differs from device_add() so that it does not emit uevent.
+ * Instead call device_uevent() separately.
+ *
+ * NOTE: _Never_ directly free @dev after calling this function, even
+ * if it returned an error! Always use put_device() to give up your
+ * reference instead.
+ */
+int device_add_noevent(struct device *dev)
+{
+	return __device_add(dev, false);
+}
+
+/**
+ * device_uevent - emit uevent for device addition
+ * @dev: device.
+ *
+ * This is part 2 of device_add_noevent(). It emits the signal which was
+ * not sent when device_add_noevent() was called.
+ */
+void device_uevent(struct device *dev)
+{
+	kobject_uevent(&dev->kobj, KOBJ_ADD);
+}
+
+/**
  * device_register - register a device with the system.
  * @dev: pointer to the device structure
  *
diff --git a/include/linux/device.h b/include/linux/device.h
index ab8dfc0..408d4f0 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -546,6 +546,8 @@ extern int __must_check device_register(struct device *dev);
 extern void device_unregister(struct device *dev);
 extern void device_initialize(struct device *dev);
 extern int __must_check device_add(struct device *dev);
+extern int __must_check device_add_noevent(struct device *dev);
+extern void device_uevent(struct device *dev);
 extern void device_del(struct device *dev);
 extern int device_for_each_child(struct device *dev, void *data,
 		     int (*fn)(struct device *dev, void *data));

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/2] net: postpone net device uevent to fix a race

[Kalle Valo]

From: Kalle Valo <kalle.valo@atheros.com>

There's a race in register_netdevice() so that the udev event is sent before
the device is actually ready. This was visible with flimflam, chrome os
connection manager:

00:21:35 roska flimflamd[2598]: src/udev.c:add_net_device()
00:21:35 roska flimflamd[2598]: connman_inet_ifname: SIOCGIFNAME(index
   4): No such device

So the kobject is visible in udev before the device is ready.

The issue is reported here:

https://bugzilla.kernel.org/show_bug.cgi?id=15606

Fix this by postponing the uevent with device_add_noevent() and
calling device_uevent() once the device is really ready.

Thanks to Johannes Berg for the original idea.

Signed-off-by: Kalle Valo <kalle.valo@atheros.com>
---
 net/core/dev.c       |    3 +++
 net/core/net-sysfs.c |    2 +-
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 856b6ee..80cfa23 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5454,6 +5454,9 @@ int register_netdevice(struct net_device *dev)
 		rollback_registered(dev);
 		dev->reg_state = NETREG_UNREGISTERED;
 	}
+
+	device_uevent(&(dev->dev));
+
 	/*
 	 *	Prevent userspace races by waiting until the network
 	 *	device is fully setup before sending notifications.
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 5ceb257..c10b1d5 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -1337,7 +1337,7 @@ int netdev_register_kobject(struct net_device *net)
 #endif
 #endif /* CONFIG_SYSFS */
 
-	error = device_add(dev);
+	error = device_add_noevent(dev);
 	if (error)
 		return error;
 

Re: [PATCH v3 0/2] Fix uevent race in register_netdevice()

[David Miller]

From: Kalle Valo <kvalo@adurom.com>
Date: Mon, 16 May 2011 17:46:30 +0300

> I'm trying to fix a race in register_netdevice(). The problem is that
> there's a uevent to userspace before the netdevice is ready for use. The
> problem is described here:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=15606
> 
> I have sent few different ways to fix this, but none of them have been
> really usable. Now I came up with a way which changes the driver core
> to make it possible send the uevent in a separate call. This is a clean
> and safe way to fix the race. Downside is that two new functions are
> added to the driver core interface.
> 
> Please comment.

This doesn't work.

The sysfs file will still be there before the uevent, so any
process can go in there, and see the inconsistent state.

Re: [PATCH v3 0/2] Fix uevent race in register_netdevice()

[Kalle Valo]

David Miller <davem@davemloft.net> writes:

> From: Kalle Valo <kvalo@adurom.com>
> Date: Mon, 16 May 2011 17:46:30 +0300
>
>> I'm trying to fix a race in register_netdevice(). The problem is that
>> there's a uevent to userspace before the netdevice is ready for use. The
>> problem is described here:
>> 
>> https://bugzilla.kernel.org/show_bug.cgi?id=15606
>> 
>> I have sent few different ways to fix this, but none of them have been
>> really usable. Now I came up with a way which changes the driver core
>> to make it possible send the uevent in a separate call. This is a clean
>> and safe way to fix the race. Downside is that two new functions are
>> added to the driver core interface.
>> 
>> Please comment.
>
> This doesn't work.
>
> The sysfs file will still be there before the uevent, so any
> process can go in there, and see the inconsistent state.

I considered that user space would not notice the device until the
uevent is emitted so that wouldn't matter that much.

But it's back to the drawing board again. I'm running out of options
how to do this in a not so intrusive way. Do you have any tips how to
fix the race for good?

-- 
Kalle Valo