From 67223ee205364afb203361b134f16b890c4d726c Mon Sep 17 00:00:00 2001 From: aszlig Date: Fri, 6 May 2016 12:36:58 +0200 Subject: [PATCH 1/5] nixos/stage-1: Don't kill kernel threads Unfortunately, pkill doesn't distinguish between kernel and user space processes, so we need to make sure we don't accidentally kill kernel threads. Normally, a kernel thread ignores all signals, but there are a few that do. A quick grep on the kernel source tree (as of kernel 4.6.0) shows the following source files which use allow_signal(): drivers/isdn/mISDN/l1oip_core.c drivers/md/md.c drivers/misc/mic/cosm/cosm_scif_server.c drivers/misc/mic/cosm_client/cosm_scif_client.c drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c drivers/staging/rtl8188eu/core/rtw_cmd.c drivers/staging/rtl8712/rtl8712_cmd.c drivers/target/iscsi/iscsi_target.c drivers/target/iscsi/iscsi_target_login.c drivers/target/iscsi/iscsi_target_nego.c drivers/usb/atm/usbatm.c drivers/usb/gadget/function/f_mass_storage.c fs/jffs2/background.c fs/lockd/clntlock.c fs/lockd/svc.c fs/nfs/nfs4state.c fs/nfsd/nfssvc.c While not all of these are necessarily kthreads and some functionality may still be unimpeded, it's still quite harmful and can cause unexpected side-effects, especially because some of these kthreads are storage-related (which we obviously don't want to kill during bootup). During discussion at #15226, @dezgeg suggested the following implementation: for pid in $(pgrep -v -f '@'); do if [ "$(cat /proc/$pid/cmdline)" != "" ]; then kill -9 "$pid" fi done This has a few downsides: * User space processes which use an empty string in their command line won't be killed. * It results in errors during bootup because some shell-related processes are already terminated (maybe it's pgrep itself, haven't checked). * The @ is searched within the full command line, not just at the beginning of the string. Of course, we already had this until now, so it's not a problem of his implementation. I posted an alternative implementation which doesn't suffer from the first point, but even that one wasn't sufficient: for pid in $(pgrep -v -f '^@'); do readlink "/proc/$pid/exe" &> /dev/null || continue echo "$pid" done | xargs kill -9 This one spawns a subshell, which would be included in the processes to kill and actually kills itself during the process. So what we have now is even checking whether the shell process itself is in the list to kill and avoids killing it just to be sure. Also, we don't spawn a subshell anymore and use /proc/$pid/exe to distinguish between user space and kernel processes like in the comments of the following StackOverflow answer: http://stackoverflow.com/a/12231039 We don't need to take care of terminating processes, because what we actually want IS to terminate the processes. The only point where this (and any previous) approach falls short if we have processes that act like fork bombs, because they might spawn additional processes between the pgrep and the killing. We can only address this with process/control groups and this still won't save us because the root user can escape from that as well. Signed-off-by: aszlig Fixes: #15226 --- nixos/modules/system/boot/stage-1-init.sh | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/nixos/modules/system/boot/stage-1-init.sh b/nixos/modules/system/boot/stage-1-init.sh index 1f8779abf0c..9bffcd31b9b 100644 --- a/nixos/modules/system/boot/stage-1-init.sh +++ b/nixos/modules/system/boot/stage-1-init.sh @@ -439,8 +439,18 @@ eval "exec $logOutFd>&- $logErrFd>&-" # Kill any remaining processes, just to be sure we're not taking any # with us into stage 2. But keep storage daemons like unionfs-fuse. -pkill -9 -v -f '@' - +# +# Storage daemons are distinguished by an @ in front of their command line: +# https://www.freedesktop.org/wiki/Software/systemd/RootStorageDaemons/ +local pidsToKill="$(pgrep -v -f '^@')" +for pid in $pidsToKill; do + # Make sure we don't kill kernel processes, see #15226 and: + # http://stackoverflow.com/questions/12213445/identifying-kernel-threads + readlink "/proc/$pid/exe" &> /dev/null || continue + # Try to avoid killing ourselves. + [ $pid -eq $$ ] && continue + kill -9 "$pid" +done if test -n "$debug1mounts"; then fail; fi From dc6d0030110d0ebd7f13a0fdaccfe494fbad0e29 Mon Sep 17 00:00:00 2001 From: aszlig Date: Fri, 6 May 2016 12:12:35 +0200 Subject: [PATCH 2/5] nixos/tests/installer/swraid: Check for safemode This is a regression test for #15226, so that the test will fail once we accidentally kill one or more of the md kthreads (aka: if safe mode is enabled). Signed-off-by: aszlig --- nixos/tests/installer.nix | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/nixos/tests/installer.nix b/nixos/tests/installer.nix index c9aa9f9c85d..0b0e53ee732 100644 --- a/nixos/tests/installer.nix +++ b/nixos/tests/installer.nix @@ -407,6 +407,10 @@ in { "mdadm --verbose -W /dev/md1", ); ''; + preBootCommands = '' + $machine->start; + $machine->fail("dmesg | grep 'immediate safe mode'"); + ''; }; # Test a basic install using GRUB 1. From 4f796c28d57887cc9812190bc99fb45b2acd6d1c Mon Sep 17 00:00:00 2001 From: aszlig Date: Fri, 6 May 2016 16:06:22 +0200 Subject: [PATCH 3/5] nixos/tests: Add a test for boot stage 1 We already have a small regression test for #15226 within the swraid installer test. Unfortunately, we only check there whether the md kthread got signalled but not whether other rampaging processes are still alive that *should* have been killed. So in order to do this we provide multiple canary processes which are checked after the system has booted up: * canary1: It's a simple forking daemon which just sleeps until it's going to be killed. Of course we expect this process to not be alive anymore after boot up. * canary2: Similar to canary1, but tries to mimick a kthread to make sure that it's going to be properly killed at the end of stage 1. * canary3: Like canary2, but this time using a @ in front of its command name to actually prevent it from being killed. * kcanary: This one is a real kthread and it runs until killed, which shouldn't be the case. Tested with and without 67223ee and everything works as expected, at least on my machine. Signed-off-by: aszlig --- nixos/release.nix | 1 + nixos/tests/boot-stage1.nix | 153 ++++++++++++++++++++++++++++++++++++ 2 files changed, 154 insertions(+) create mode 100644 nixos/tests/boot-stage1.nix diff --git a/nixos/release.nix b/nixos/release.nix index 97f6df16dc9..c8547784bbc 100644 --- a/nixos/release.nix +++ b/nixos/release.nix @@ -209,6 +209,7 @@ in rec { tests.bittorrent = callTest tests/bittorrent.nix {}; tests.blivet = callTest tests/blivet.nix {}; tests.boot = callSubTests tests/boot.nix {}; + tests.boot-stage1 = callTest tests/boot-stage1.nix {}; tests.cadvisor = hydraJob (import tests/cadvisor.nix { system = "x86_64-linux"; }); tests.chromium = (callSubTests tests/chromium.nix { system = "x86_64-linux"; }).stable; tests.cjdns = callTest tests/cjdns.nix {}; diff --git a/nixos/tests/boot-stage1.nix b/nixos/tests/boot-stage1.nix new file mode 100644 index 00000000000..311acd7bb1c --- /dev/null +++ b/nixos/tests/boot-stage1.nix @@ -0,0 +1,153 @@ +import ./make-test.nix { + name = "boot-stage1"; + + machine = { config, pkgs, lib, ... }: { + boot.extraModulePackages = let + compileKernelModule = name: source: pkgs.runCommand name rec { + inherit source; + kdev = config.boot.kernelPackages.kernel.dev; + kver = config.boot.kernelPackages.kernel.modDirVersion; + ksrc = "${kdev}/lib/modules/${kver}/build"; + } '' + echo "obj-m += $name.o" > Makefile + echo "$source" > "$name.c" + make -C "$ksrc" M=$(pwd) modules + install -vD "$name.ko" "$out/lib/modules/$kver/$name.ko" + ''; + + # This spawns a kthread which just waits until it gets a signal and + # terminates if that is the case. We want to make sure that nothing during + # the boot process kills any kthread by accident, like what happened in + # issue #15226. + kcanary = compileKernelModule "kcanary" '' + #include + #include + #include + #include + #include + + struct task_struct *canaryTask; + + static int kcanary(void *nothing) + { + allow_signal(SIGINT); + allow_signal(SIGTERM); + allow_signal(SIGKILL); + while (!kthread_should_stop()) { + set_current_state(TASK_INTERRUPTIBLE); + schedule_timeout_interruptible(msecs_to_jiffies(100)); + if (signal_pending(current)) break; + } + return 0; + } + + static int kcanaryInit(void) + { + kthread_run(&kcanary, NULL, "kcanary"); + return 0; + } + + static void kcanaryExit(void) + { + kthread_stop(canaryTask); + } + + module_init(kcanaryInit); + module_exit(kcanaryExit); + ''; + + in lib.singleton kcanary; + + boot.initrd.kernelModules = [ "kcanary" ]; + + boot.initrd.extraUtilsCommands = let + compile = name: source: pkgs.runCommand name { inherit source; } '' + mkdir -p "$out/bin" + echo "$source" | gcc -Wall -o "$out/bin/$name" -xc - + ''; + + daemonize = name: source: compile name '' + #include + #include + + void runSource(void) { + ${source} + } + + int main(void) { + if (fork() > 0) return 0; + setsid(); + runSource(); + return 1; + } + ''; + + mkCmdlineCanary = { name, cmdline ? "", source ? "" }: (daemonize name '' + char *argv[] = {"${cmdline}", NULL}; + execvp("${name}-child", argv); + '') // { + child = compile "${name}-child" '' + #include + #include + + int main(void) { + ${source} + while (1) sleep(1); + return 1; + } + ''; + }; + + copyCanaries = with lib; concatMapStrings (canary: '' + ${optionalString (canary ? child) '' + copy_bin_and_libs "${canary.child}/bin/${canary.child.name}" + ''} + copy_bin_and_libs "${canary}/bin/${canary.name}" + ''); + + in copyCanaries [ + # Simple canary process which just sleeps forever and should be killed by + # stage 2. + (daemonize "canary1" "while (1) sleep(1);") + + # We want this canary process to try mimicking a kthread using a cmdline + # with a zero length so we can make sure that the process is properly + # killed in stage 1. + (mkCmdlineCanary { + name = "canary2"; + source = '' + FILE *f; + f = fopen("/run/canary2.pid", "w"); + fprintf(f, "%d\n", getpid()); + fclose(f); + ''; + }) + + # This canary process mimicks a storage daemon, which we do NOT want to be + # killed before going into stage 2. For more on root storage daemons, see: + # https://www.freedesktop.org/wiki/Software/systemd/RootStorageDaemons/ + (mkCmdlineCanary { + name = "canary3"; + cmdline = "@canary3"; + }) + ]; + + boot.initrd.postMountCommands = '' + canary1 + canary2 + canary3 + # Make sure the pidfile of canary 2 is created so that we still can get + # its former pid after the killing spree starts next within stage 1. + while [ ! -s /run/canary2.pid ]; do sleep 0.1; done + ''; + }; + + testScript = '' + $machine->waitForUnit("multi-user.target"); + $machine->succeed('test -s /run/canary2.pid'); + $machine->fail('pgrep -a canary1'); + $machine->fail('kill -0 $(< /run/canary2.pid)'); + $machine->succeed('pgrep -a -f \'^@canary3$\'''); + $machine->succeed('pgrep -a -f \'^kcanary$\'''); + ''; +} From eb6e366446ea00d92e2a39d2700dc4faa0a6bdf0 Mon Sep 17 00:00:00 2001 From: aszlig Date: Fri, 6 May 2016 16:56:54 +0200 Subject: [PATCH 4/5] nixos/release-combined: Add boot-stage1 test We don't want to push out a channel update whenever this test fails, because that might have unexpected and confused side effects and it *really* means that stage 1 of our boot up is broken. Signed-off-by: aszlig --- nixos/release-combined.nix | 1 + 1 file changed, 1 insertion(+) diff --git a/nixos/release-combined.nix b/nixos/release-combined.nix index acf9ab143da..c8c4df5c913 100644 --- a/nixos/release-combined.nix +++ b/nixos/release-combined.nix @@ -67,6 +67,7 @@ in rec { (all nixos.tests.boot.biosUsb) (all nixos.tests.boot.uefiCdrom) (all nixos.tests.boot.uefiUsb) + (all nixos.tests.boot-stage1) (all nixos.tests.ipv6) (all nixos.tests.kde4) #(all nixos.tests.lightdm) From 64ca91cac9b5dd520a736528a3f0a29ba1480593 Mon Sep 17 00:00:00 2001 From: aszlig Date: Fri, 6 May 2016 21:32:21 +0200 Subject: [PATCH 5/5] nixos/tests/boot-stage1: Add myself to maintainers As @edolstra pointed out that the kernel module might be painful to maintain. I strongly disagree because it's only a small module and it's good to have such a canary in the tests no matter how the bootup process looks like, so I'm going the masochistic route and try to maintain it. If it *really* becomes too much maintenance burden, we can still drop or disable kcanary. Signed-off-by: aszlig --- nixos/tests/boot-stage1.nix | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/nixos/tests/boot-stage1.nix b/nixos/tests/boot-stage1.nix index 311acd7bb1c..ad253d23c54 100644 --- a/nixos/tests/boot-stage1.nix +++ b/nixos/tests/boot-stage1.nix @@ -1,4 +1,4 @@ -import ./make-test.nix { +import ./make-test.nix ({ pkgs, ... }: { name = "boot-stage1"; machine = { config, pkgs, lib, ... }: { @@ -150,4 +150,6 @@ import ./make-test.nix { $machine->succeed('pgrep -a -f \'^@canary3$\'''); $machine->succeed('pgrep -a -f \'^kcanary$\'''); ''; -} + + meta.maintainers = with pkgs.stdenv.lib.maintainers; [ aszlig ]; +})