Supporting chroot builds on GNU/Hurd

Ludovic Courtès wrote on 7 Oct 2020 23:57

Recipients:(address . guix-patches@gnu.org)

Message-ID:87sgapsnqz.fsf@gnu.org

Hello!

The patch below is an attempt at supporting “chroot builds” on GNU/Hurd;

it’s “almost working”. The main feature we need is firmlinks (or “bind

mounts”) and commenting out Linux-specific things (private bind mounts,

‘pivot_root’, etc.).

The patch introduces a ‘firmlink’ function that creates a bind mount on

GNU/Linux and otherwise spawns the /hurd/firmlink translator. It also

adjusts ‘deletePath’ to remove any active translators from files it’s

about to delete, the goal being to terminate those firmlink translators.

(Apparently that code isn’t reached yet though.)

The set of /dev nodes firmlinked into the chroot is reduced compared to

GNU/Linux; I added /servers, meaning that /servers is firmlinked as is,

which is not ideal (see below).

With this patch, I can run “guix build hello --check” in a chroot… but

it eventually hangs somewhere in ‘DerivationGoal::buildDone’ (I presume)

once the build has completed. It leaves behind it all its firmlink

processes:

Toggle snippet (18 lines)root@childhurd ~# ps --width=200 aux|grep firmlink
root     16426 120.0 0.0  147M  616K p1 RNm  11:38PM  0:00.00 grep --color=auto firmlink
root     11191  0.0  0.0  147M  888K  - S<o  11:27PM  0:00.00 /hurd/firmlink /dev/full
root     11192  0.0  0.0  147M  936K  - S<o  11:27PM  0:00.01 /hurd/firmlink /dev/null
root     11193  0.0  0.0  147M  888K  - S<o  11:27PM  0:00.00 /hurd/firmlink /dev/random
root     11194  0.0  0.0  147M  936K  - S<o  11:27PM  0:00.00 /hurd/firmlink /dev/urandom
root     11195  0.0  0.0  147M  888K  - S<o  11:27PM  0:00.00 /hurd/firmlink /dev/zero
root     11196  0.0  0.0  147M  936K  - S<o  11:27PM  0:00.02 /hurd/firmlink /gnu/store/1brrgqhgjni8g8dbp97s8c7d7nq0p9a0-libgc-8.0.4
root     11197  0.0  0.0  147M  936K  - S<o  11:27PM  0:00.05 /hurd/firmlink /gnu/store/1gmsfg4cfmj3l54s3nkn3lyas3nnzjgs-hurd-core-headers-0.9-1.91a5167
root     11198  0.0  0.0  147M  936K  - S<o  11:27PM  0:00.00 /hurd/firmlink /gnu/store/5ndcmqx0pzksq5c4mznbv20xhfnk3p97-zlib-1.2.11
root     11199  0.0  0.0  147M  936K  - S<o  11:27PM  0:00.02 /hurd/firmlink /gnu/store/631ixr5flpk66ziigbmkxwafz8skpyzs-findutils-4.7.0
root     11200  0.0  0.0  147M  936K  - S<o  11:27PM  0:00.59 /hurd/firmlink /gnu/store/6zfpncmmz1aqpsg1ba7zjna8qb4nmwdx-file-5.38
[...]
root     11233  0.0  0.0  147M  936K  - S<o  11:27PM  0:00.58 /hurd/firmlink /gnu/store/zjlh3sljylr3y1cavxp2z7g37g922mbb-gcc-7.5.0
root     11234  0.0  0.0  147M  936K  - S<o  11:27PM  0:00.02 /hurd/firmlink /servers
root     11235  0.0  0.0  147M  936K  - S<o  11:27PM  0:00.07 /hurd/firmlink /tmp/guix-build-hello-2.10.drv-6

This has yet to be debugged. :-)

Apart from that, this raises the question of what to put in the build

environment. As written in the blog post about childhurds that should

go out tomorrow, on GNU/Linux, we do not include any piece of userland

software in the environment. But here, we’d be doing just that: running

Hurd translators that are not specified as derivation inputs. It’s OK

