Merge lp:~stgraber/upstart/upstart-prctl into lp:upstart

[Stéphane Graber]

Add a call to prctl to set the init process as the subreaper.

This basically prevents any child process to double fork and get re-parented
to PID 1. Any process attempting a double fork will simply be re-parented to
the closest init process in the process tree.

This feature allows Session Inits to track the state of their children and
receive SIGCHLD for them when they die.

The PR_SET_CHILD_SUBREAPER flag was added in the 3.4 kernel.
In the event where the kernel doesn't support it, upstart will emit a prctl-subreaper-failed event as well as log a warning message.

On such system, tracking child process status won't be reliable and once we have the user jobs implemented, the respawn statement for those should be ignored when we can't setuup the subreaper as to avoid tracking problems, potentially leading to loops.

[James Hunt (jamesodhunt)]

- Could you change:

    "Unable to mark us as a subreaper"

  ... to:

    "Unable to register as subreaper"

- I'd prefer the event to be called something like 'child-subreaper-failed', to allow for the fact
  that other systems may implement it in ways other than prctl. And it's shorter :)

- There is an extraneous C++ comment that can be removed:

    // Emit prctl-subreaper-failed event

[Stéphane Graber (stgraber)]

All done.

[lp:~stgraber/upstart/upstart-prctl]

1392. By James Hunt on 2012-11-30

Merge of lp:~stgraber/upstart/upstart-initctl2dot-python3.

1393. By James Hunt on 2012-12-03

* Merge of lp:~xnox/upstart/fix-qemu-test.

1394. By James Hunt on 2012-12-03

* re-merge of lp:~xnox/upstart/fix-qemu-test to fix scenario where
  'filename' was not set causing unlink to fail.

1395. By James Hunt on 2012-12-04

