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

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
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.

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
=== modified file 'init/main.c'
--- init/main.c 2011-12-15 16:26:34 +0000
+++ init/main.c 2012-08-03 23:21:19 +0000
@@ -28,6 +28,7 @@
28#include <sys/ioctl.h>28#include <sys/ioctl.h>
29#include <sys/reboot.h>29#include <sys/reboot.h>
30#include <sys/resource.h>30#include <sys/resource.h>
31#include <sys/mount.h>
3132
32#include <errno.h>33#include <errno.h>
33#include <stdio.h>34#include <stdio.h>
@@ -192,6 +193,9 @@
192#ifndef DEBUG193#ifndef DEBUG
193 if (use_session_bus == FALSE) {194 if (use_session_bus == FALSE) {
194195
196 struct stat statbuf;
197 int needs_devtmpfs = 0;
198
195 /* Check we're root */199 /* Check we're root */
196 if (getuid ()) {200 if (getuid ()) {
197 nih_fatal (_("Need to be root"));201 nih_fatal (_("Need to be root"));
@@ -271,9 +275,10 @@
271275
272 /* Mount the /proc and /sys filesystems, which are pretty much276 /* Mount the /proc and /sys filesystems, which are pretty much
273 * essential for any Linux system; not to mention used by277 * essential for any Linux system; not to mention used by
274 * ourselves.278 * ourselves. Also mount /dev/pts to allow CONSOLE_LOG
279 * to function if booted in an initramfs-less environment.
275 */280 */
276 if (system_mount ("proc", "/proc") < 0) {281 if (system_mount ("proc", "/proc", (MS_NODEV | MS_NOEXEC | MS_NOSUID)) < 0) {
277 NihError *err;282 NihError *err;
278283
279 err = nih_error_get ();284 err = nih_error_get ();
@@ -282,7 +287,7 @@
282 nih_free (err);287 nih_free (err);
283 }288 }
284289
285 if (system_mount ("sysfs", "/sys") < 0) {290 if (system_mount ("sysfs", "/sys", (MS_NODEV | MS_NOEXEC | MS_NOSUID)) < 0) {
286 NihError *err;291 NihError *err;
287292
288 err = nih_error_get ();293 err = nih_error_get ();
@@ -290,6 +295,40 @@
290 err->message);295 err->message);
291 nih_free (err);296 nih_free (err);
292 }297 }
298 /* Check if /dev/ptmx and /dev/pts already exist; if they do,
299 * we should assume we don't need to mount /dev.
300 */
301 if (stat ("/dev/ptmx", &statbuf) < 0 || !S_ISCHR(statbuf.st_mode)
302 || major(statbuf.st_dev) != 5 || minor(statbuf.st_dev) != 2
303 || stat("/dev/pts", &statbuf) < 0 || !S_ISDIR(statbuf.st_mode))
304 needs_devtmpfs = 1;
305
306 if (needs_devtmpfs) {
307 if (system_mount ("devtmpfs", "/dev", (MS_NOEXEC | MS_NOSUID)) < 0) {
308 NihError *err;
309
310 err = nih_error_get ();
311 nih_error ("%s: %s", _("Unable to mount /dev filesystem"),
312 err->message);
313 nih_free (err);
314 }
315
316 /* Required to exist before /dev/pts accessed */
317 if (mknod ("/dev/ptmx", (S_IFCHR | 0666), makedev (5, 2)) < 0 && errno != EEXIST)
318 nih_error ("Unable to create /dev/ptmx");
319
320 if (mkdir ("/dev/pts", 0755) < 0 && errno != EEXIST)
321 nih_error ("%s: %s", _("Cannot create directory"), "/dev/pts");
322 }
323
324 if (system_mount ("devpts", "/dev/pts", (MS_NOEXEC | MS_NOSUID)) < 0) {
325 NihError *err;
326
327 err = nih_error_get ();
328 nih_error ("%s: %s", _("Unable to mount /dev/pts filesystem"),
329 err->message);
330 nih_free (err);
331 }
293 } else {332 } else {
294 nih_log_set_priority (NIH_LOG_DEBUG);333 nih_log_set_priority (NIH_LOG_DEBUG);
295 nih_debug ("Running with UID %d as PID %d (PPID %d)",334 nih_debug ("Running with UID %d as PID %d (PPID %d)",
296335
=== modified file 'init/system.c'
--- init/system.c 2011-12-09 14:07:11 +0000
+++ init/system.c 2012-08-03 23:21:19 +0000
@@ -164,9 +164,10 @@
164/**164/**
165 * system_mount:165 * system_mount:
166 * @type: filesystem type,166 * @type: filesystem type,
167 * @dir: mountpoint.167 * @dir: mountpoint,
168 * @flags: mount flags.
168 *169 *
169 * Mount the kernel filesystem @type at @dir, if not already mounted. This170 * Mount the kernel filesystem @type at @dir with @flags, if not already mounted. This
170 * is used to ensure that the proc and sysfs filesystems are always171 * is used to ensure that the proc and sysfs filesystems are always
171 * available.172 * available.
172 *173 *
@@ -177,7 +178,8 @@
177 **/178 **/
178int179int
179system_mount (const char *type,180system_mount (const char *type,
180 const char *dir)181 const char *dir,
182 unsigned long flags)
181{183{
182 nih_local char *parent = NULL;184 nih_local char *parent = NULL;
183 char * ptr;185 char * ptr;
@@ -206,8 +208,7 @@
206 return 0;208 return 0;
207209
208 /* Mount the filesystem */210 /* Mount the filesystem */
209 if (mount ("none", dir, type,211 if (mount ("none", dir, type, flags, NULL) < 0)
210 MS_NODEV | MS_NOEXEC | MS_NOSUID, NULL) < 0)
211 nih_return_system_error (-1);212 nih_return_system_error (-1);
212213
213 return 0;214 return 0;
214215
=== modified file 'init/system.h'
--- init/system.h 2011-05-12 20:42:28 +0000
+++ init/system.h 2012-08-03 23:21:19 +0000
@@ -35,7 +35,8 @@
35int system_setup_console (ConsoleType type, int reset)35int system_setup_console (ConsoleType type, int reset)
36 __attribute__ ((warn_unused_result));36 __attribute__ ((warn_unused_result));
3737
38int system_mount (const char *type, const char *dir)38int system_mount (const char *type, const char *dir,
39 unsigned long flags)
39 __attribute__ ((warn_unused_result));40 __attribute__ ((warn_unused_result));
4041
41NIH_END_EXTERN42NIH_END_EXTERN

Subscribers

People subscribed via source and target branches