for /dev/null, but maybe questionable for /servers/socket/*.

Regarding /servers, we’ll probably want to spawn separate servers (a

separate /servers/proc would give up a new “PID namespace”.) Should

that be done magically by the daemon, or should we leave it up to build

processes, so that build processes really specify all the user-land

software they depend on? We could imagine a new phase in

‘gnu-build-system’, on GNU/Hurd, that would start by setting up a bunch

of translators. Food for thought… As a first step, we can firmlink it

from the host, with an eye towards “doing it right”.

I felt a need to hack on this as I was investigating an util-linux test

failure in a ‘--disable-chroot’ setup: the test would find /proc and

would later fail for some reason. Had /proc been missing from the build

environment (as is the case with this patch), the test would have been

skipped (it explicitly handles that case). I think we’d rather not

fiddle too much with test suites until we have defined what’s available

in the default build environment.

Thoughts?

For the record, I’ve been testing this by:

1. Adding ‘hurd’ as an input to the ‘guix’ package.

2. Cross-building the daemon using code from my branch:

~/src/guix/pre-inst-env guix build --with-git-url=guix-daemon=$PWD --with-branch=guix-daemon=wip-hurd-chroot --target=i586-pc-gnu -e '(@ (gnu packages package-management) guix-daemon)'

3. Sending it over to the childhurd:

guix copy --to=localhost:10022 /gnu/store/…

4. Running it in the childhurd with:

GUIX=/run/current-system/profile/bin/guix /gnu/store/…-guix-daemon-git.wip-hurd-chroot/bin/guix-daemon --build-users-group=guixbuild --disable-deduplication

I guess I should probably go ahead and do everything natively in the

childhurd, but I thought I’d share my weird workflow in case that gives

me a chance to participate in some weirdness contest.

Ludo’.

From 1887d0dee0031df0de117b3a6339495504b4b489 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= <ludo@gnu.org>
Date: Tue, 6 Oct 2020 23:53:24 +0200
Subject: [PATCH] DRAFT daemon: Support chroot builds on GNU/Hurd.

* nix/libutil/util.cc (firmlink): New functions.
(_deletePath) [__GNU__]: Check whether a translator is set on PATH.
Call 'fsys_goaway' if this is the case.
* nix/libutil/util.hh (firmlink): New declaration.
* nix/libstore/build.cc (CHROOT_ENABLED): Define to 1.  Error out when
both __GNU__ and __linux__ are undefined.
(DerivationGoal::runChild): Remove special treatment of /proc.  Use
'firmlink' instead of 'mount' with MS_BIND.
Wrap /proc, /dev/shm, /dev/pts, and /proc/self handling in #ifdef
__linux__.  Same for 'pivot_root' call.
* config-daemon.ac: Set and substitute 'HURD_LIBS'.
* nix/local.mk (guix_daemon_LDADD): Add $(HURD_LIBS).
---
 config-daemon.ac      |  13 ++++
 nix/libstore/build.cc |  43 +++++++++----
 nix/libutil/util.cc   | 139 ++++++++++++++++++++++++++++++++++++++++++
 nix/libutil/util.hh   |   3 +
 nix/local.mk          |   3 +-
 5 files changed, 189 insertions(+), 12 deletions(-)

Toggle diff (337 lines)diff --git a/config-daemon.ac b/config-daemon.ac
index 50ead355a8..bdaee82fb8 100644
--- a/config-daemon.ac
+++ b/config-daemon.ac
@@ -38,6 +38,19 @@ if test "x$guix_build_daemon" = "xyes"; then
   AC_DEFINE_UNQUOTED([SYSTEM], ["$guix_system"],
     [Guix host system type--i.e., platform and OS kernel tuple.])
 
+  dnl On GNU/Hurd guix-daemon depends on libfshelp.
+  case "$guix_system" in
+    *-gnu)
+      AC_CHECK_LIB([fshelp], [fshelp_start_translator])
+      if test "x$ac_cv_lib_fshelp_fshelp_start_translator" != "xyes"; then
+        AC_MSG_ERROR([libfshelp (GNU Hurd) could not be found])
+      fi
+      HURD_LIBS="-lfshelp";;
+    *)
+      HURD_LIBS="";;
+  esac
+  AC_SUBST([HURD_LIBS])
+
   case "$LIBGCRYPT_PREFIX" in
     no)
       LIBGCRYPT_CFLAGS=""
diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index ccec513d8d..7151932403 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -52,7 +52,13 @@
 #endif
 
 
-#define CHROOT_ENABLED HAVE_CHROOT && HAVE_SYS_MOUNT_H && defined(MS_BIND) && defined(MS_PRIVATE)
+/* Chroot builds are supported both on GNU/Linux and on GNU/Hurd.  */
+#if defined __linux__ || defined __GNU__
+# define CHROOT_ENABLED 1
+#else
+# error unsupported operating system
+#endif
+
 #define CLONE_ENABLED defined(CLONE_NEWNS)
 
 #if defined(SYS_pivot_root)