* init/tests/test_job_process.c: test_run(): Set process->script
  explicitly for multiple tests to avoid undefined behaviour where a
  shell may or may not be used (LP: #1086474).

1396. By James Hunt on 2012-12-05

* Merge of lp:~stgraber/upstart/upstart-initgroups.

1397. By James Hunt on 2012-12-06

* init/tests/test_job_process.c: Sync with lp:ubuntu/upstart branch
  as unintended divergence meant lp:upstart was missing TEST_SIGNALS.
  This results in two new tests:
  - "ensure sane signal state with no console"
  - "ensure sane signal state with log console"

1398. By Steve Langasek on 2012-12-06

Merge fix for bug #1079715

1399. By Steve Langasek on 2012-12-06

Merge fix for bug #1083723

1400. By James Hunt on 2012-12-07

* init/Makefile.am:
  - TEST_DATA_DIR: use $srcdir, not $PWD.
  - TEST_DATA_FILES: Corrected filename.

[James Hunt (jamesodhunt)]

Hi Stéphane,

My only worry is that if a user attempts to build this on a system whose libc is too old, PR_SET_CHILD_SUBREAPER won't be defined.

I think we need some autoconf magic so we can "#ifdef HAVE_CHILD_SUBREAPER".

If we absolutely require SUBREAPER (added to kernel in v3.4), we should update the top-level README for libc+kernel versions too.

[lp:~stgraber/upstart/upstart-prctl]

1401. By Steve Langasek on 2012-12-07

Merge fix for 'telinit u' returning an error due to dbus disconnect

1402. By Steve Langasek on 2012-12-07

Merge 1.6.1 into the history

[Stéphane Graber (stgraber)]

So I ended up doing it in a slightly different way, based on what systemd and gnome are doing.

Basically, moving all the prctl stuff under an ifdef and having the subreaper value hardcoded in cases where it's not defined in prctl.h.

This should cover systems that for some reason lack prctl.h, systems that have prctl.h but miss the subreaper definition and systems that have older kernels where the subreaper option doesn't exist.

On systems that don't have prctl.h, the code will be entirely skipped and no event will be emitted.
On systems that have prctl.h but where the subreaper isn't defined or isn't supported by the kernel, the event will be emitted.

[lp:~stgraber/upstart/upstart-prctl]

1403. By Steve Langasek on 2012-12-07

Revert commit 1397; this was a deliberate change in rev 1387.

[Steve Langasek (vorlon)]

On Fri, Dec 07, 2012 at 09:45:24PM -0000, Stéphane Graber wrote:

> So I ended up doing it in a slightly different way, based on what systemd
> and gnome are doing.

> Basically, moving all the prctl stuff under an ifdef and having the
> subreaper value hardcoded in cases where it's not defined in prctl.h.

> This should cover systems that for some reason lack prctl.h, systems that
> have prctl.h but miss the subreaper definition and systems that have older
> kernels where the subreaper option doesn't exist.

> On systems that don't have prctl.h, the code will be entirely skipped and
> no event will be emitted.

> On systems that have prctl.h but where the subreaper isn't defined or
> isn't supported by the kernel, the event will be emitted.

IMHO if we're relying on subreaper support for proper process supervision of
user jobs, in the absence of subreaper support (at runtime or build time),
we should refuse to start a user session at all.

[Steve Langasek (vorlon)]

On Fri, Dec 07, 2012 at 11:41:21AM -0000, James Hunt wrote:
> My only worry is that if a user attempts to build this on a system whose
> libc is too old, PR_SET_CHILD_SUBREAPER won't be defined.

FWIW, this definition comes from /usr/include/linux/prctl.h, which is a
header from linux-libc-dev rather than from libc. So I think only the
kernel version is relevant here.

[lp:~stgraber/upstart/upstart-prctl]

1404. By Steve Langasek on 2012-12-08

Add tests/test_user_sessions.sh to EXTRA_DIST, without which it doesn't make it
into the release tarballs.

[Stéphane Graber (stgraber)]

> On Fri, Dec 07, 2012 at 11:41:21AM -0000, James Hunt wrote:
> > My only worry is that if a user attempts to build this on a system whose
> > libc is too old, PR_SET_CHILD_SUBREAPER won't be defined.
>
> FWIW, this definition comes from /usr/include/linux/prctl.h, which is a
> header from linux-libc-dev rather than from libc. So I think only the
> kernel version is relevant here.

You're correct, I didn't look too closely for the source of the define.

Now for your other comment on relying on the prctl subreaper feature.
Yes, we'll be relying on that feature a fair bit for the user session, however after we discussed it with James and Dmitrijs, we came to the conclusion that we still need to allow starting a user session on a system that doesn't support it.

The biggest use case will be for people upgrading from 12.04's stock kernel to 14.04. On those systems, the current running kernel won't support prctl and we can't exclude that a user will attempt to open a user session before reboot.
That's why we came up with the warning+event fallback where if prctl isn't supported, we write a warning that'll ultimately end up in .xsession_errors and we emit the child-subreaper-failed event so that we can have jobs react on the missing feature and for example show a window telling the user about it and recommending a reboot.

In practice, running without prctl support will be identical to what you currently get when running upstart on the session bus as a user. Upstart can spawn jobs, can track their PID but can't know whether they stopped or need respawning. That's annoying but isn't much worse than what we have on the current gnome-session managed system, so I don't think preventing upstart from starting completely would be appropriate.

[lp:~stgraber/upstart/upstart-prctl]

1405. By James Hunt on 2012-12-11

* init/Makefile.am: Add explicit -lrt for tests (LP: #1088863)

1406. By Dimitri John Ledkov on 2012-12-12

Fix re-definition of EXTRA_DIST.

1409. By Stéphane Graber on 2012-11-29

Drop extra comment, change event from prctl-subreaper-failed to child-subreaper-failed and change wording of the warning.

1410. By Stéphane Graber on 2012-12-07

Drop sys/prctl.h from files that aren't using prctl

1411. By Stéphane Graber on 2012-12-07

Skip prctl on systems that don't have it. Also do an explicit define of PR_SET_CHILD_SUBREAPER for systems with older headers. In the case where the kernel doesn't support it, child-subreaper-failed will simply be emitted.

[Stéphane Graber (stgraber)]

Rebased the branch on current upstart trunk, builds fine and all tests pass.

[James Hunt (jamesodhunt)]

LGTM - merged.

review: Approve

=== modified file 'configure.ac'
--- configure.ac	2012-12-07 20:33:03 +0000
+++ configure.ac	2012-12-15 00:17:22 +0000
@@ -53,7 +53,7 @@
 AM_CONDITIONAL([HAVE_UDEV], [test "$have_udev" = yes])
 
 # Checks for header files.
-AC_CHECK_HEADERS([valgrind/valgrind.h])
+AC_CHECK_HEADERS([valgrind/valgrind.h, sys/prctl.h])
 
 # Checks for typedefs, structures, and compiler characteristics.
 AC_PROG_CC_C99

=== modified file 'init/log.c'
--- init/log.c	2012-11-23 11:36:47 +0000
+++ init/log.c	2012-12-15 00:17:22 +0000
@@ -31,7 +31,6 @@
 #include "session.h"
 #include "conf.h"
 #include "paths.h"
-#include <sys/prctl.h>
 
 static int  log_file_open   (Log *log);
 static int  log_file_write  (Log *log, const char *buf, size_t len);

=== modified file 'init/main.c'
--- init/main.c	2012-11-07 15:17:58 +0000
+++ init/main.c	2012-12-15 00:17:22 +0000
@@ -30,6 +30,13 @@
 #include <sys/resource.h>
 #include <sys/mount.h>
 
+#ifdef HAVE_SYS_PRCTL_H
+#include <sys/prctl.h>
+#ifndef PR_SET_CHILD_SUBREAPER
+#define PR_SET_CHILD_SUBREAPER 35
+#endif
+#endif
+
 #include <errno.h>
 #include <stdio.h>
 #include <limits.h>
@@ -603,6 +610,19 @@
 	if (disable_sessions)
 		nih_debug ("Sessions disabled");
 
+	/* Set us as the child subreaper.
+	 * This ensures that even when init doesn't run as PID 1, it'll always be
+	 * the ultimate parent of everything it spawns. */
+
+#ifdef HAVE_SYS_PRCTL_H
+	if (getpid () > 1 && prctl (PR_SET_CHILD_SUBREAPER, 1) < 0) {
+		nih_warn ("%s: %s", _("Unable to register as subreaper"),
+				  strerror (errno));
+
+		NIH_MUST (event_new (NULL, "child-subreaper-failed", NULL));
+	}
+#endif
+
 	/* Run through the loop at least once to deal with signals that were
 	 * delivered to the previous process while the mask was set or to
 	 * process the startup event we emitted.

=== modified file 'init/tests/test_log.c'
--- init/tests/test_log.c	2012-09-20 08:12:05 +0000
+++ init/tests/test_log.c	2012-12-15 00:17:22 +0000
@@ -26,7 +26,6 @@
 #include <libgen.h>
 #include <sys/types.h>
 #include <sys/ioctl.h>
-#include <sys/prctl.h>
 #include <nih/test.h>
 #include <nih/timer.h>
 #include <nih/child.h>

=== modified file 'init/tests/test_state.c'
--- init/tests/test_state.c	2012-12-04 16:09:18 +0000
+++ init/tests/test_state.c	2012-12-15 00:17:22 +0000
@@ -28,7 +28,6 @@
 #include <libgen.h>
 #include <sys/types.h>
 #include <sys/ioctl.h>
-#include <sys/prctl.h>
 #include <nih/test.h>
 #include <nih/timer.h>
 #include <nih/child.h>