upstart

Merge lp:~vorlon/upstart/lp.980917 into lp:upstart

  1. lp.980917
  2. Merge into trunk
Proposed by Steve Langasek
Status: Work in progress
Proposed branch: lp:~vorlon/upstart/lp.980917
Merge into: lp:upstart
Diff against target: 135 lines (+50/-9)
3 files modified
init/main.c (+42/-3)
init/system.c (+6/-5)
init/system.h (+2/-1)
To merge this branch: bzr merge lp:~vorlon/upstart/lp.980917
Reviewer Review Type Date Requested Status
James Hunt Needs Fixing
Upstart Reviewers Pending
Review via email: mp+117985@code.launchpad.net

Description of the change

Build on top of lp:~jamesodhunt/upstart/bug-980917, addressing the case where
we would mount /dev unnecessarily (and possibly contrary to the system's
filesystem configuration)

To post a comment you must log in.
Revision history for this message
James Hunt (jamesodhunt) wrote :

Hi Steve,

Good catch! The patch seems to have a stray 'needs_devtmpfs' at line 304:

301 if (stat ("/dev/ptmx", &statbuf) < 0 || !S_ISCHR(statbuf.st_mode)
302 || major(statbuf.st_dev) != 5 || minor(statbuf.st_dev) != 2)
303 needs_devtmpfs = 1;
304 if (needs_devtmpfs
305 || stat("/dev/pts", &statbuf) < 0 || !S_ISDIR(statbuf.st_mode))
306 needs_devtmpfs = 1;