@@ -1991,6 +1997,7 @@ void DerivationGoal::runChild()
             if (setdomainname(domainname, sizeof(domainname)) == -1)
                 throw SysError("cannot set domain name");
 
+#ifdef __linux__
             /* Make all filesystems private.  This is necessary
                because subtrees may have been mounted as "shared"
                (MS_SHARED).  (Systemd does this, for instance.)  Even
@@ -2007,27 +2014,30 @@ void DerivationGoal::runChild()
                different filesystem from /, as needed for pivot_root. */
             if (mount(chrootRootDir.c_str(), chrootRootDir.c_str(), 0, MS_BIND, 0) == -1)
                 throw SysError(format("unable to bind mount ‘%1%’") % chrootRootDir);
+#endif
 
             /* Set up a nearly empty /dev, unless the user asked to
                bind-mount the host /dev. */
             Strings ss;
             if (dirsInChroot.find("/dev") == dirsInChroot.end()) {
+#ifdef __linux__
                 createDirs(chrootRootDir + "/dev/shm");
                 createDirs(chrootRootDir + "/dev/pts");
-                ss.push_back("/dev/full");
-#ifdef __linux__
+                createSymlink("/proc/self/fd", chrootRootDir + "/dev/fd");
+                createSymlink("/proc/self/fd/0", chrootRootDir + "/dev/stdin");
+                createSymlink("/proc/self/fd/1", chrootRootDir + "/dev/stdout");
+                createSymlink("/proc/self/fd/2", chrootRootDir + "/dev/stderr");
+                ss.push_back("/dev/tty");
                 if (pathExists("/dev/kvm"))
                     ss.push_back("/dev/kvm");
+#elif __GNU__
+		ss.push_back("/servers");
 #endif
+                ss.push_back("/dev/full");
                 ss.push_back("/dev/null");
                 ss.push_back("/dev/random");
-                ss.push_back("/dev/tty");
                 ss.push_back("/dev/urandom");
                 ss.push_back("/dev/zero");
-                createSymlink("/proc/self/fd", chrootRootDir + "/dev/fd");
-                createSymlink("/proc/self/fd/0", chrootRootDir + "/dev/stdin");
-                createSymlink("/proc/self/fd/1", chrootRootDir + "/dev/stdout");
-                createSymlink("/proc/self/fd/2", chrootRootDir + "/dev/stderr");
             }
 
             /* Fixed-output derivations typically need to access the
@@ -2049,7 +2059,7 @@ void DerivationGoal::runChild()
                 struct stat st;
                 Path source = i->second;
                 Path target = chrootRootDir + i->first;
-                if (source == "/proc") continue; // backwards compatibility
+
                 debug(format("bind mounting `%1%' to `%2%'") % source % target);
                 if (stat(source.c_str(), &st) == -1)
                     throw SysError(format("getting attributes of path `%1%'") % source);
@@ -2059,10 +2069,11 @@ void DerivationGoal::runChild()
                     createDirs(dirOf(target));
                     writeFile(target, "");
                 }
-                if (mount(source.c_str(), target.c_str(), "", MS_BIND, 0) == -1)
+                if (firmlink(source, target) == -1)
                     throw SysError(format("bind mount from `%1%' to `%2%' failed") % source % target);
             }
 
+#ifdef __linux__
             /* Bind a new instance of procfs on /proc to reflect our
                private PID namespace. */
             createDirs(chrootRootDir + "/proc");
