Merge lp:~jamesodhunt/upstart/bug-980917-reworked into lp:upstart

[James Hunt]

Based upon branch lp:~vorlon/upstart/lp.980917.

* init/main.c:main(): Handle hostile initramfs-less environments by
  calling umask and creating required device nodes as early as possible.
* init/system.c: New functions to simplify code:
  - system_mknod()
  - system_check_file()

[Steve Langasek (vorlon)]

I think there's some very unfortunate scope creep here. My previous revision was addressing what would have been a regression for users of static /dev, due to the incorrect overmounting with devtmpfs. This subsequent change doesn't address any problems related to the /dev/pts case at all, instead adding checks for additional devices that have never been the source of error reports.

+ if (system_check_file ("/dev/null", S_IFCHR, makedev (1, 3)) < 0)
+ needs_devtmpfs = 1;
+
+ if (system_check_file ("/dev/console", S_IFCHR, makedev (5, 1)) < 0)
+ needs_devtmpfs = 1;
+
+ if (system_check_file ("/dev/tty", S_IFCHR, makedev (5, 0)) < 0)
+ needs_devtmpfs = 1;
+

This is a completely unnecessary check. These three device nodes are guaranteed to *always* be present at boot time. Either they're set up by the initramfs, or they're required to be part of /dev on the root filesystem, or the system must be configured for the kernel to automount devtmpfs at boot time. It is therefore *wrong* for upstart to take action when these device nodes are missing, as that means the system is badly broken and requiring admin intervention, and papering over one of the symptoms will only make it harder for the problem to be fixed at its source. This adds complexity where none is needed.

+ if (system_check_file ("/dev/kmsg", S_IFCHR, makedev (1, 11)) < 0)
+ needs_devtmpfs = 1;