Also, I noticed (gcc didn't ;-) that the mknod parameters have been inadvertently transposed. To be safe we should also specify the permissions for the device nodes we're creating (and possibly explicitly set umask).

Finally, I think we need a slightly more holistic view on Upstart-in-initramfs-less-environments since we cannot assume any devices exist, including /dev/null, /dev/console, /dev/kmsg, /dev/tty. On Ubuntu, I don't think we'd see an issue since when a system is installed from the live media, some devices already exist. However, we cannot assume that for all systems.

review: Needs Fixing
Reply
lp:~vorlon/upstart/lp.980917 updated
1376. By Steve Langasek

fix ordering of mknod args

1377. By Steve Langasek

set correct permissions on /dev/ptmx when creating

Revision history for this message
Steve Langasek (vorlon) wrote :

> The patch seems to have a stray 'needs_devtmpfs' at line 304:

Sorry, it's not stray, just non-idiomatic. The intent was to avoid additional filesystem checks in the case where we already know the answer. It should probably have been written:

               if (stat ("/dev/ptmx", &statbuf) < 0 || !S_ISCHR(statbuf.st_mode)
                   || major(statbuf.st_dev) != 5 || minor(statbuf.st_dev) != 2
                   || stat("/dev/pts", &statbuf) < 0 || !S_ISDIR(statbuf.st_mode))
                       needs_devtmpfs = 1;

> Also, I noticed (gcc didn't ;-) that the mknod parameters have been inadvertently
> transposed. To be safe we should also specify the permissions for the device nodes
> we're creating (and possibly explicitly set umask).

Right - I've pushed an update to the branch for these three issues.

Reply
lp:~vorlon/upstart/lp.980917 updated
1378. By Steve Langasek

skip non-idiomatic duplication of needs_devtmpfs

Unmerged revisions

1378. By Steve Langasek

skip non-idiomatic duplication of needs_devtmpfs

1377. By Steve Langasek

set correct permissions on /dev/ptmx when creating

1376. By Steve Langasek

fix ordering of mknod args

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'init/main.c'
2--- init/main.c 2011-12-15 16:26:34 +0000
3+++ init/main.c 2012-08-03 23:21:19 +0000
4@@ -28,6 +28,7 @@
5 #include <sys/ioctl.h>
6 #include <sys/reboot.h>
7 #include <sys/resource.h>
8+#include <sys/mount.h>
9
10 #include <errno.h>
11 #include <stdio.h>
12@@ -192,6 +193,9 @@
13 #ifndef DEBUG
14 if (use_session_bus == FALSE) {
15
16+ struct stat statbuf;
17+ int needs_devtmpfs = 0;
18+
19 /* Check we're root */
20 if (getuid ()) {
21 nih_fatal (_("Need to be root"));
22@@ -271,9 +275,10 @@
23
24 /* Mount the /proc and /sys filesystems, which are pretty much
25 * essential for any Linux system; not to mention used by
26- * ourselves.
27+ * ourselves. Also mount /dev/pts to allow CONSOLE_LOG
28+ * to function if booted in an initramfs-less environment.
29 */
30- if (system_mount ("proc", "/proc") < 0) {
31+ if (system_mount ("proc", "/proc", (MS_NODEV | MS_NOEXEC | MS_NOSUID)) < 0) {
32 NihError *err;
33
34 err = nih_error_get ();
35@@ -282,7 +287,7 @@
36 nih_free (err);
37 }
38
39- if (system_mount ("sysfs", "/sys") < 0) {
40+ if (system_mount ("sysfs", "/sys", (MS_NODEV | MS_NOEXEC | MS_NOSUID)) < 0) {
41 NihError *err;
42
43 err = nih_error_get ();
44@@ -290,6 +295,40 @@
45 err->message);
46 nih_free (err);
47 }
48+ /* Check if /dev/ptmx and /dev/pts already exist; if they do,
49+ * we should assume we don't need to mount /dev.
50+ */
51+ if (stat ("/dev/ptmx", &statbuf) < 0 || !S_ISCHR(statbuf.st_mode)
52+ || major(statbuf.st_dev) != 5 || minor(statbuf.st_dev) != 2
53+ || stat("/dev/pts", &statbuf) < 0 || !S_ISDIR(statbuf.st_mode))
54+ needs_devtmpfs = 1;
55+
56+ if (needs_devtmpfs) {
57+ if (system_mount ("devtmpfs", "/dev", (MS_NOEXEC | MS_NOSUID)) < 0) {
58+ NihError *err;
59+
60+ err = nih_error_get ();
61+ nih_error ("%s: %s", _("Unable to mount /dev filesystem"),
62+ err->message);
63+ nih_free (err);
64+ }
65+
66+ /* Required to exist before /dev/pts accessed */
67+ if (mknod ("/dev/ptmx", (S_IFCHR | 0666), makedev (5, 2)) < 0 && errno != EEXIST)
68+ nih_error ("Unable to create /dev/ptmx");
69+
70+ if (mkdir ("/dev/pts", 0755) < 0 && errno != EEXIST)
71+ nih_error ("%s: %s", _("Cannot create directory"), "/dev/pts");
72+ }
73+
74+ if (system_mount ("devpts", "/dev/pts", (MS_NOEXEC | MS_NOSUID)) < 0) {
75+ NihError *err;
76+
77+ err = nih_error_get ();
78+ nih_error ("%s: %s", _("Unable to mount /dev/pts filesystem"),
79+ err->message);
80+ nih_free (err);
81+ }
82 } else {
83 nih_log_set_priority (NIH_LOG_DEBUG);
84 nih_debug ("Running with UID %d as PID %d (PPID %d)",
85
86=== modified file 'init/system.c'
87--- init/system.c 2011-12-09 14:07:11 +0000
88+++ init/system.c 2012-08-03 23:21:19 +0000
89@@ -164,9 +164,10 @@
90 /**
91 * system_mount:
92 * @type: filesystem type,
93- * @dir: mountpoint.
94+ * @dir: mountpoint,
95+ * @flags: mount flags.
96 *
97- * Mount the kernel filesystem @type at @dir, if not already mounted. This
98+ * Mount the kernel filesystem @type at @dir with @flags, if not already mounted. This
99 * is used to ensure that the proc and sysfs filesystems are always
100 * available.
101 *
102@@ -177,7 +178,8 @@
103 **/
104 int
105 system_mount (const char *type,
106- const char *dir)
107+ const char *dir,
108+ unsigned long flags)
109 {
110 nih_local char *parent = NULL;
111 char * ptr;
112@@ -206,8 +208,7 @@
113 return 0;
114
115 /* Mount the filesystem */
116- if (mount ("none", dir, type,
117- MS_NODEV | MS_NOEXEC | MS_NOSUID, NULL) < 0)
118+ if (mount ("none", dir, type, flags, NULL) < 0)
119 nih_return_system_error (-1);
120
121 return 0;
122
123=== modified file 'init/system.h'
124--- init/system.h 2011-05-12 20:42:28 +0000
125+++ init/system.h 2012-08-03 23:21:19 +0000
126@@ -35,7 +35,8 @@
127 int system_setup_console (ConsoleType type, int reset)
128 __attribute__ ((warn_unused_result));
129
130-int system_mount (const char *type, const char *dir)
131+int system_mount (const char *type, const char *dir,
132+ unsigned long flags)
133 __attribute__ ((warn_unused_result));
134
135 NIH_END_EXTERN

Subscribers

People subscribed via source and target branches