@@ -2090,11 +2101,16 @@ void DerivationGoal::runChild()
                    Linux versions, it is created with permissions 0.  */
                 chmod_(chrootRootDir + "/dev/pts/ptmx", 0666);
             }
+#elif __GNU__
+	    /* Do not mount things that are implemented in user land: /proc,
+	       /dev/shm, /dev/pts, etc.  */
+#endif
 
             /* Do the chroot(). */
             if (chdir(chrootRootDir.c_str()) == -1)
                 throw SysError(format("cannot change directory to '%1%'") % chrootRootDir);
 
+#ifdef __linux__
             if (mkdir("real-root", 0) == -1)
                 throw SysError("cannot create real-root directory");
 
@@ -2109,8 +2125,13 @@ void DerivationGoal::runChild()
 
             if (rmdir("real-root") == -1)
                 throw SysError("cannot remove real-root directory");
-        }
+#elif __GNU__
+            if (chroot(".") == -1)
+                throw SysError(format("cannot change root directory to '%1%'") % chrootRootDir);
+
 #endif
+        }
+#endif	// CHROOT_ENABLED
 
         if (chdir(tmpDirInSandbox.c_str()) == -1)
             throw SysError(format("changing into `%1%'") % tmpDir);
diff --git a/nix/libutil/util.cc b/nix/libutil/util.cc
index 59a2981359..b49a17a6eb 100644
--- a/nix/libutil/util.cc
+++ b/nix/libutil/util.cc
@@ -23,6 +23,41 @@
 #include <sys/prctl.h>
 #endif
 
+#if HAVE_SYS_MOUNT_H
+#include <sys/mount.h>
+#endif
+
+#ifdef __GNU__
+
+extern "C" {
+
+/* XXX: <idvec.h> uses 'new' as a parameter name.  Work around it.  */
+# define new new_param
+
+# include <hurd.h>
+# include <hurd/paths.h>
+# include <hurd/fsys.h>
+# include <argz.h>
+
+# undef new
+
+/* XXX: <fshelp.h> is not C++-compatible.  Copy these declarations to work
+   around it.  */
+
+typedef error_t (*fshelp_open_fn_t) (int flags,
+				     file_t *node,
+				     mach_msg_type_name_t *node_type,
+				     task_t, void *cookie);
+
+extern error_t
+fshelp_start_translator (fshelp_open_fn_t underlying_open_fn, void *cookie,
+			 char *name, char *argz, int argz_len,
+			 int timeout, fsys_t *control);
+
+}
+
+# define _HURD_FIRMLINK  _HURD "firmlink"
+#endif
 
 extern char * * environ;
 
@@ -214,6 +249,89 @@ bool isLink(const Path & path)
     return S_ISLNK(st.st_mode);
 }
 