This is a device that could possibly be missing (it's not included in the "std" set from MAKEDEV), but it also seems optional... if missing, you just don't get messages logged, right?

I think it's borderline as to whether upstart should do anything to create /dev/kmsg. If it's not present at boot time, chances are that it will be present soon afterwards. On the other hand, if it's going to become "present soon afterwards", it means we have a dynamic /dev, so there's probably no *harm* in mounting /dev here. But I think it's still orthogonal to the problem this branch was supposed to solve and should be treated separately.

So the only part of this that I would include is the fix of the mknod() argument ordering. (Which, btw, you probably never noticed problems with when testing because the node is created by the kernel at the same time as /dev/null, so in practice it's always present by the time devtmpfs is mounted.)

review: Disapprove

[Steve Langasek (vorlon)]

Correction: /dev/ptmx is *not* created at the same time that /dev/null is. So there may be a small race allowing the unix98 pty driver to have not been initialized by the time of devtmpfs mount, whereas the others should never need to be created by hand because they're guaranteed available on the devtmpfs before the handoff to init.

[James Hunt (jamesodhunt)]

Hi Steve,

> This is a completely unnecessary check. These three device nodes are
> guaranteed to *always* be present at boot time.

On Ubuntu we run MAKEDEV when the devtmpfs /tmp is mounted is mounted, but although these devices are expected to exist on a normal working system, I don't see that we can guarantee that for non-Ubuntu systems, particularly those using a static /dev. Hence, the defensive programming to try to recover in the case where 'the impossible' happens.

> Either they're set up by the initramfs, or they're required to be part
> of /dev on the root filesystem, or the system must be configured for the
> kernel to automount devtmpfs at boot time.
> It is therefore *wrong* for upstart to take action when these device nodes
> are missing, as that means the system is badly broken and requiring admin
> intervention

Ok, so unlikely though it is, if an admin _does_ inadvertently remove a crucial device node on a system running Upstart with no initramfs and a static /dev, assuming they have access to another working system, they'll need to do something akin to the following to recover:

- boot with "init=/bin/sh root=/dev/sda1 rootfstype=ext4"
  (remove "quiet" and "splash" if present too).

- run the following to mount disk read-write:

  # mount -oremount,rw /

- recreate any missing device nodes using mknod(1), or better, just run:

  # MAKEDEV std

- either call "sync", remount disks read-only, halt and power-cycle ("sync && mount
  -oremount,ro / && halt"), or just start Upstart:

  # exec /sbin/init </dev/console >/dev/console 2>&1

> This (/dev/kmsg) is a device that could possibly be missing (it's not
> included in the "std" set from MAKEDEV), but it also seems optional...
> if missing, you just don't get messages logged, right?

That's right - Upstarts switches the NIH logger to use /dev/kmsg to make
use of the kernel ring buffer for logging all output. This allows early
Upstart output to appear in the system log when a syslog daemon
eventually starts.

=== modified file 'ChangeLog'
--- ChangeLog	2012-07-31 08:39:57 +0000
+++ ChangeLog	2012-08-03 15:50:43 +0000
@@ -1,3 +1,11 @@
[phone]  James Hunt  <[e-mail]>
+
+	* init/main.c:main(): Handle hostile initramfs-less environments by
+	  calling umask and creating required device nodes as early as possible.
+	* init/system.c: New functions to simplify code:
+	  - system_mknod()
+	  - system_check_file()
+
 2012-07-31  James Hunt  <[e-mail]>
 
 	[ Eric S. Raymond <[e-mail]> ]

=== modified file 'init/main.c'
--- init/main.c	2011-12-15 16:26:34 +0000
+++ init/main.c	2012-08-03 15:50:43 +0000
@@ -28,6 +28,7 @@
 #include <sys/ioctl.h>
 #include <sys/reboot.h>
 #include <sys/resource.h>
+#include <sys/mount.h>
 
 #include <errno.h>
 #include <stdio.h>
@@ -192,6 +193,8 @@
 #ifndef DEBUG
 	if (use_session_bus == FALSE) {
 
+		int needs_devtmpfs = 0;
+
 		/* Check we're root */
 		if (getuid ()) {
 			nih_fatal (_("Need to be root"));
@@ -236,6 +239,70 @@
 		 */
 		setsid ();
 
+		/* Allow devices to be created with the actual perms
+		 * specified.
+		 */
+		(void)umask (0);
+
+		/* Check if key devices already exist; if they do,
+		 * we should assume we don't need to mount /dev.
+		 */
+		if (system_check_file ("/dev/null", S_IFCHR, makedev (1, 3)) < 0)
+			needs_devtmpfs = 1;
+
+		if (system_check_file ("/dev/console", S_IFCHR, makedev (5, 1)) < 0)
+			needs_devtmpfs = 1;
+
+		if (system_check_file ("/dev/tty", S_IFCHR, makedev (5, 0)) < 0)
+			needs_devtmpfs = 1;
+
+		if (system_check_file ("/dev/kmsg", S_IFCHR, makedev (1, 11)) < 0)
+			needs_devtmpfs = 1;
+
+		if (system_check_file ("/dev/ptmx", S_IFCHR, makedev (5, 2)) < 0)
+			needs_devtmpfs = 1;
+
+		if (system_check_file ("/dev/pts", S_IFDIR, 0) < 0)
+			needs_devtmpfs = 1;
+
+		if (needs_devtmpfs) {
+			if (system_mount ("devtmpfs", "/dev", (MS_NOEXEC | MS_NOSUID)) < 0) {
+				NihError *err;
+
+				err = nih_error_get ();
+				nih_error ("%s: %s", _("Unable to mount /dev filesystem"),
+						err->message);
+				nih_free (err);
+			}
+
+			/* Required to exist before /dev/pts accessed */
+			system_mknod ("/dev/ptmx", (S_IFCHR | 0666), makedev (5, 2));
+
+			if (mkdir ("/dev/pts", 0755) < 0 && errno != EEXIST)
+				nih_error ("%s: %s", _("Cannot create directory"), "/dev/pts");
+		}
+
+		if (system_mount ("devpts", "/dev/pts", (MS_NOEXEC | MS_NOSUID)) < 0) {
+			NihError *err;
+
+			err = nih_error_get ();
+			nih_error ("%s: %s", _("Unable to mount /dev/pts filesystem"),
+					err->message);
+			nih_free (err);
+		}
+
+		/* These devices must exist, but we have to have handled the /dev
+		 * check (and possible mount) prior to considering
+		 * creating them. And yet, if /dev is not available from
+		 * the outset and an error occurs, we are unable to report it,
+		 * hence these checks are performed as early as is
+		 * feasible.
+		 */
+		system_mknod ("/dev/null", (S_IFCHR | 0666), makedev (1, 3));
+		system_mknod ("/dev/tty", (S_IFCHR | 0666), makedev (5, 0));
+		system_mknod ("/dev/console", (S_IFCHR | 0600), makedev (5, 1));
+		system_mknod ("/dev/kmsg", (S_IFCHR | 0600), makedev (1, 11));
+
 		/* Set the standard file descriptors to the ordinary console device,
 		 * resetting it to sane defaults unless we're inheriting from another
 		 * init process which we know left it in a sane state.
@@ -271,9 +338,10 @@
 
 		/* Mount the /proc and /sys filesystems, which are pretty much
 		 * essential for any Linux system; not to mention used by
-		 * ourselves.
+		 * ourselves. Also mount /dev/pts to allow CONSOLE_LOG
+		 * to function if booted in an initramfs-less environment.
 		 */
-		if (system_mount ("proc", "/proc") < 0) {
+		if (system_mount ("proc", "/proc", (MS_NODEV | MS_NOEXEC | MS_NOSUID)) < 0) {
 			NihError *err;
 
 			err = nih_error_get ();
@@ -282,7 +350,7 @@
 			nih_free (err);
 		}
 
-		if (system_mount ("sysfs", "/sys") < 0) {
+		if (system_mount ("sysfs", "/sys", (MS_NODEV | MS_NOEXEC | MS_NOSUID)) < 0) {
 			NihError *err;
 
 			err = nih_error_get ();
@@ -290,6 +358,7 @@
 				err->message);
 			nih_free (err);
 		}
+
 	} else {
 		nih_log_set_priority (NIH_LOG_DEBUG);
 		nih_debug ("Running with UID %d as PID %d (PPID %d)",

=== modified file 'init/system.c'
--- init/system.c	2011-12-09 14:07:11 +0000
+++ init/system.c	2012-08-03 15:50:43 +0000
@@ -164,9 +164,10 @@
 /**
  * system_mount:
  * @type: filesystem type,
- * @dir: mountpoint.
+ * @dir: mountpoint,
+ * @flags: mount flags.
  *
- * Mount the kernel filesystem @type at @dir, if not already mounted.  This
+ * Mount the kernel filesystem @type at @dir with @flags, if not already mounted.  This
  * is used to ensure that the proc and sysfs filesystems are always
  * available.
  *
@@ -177,7 +178,8 @@
  **/
 int
 system_mount (const char *type,
-	      const char *dir)
+	      const char *dir,
+	      unsigned long flags)
 {
 	nih_local char *parent = NULL;
 	char *          ptr;
@@ -206,9 +208,65 @@
 		return 0;
 
 	/* Mount the filesystem */
-	if (mount ("none", dir, type,
-		   MS_NODEV | MS_NOEXEC | MS_NOSUID, NULL) < 0)
+	if (mount ("none", dir, type, flags, NULL) < 0)
 		nih_return_system_error (-1);
 
 	return 0;
 }
+
+/**
+ * system_mknod:
+ *
+ * @path: full path,
+ * @mode: mode to create device with,
+ * @dev: device major and minor numbers.
+ *
+ * Create specified device.
+ *
+ * Note that depending on the device, if an error occurs
+ * it may not be reportable, hence no return value,
+ * but an attempt to display an error.
+ **/
+void
+system_mknod (const char *path, mode_t mode, dev_t dev)
+{
+	nih_assert (path);
+
+	if (mknod (path, mode, dev) < 0 && errno != EEXIST)
+		nih_error ("%s: %s", _("Unable to create device"), path);
+}
+
+/**
+ * system_check_file:
+ *
+ * @path: full path,
+ * @type: file type,
+ * @dev: device major and minor numbers (only checked for character and
+ * block devices).
+ *
+ * Perform checks on specified file.
+ *
+ * Returns: 0 if device exists and has the specified @path,
+ * @type and @dev attributes, else -1.
+ **/
+int
+system_check_file (const char *path, mode_t type, dev_t dev)
+{
+	struct stat  statbuf;
+	int          ret;
+
+	nih_assert (path);
+
+	ret = stat (path, &statbuf);
+
+	if (ret < 0 || ! ((statbuf.st_mode & S_IFMT) == type))
+		return -1;
+
+	if (type == S_IFCHR || type == S_IFBLK) {
+		if (major (statbuf.st_rdev) != major (dev)
+			|| minor (statbuf.st_rdev) != minor (dev))
+		return -1;
+	}
+
+	return 0;
+}

=== modified file 'init/system.h'
--- init/system.h	2011-05-12 20:42:28 +0000
+++ init/system.h	2012-08-03 15:50:43 +0000
@@ -35,7 +35,13 @@
 int system_setup_console (ConsoleType type, int reset)
 	__attribute__ ((warn_unused_result));
 
-int system_mount         (const char *type, const char *dir)
+int system_mount         (const char *type, const char *dir,
+			  unsigned long flags)
+	__attribute__ ((warn_unused_result));
+
+void system_mknod        (const char *path, mode_t mode, dev_t dev);
+
+int  system_check_file   (const char *path, mode_t type, dev_t dev)
 	__attribute__ ((warn_unused_result));
 
 NIH_END_EXTERN