improve atomicity of device creation

This is a patch we've written and applied in Ubuntu to improve the atomicity of devmapper device creation somewhat, and avoid races with udev.

Scott

Scott James Remnant
[email protected]

@@ -252,12 +252,11 @@
 static int _add_dev_node(const char *dev_name, uint32_t major, uint32_t minor,
 			 uid_t uid, gid_t gid, mode_t mode)
 {
-	char path[PATH_MAX];
+	char path[PATH_MAX], tmppath[PATH_MAX + 7];
 	struct stat info;
 	dev_t dev = MKDEV(major, minor);
 	mode_t old_mask;
-
-	_build_dev_path(path, sizeof(path), dev_name);
+	int retval;
 
 	if (stat(path, &info) >= 0) {
 		if (!S_ISBLK(info.st_mode)) {

@@ -269,31 +268,39 @@
 		/* If right inode already exists we don't touch uid etc. */
 		if (info.st_rdev == dev)
 			return 1;
-
-		if (unlink(path) < 0) {
-			log_error("Unable to unlink device node for '%s'",
-				  dev_name);
-			return 0;
-		}
 	}
 
+	_build_dev_path(path, sizeof(path), dev_name);
+	strcpy (tmppath, path);
+	strcat (tmppath, ".dm-tmp");
+
 	old_mask = umask(0);
-	if (mknod(path, S_IFBLK | mode, dev) < 0) {
-		log_error("Unable to make device node for '%s'", dev_name);
+	retval = mknod(tmppath, S_IFBLK | mode, dev);
+	umask(old_mask);
+	if (retval < 0) {
+		log_error("Unable to make temporary device node for '%s'", dev_name);
 		return 0;
 	}
-	umask(old_mask);
 
-	if (chown(path, uid, gid) < 0) {
+	if (chown(tmppath, uid, gid) < 0) {
 		log_error("%s: chown failed: %s", path, strerror(errno));
+		unlink(tmppath);
 		return 0;
 	}
 
 #ifdef HAVE_SELINUX
-	if (!dm_set_selinux_context(path, S_IFBLK))
+	if (!dm_set_selinux_context(tmppath, S_IFBLK)) {
+		unlink(tmppath);
 		return 0;
+	}
 #endif
 
+	if (rename(tmppath, path) < 0) {
+		log_error("Unable to replace device node for '%s'", dev_name);
+		unlink(tmppath);
+		return 0;
+	}
+
 	return 1;
 }
 

-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php

_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

BTW I won't be applying this upstream - these nodes are designed to be managed by libdevmapper not udev so it's not relevant. I'm awaiting patches that have been promised to an agreed design that will provide for correct libdevmapper/udev integration without races/deadlocks etc.

Alasdair

[email protected]

What is that agreed design?

Scott

Scott James Remnant
[email protected]

-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php

_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

In simple terms:

- udev takes over full responsibility for creating nodes [this will probably be a ./configure option]

- udev provides interface we use to wait until it has finished processing all the outstanding requests we sent it.

Key point is that our udev requests appear in batches, then we wait for the batch to complete, as when the system is under memory pressure, udev userspace may find itself blocked processing the first of the batch until just before we issue the wait at the end of the batch. [Note how libdevmapper today pushes all the requests onto an internal stack and processes them all together at the end.]

Alasdair

[email protected]

Excellent, this is what we've wanted for a while.

What's this interface?

I had a proposed patch that made udev write its sequence number to the kernel, and thus allowed any kobject add event to have a "bottom half" that could happen when udev had finished. The idea was that the existing ioctls could then just block.

The other patch we tried a couple of releases ago was have devmapper spin until udev had caught up.

Scott

Scott James Remnant
[email protected]

-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php

_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Sounds fine.

You want to block processes (doing ioctl) until udev has finished? Like a kernel userspace transaction? Where could you see which requests you will need to fulfill to continue the hanging processes?

It could work, but if something goes wrong, it seems pretty hard to recover from such a failure. Just remember how firmware loading works. :)

The currently running udev events are in: /dev/.udev/queue/<seqnum>. The last seqnum that arrived in the udev daemon is: /dev/.udev/uevent_seqnum. So udev's state is exported.

We can make the kernel's kobject_uevent() return the generated seqnum to the caller. A devmapper ioctl, an existing one to extend, or a new one like "create node" can return the seqnum to libdevmapper. Libdevmapper then checks and spins until /dev/.udev/uevent_seqnum has seen the seqnum, and /dev/.udev/queue/<seqnum> does not exist.

Kay

-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php

Is 32 bits enough?

We added 'uint32_t padding;' recently which we I reckon we could now use for this.

dm_kobject_uevent() and alloc_dev() could keep a new field in struct mapped_device up-to-date, and a new function could return its value to dm-ioctl.c code to place into a renamed 'padding' field before returning to userspace?

Alasdair

[email protected]

In the kernel it is 64 bit. Udev also uses 64bit number, but it does not care about the value these days (netlink is never out of order, unlike /sbin/hotplug). Only the udev queue export would break if the number wraps around and duplicates are generated in a window of 180 seconds (the event process kills itself after that time). :)

So we would need to strip the upper 32 bit of the numbers we see from udev, and handle the case where the 32 bit number wraps around.

Yes, we would return the kernel generated seqnum to the libdevmapper ioctl.

Should we create an additional new iocl to request the device node creation, or add that call to an existing one?

Could it be that we want to pass more properties to the event, which are added as environment keys?

Kay

-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php

Currently we get an ADD from alloc_dev -> add_disk -> register_disk. But the device is not usable until the dm resume ioctl, so ideally that one should be ignored, and it's the CHANGE issued by dm resume that udev should act upon. (That's not essential though - the /dev node can be created in response to the ADD, but nothing in udev userspace should attempt to open it until after the CHANGE is received.)

Don't think so - we'll be handing full control of ownership/permissions over to udev.

Alasdair

[email protected]

Should be OK I think. Otherwise we can append a 64-bit field to the struct - with care, this can be done without breaking compatibility.

Alasdair

[email protected]

Without synchronization between libdevmapper and udev, you can't predict which one will win in creating the node, right?
So this patch makes is more likely to create a valid device node, but it can still happen, that libdevmapper replaces the udev nodes, which is not what we want, right?

Kay

-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php

No, the patch is slightly skewed so that udev always wins. udev will always adopt an existing device node, adjusting permissions as necessary. The patch makes devmapper "back off" if the device node exists.

So you end up with either:

1) devmapper creates device node
2) udev adopts device node and sets permissions

or:

1) udev creates device node and sets permissions
2) devmapper no-ops since it already exists

Scott

Scott James Remnant
[email protected]

-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php

_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

...because libdevmapper has no notion that something other than itself could have created it. That's a bug, but provided the proper udev integration isn't much longer in coming, I shan't bother to fix it.

Alasdair

[email protected]

That's ok ;) no pressure on the patch, I just wanted to make sure that we weren't carrying anything that hadn't at least been shown upstream to see whether you wanted it or not.

Obviously we'd prefer the long-term solution, our patch is just a short term one since we're already relying on udev for devmapper work today.

Scott

Scott James Remnant
[email protected]

-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php

_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

There is a window between stat() and rename() in libdevmapper, how can you konw that "udev always wins"?

Kay