+#if __linux__
+
+int firmlink(const Path &source, const Path &target)
+{
+    return mount(source.c_str(), target.c_str(), "", MS_BIND, 0);
+}
+
+#elif __GNU__
+
+static error_t return_node (int flags,
+			    mach_port_t *underlying,
+			    mach_msg_type_name_t *underlying_type,
+			    task_t task, void *node)
+{
+    *underlying = * (mach_port_t *) node;
+    *underlying_type = MACH_MSG_TYPE_COPY_SEND;
+
+    return ESUCCESS;
+}
+
+int firmlink(const Path &source, const Path &target)
+{
+    static char firmlink[] = _HURD_FIRMLINK;
+    char *arg_vec[] = { firmlink, (char *) source.c_str (), NULL };
+    char *args = NULL;
+    size_t args_len;
+
+    error_t err;
+    file_t target_file = MACH_PORT_NULL;
+
+    printMsg (lvlChatty, format("creating firmlink from '%1%' to '%2%'")
+	      % source % target);
+
+    target_file = file_name_lookup (target.c_str (), O_NOTRANS, 0);
+    if (! MACH_PORT_VALID (target_file)) {
+	printMsg (lvlChatty, format("firmlink target '%s' unavailable: %s")
+		  % target % strerror (errno));
+	goto fail;
+    }
+
+    err = argz_create (arg_vec, &args, &args_len);
+    if (err != 0) goto fail;
+
+    mach_port_t control;
+    err = fshelp_start_translator (return_node, &target_file,
+				   firmlink, args, args_len,
+				   3000, &control);
+    if (err) {
+	printMsg (lvlChatty, format("failed to start '%s' translator: %s") %
+		  firmlink % strerror(errno));
+	goto fail;
+    }
+
+    free ((void *) args);
+    args = NULL;
+
+    err = (error_t) file_set_translator (target_file, 0, FS_TRANS_SET, 0,
+					 NULL, 0,
+					 control, MACH_MSG_TYPE_COPY_SEND);
+    mach_port_deallocate (mach_task_self (), control);
+    mach_port_deallocate (mach_task_self (), target_file);
+
+    if (err) {
+	printMsg (lvlChatty, format("failed to set '%s' translator on node '%s': %s") %
+		  firmlink % target % strerror(errno));
+	goto fail;
+    }
+
+    return err;
+
+fail:
+    int saved_errno = errno;
+    if (MACH_PORT_VALID (target_file))
+	mach_port_deallocate (mach_task_self (), target_file);
+    if (args != NULL)
+	free ((void *) args);
+    errno = saved_errno;
+    return -1;
+}
+
+#elif
+# error unsupported operating system
+#endif
 
 DirEntries readDirectory(const Path & path)
 {
@@ -311,6 +429,27 @@ static void _deletePath(const Path & path, unsigned long long & bytesFreed, size
 
     printMsg(lvlVomit, format("%1%") % path);
 
+#ifdef __GNU__
+    /* Check whether there's an active translator on PATH--typically
+       /hurd/firmlink.  If there is one, let it go away.  */
+    {
+	file_t file = file_name_lookup (path.c_str (), O_NOTRANS, 0);
+	if (MACH_PORT_VALID (file)) {
+	    fsys_t fsys;
+	    int err = file_get_translator_cntl (file, &fsys);
+	    mach_port_deallocate (mach_task_self (), file);
+	    if (err == 0) {
+		/* There's a translator, tell it to leave.  */
+		err = fsys_goaway (fsys, FSYS_GOAWAY_FORCE | FSYS_GOAWAY_RECURSE);
+		mach_port_deallocate (mach_task_self (), fsys);
+		if (err != 0) {
+		    throw SysError(format("removing translator from '%1%'") % path);
+		}
+	    }
+	}
+    }
+#endif
+
 #ifdef HAVE_STATX
 # define st_mode stx_mode
 # define st_size stx_size
diff --git a/nix/libutil/util.hh b/nix/libutil/util.hh
index 13cff44316..353c758895 100644
--- a/nix/libutil/util.hh
+++ b/nix/libutil/util.hh
@@ -62,6 +62,9 @@ Path readLink(const Path & path);
 
 bool isLink(const Path & path);
 
+/* Make TARGET a firmlink (aka. "bind mount") to SOURCE.  */
+int firmlink(const Path & source, const Path & target);
+
 /* Read the contents of a directory.  The entries `.' and `..' are
    removed. */
 struct DirEntry
diff --git a/nix/local.mk b/nix/local.mk
index 2bb01041b9..782e2c85cc 100644
--- a/nix/local.mk
+++ b/nix/local.mk
@@ -124,7 +124,8 @@ guix_daemon_CPPFLAGS =				\
 
 guix_daemon_LDADD =				\
   libstore.a libutil.a libformat.a -lz		\
-  $(SQLITE3_LIBS) $(LIBGCRYPT_LIBS)
+  $(SQLITE3_LIBS) $(LIBGCRYPT_LIBS)		\
+  $(HURD_LIBS)
 
 guix_daemon_headers =				\
   %D%/nix-daemon/shared.hh
-- 
2.28.0

Jan Nieuwenhuizen wrote on 11 Oct 2020 12:02

Recipients:

Message-ID:87imbhqdwk.fsf@gnu.org

Ludovic Courtès writes:

Hi!

Toggle quote (5 lines)

> The patch below is an attempt at supporting “chroot builds” on GNU/Hurd;

> it’s “almost working”. The main feature we need is firmlinks (or “bind

> mounts”) and commenting out Linux-specific things (private bind mounts,

> ‘pivot_root’, etc.).

Yay! I finally got round to trying this, and I can confirm that it

also "almost works" for me.

[..]

Toggle quote (5 lines)

> With this patch, I can run “guix build hello --check” in a chroot… but

> it eventually hangs somewhere in ‘DerivationGoal::buildDone’ (I presume)

> once the build has completed. It leaves behind it all its firmlink

> processes:

Yes, get something very similar.

Toggle quote (8 lines)

> I felt a need to hack on this as I was investigating an util-linux test

> failure in a ‘--disable-chroot’ setup: the test would find /proc and

> would later fail for some reason. Had /proc been missing from the build

> environment (as is the case with this patch), the test would have been

> skipped (it explicitly handles that case). I think we’d rather not

> fiddle too much with test suites until we have defined what’s available

> in the default build environment.

I also tried building util-linux and comparing it with the non-chrooted

build:

Toggle snippet (10 lines)

-checking whether make sets $(MAKE)... yes

+checking whether make sets $(MAKE)... no

- : mountpoint ... FAILED (libmount/utils-mountpoint)

+ : mountpoint ... SKIPPED (no /proc)

- 3 tests of 204 FAILED

+ 2 tests of 204 FAILED

Not sure about the configure change, probably it uses /proc to determine

that?

Still failing:

Toggle snippet (4 lines)

fdisk: invalid input tests ... FAILED (fdisk/oddinput)

ipcs: headers ... FAILED (ipcs/headers)

so, this is already better.

Toggle quote (7 lines)

> Apart from that, this raises the question of what to put in the build

> environment. As written in the blog post about childhurds that should

> go out tomorrow, on GNU/Linux, we do not include any piece of userland

> software in the environment. But here, we’d be doing just that: running

> Hurd translators that are not specified as derivation inputs. It’s OK

> for /dev/null, but maybe questionable for /servers/socket/*.

Yes, certainly maybe ;)

[..]

Toggle quote (2 lines)

> Thoughts?

What about doing it in small steps, starting with the patch you suggest

here and see how much it "hurts" to go towards more secure/more Hurd'y

chrooted builds?

Toggle quote (5 lines)

> From 1887d0dee0031df0de117b3a6339495504b4b489 Mon Sep 17 00:00:00 2001

> From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= <ludo@gnu.org>

> Date: Tue, 6 Oct 2020 23:53:24 +0200

> Subject: [PATCH] DRAFT daemon: Support chroot builds on GNU/Hurd.

So...apart from

Toggle quote (2 lines)

> This has yet to be debugged. :-)

otherwise, LGTM!

Thanks a lot for looking into this!

Greetings,

Janneke

Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org

Freelance IT http://JoyofSource.com| Avatar® http://AvatarAcademy.com

Ludovic Courtès wrote on 12 Oct 2020 11:44

Recipients:(name . Jan Nieuwenhuizen)(address . janneke@gnu.org)(address . 43857@debbugs.gnu.org)

Message-ID:87h7qzvkwn.fsf@gnu.org

Hi,

Jan Nieuwenhuizen <janneke@gnu.org> skribis:

Toggle quote (16 lines)> I also tried building util-linux and comparing it with the non-chrooted
> build:
>
> -checking whether make sets $(MAKE)... yes
> +checking whether make sets $(MAKE)... no
>
> -                : mountpoint                  ... FAILED (libmount/utils-mountpoint)
> +                : mountpoint                  ... SKIPPED (no /proc)
>
> -  3 tests of 204 FAILED
> +  2 tests of 204 FAILED
>
>
> Not sure about the configure change, probably it uses /proc to determine
> that?

Yes, the test has an explicit [ -d /proc ] and it is skipped when that’s

false.

Ludo’.

is:open	open issues
is:done	closed issues
submitter:<who>	search issue submitter
author:<who>	search by message author
date:yesterday..now	search by issue date
mdate:3m..2d	search by message date