Fix the error of runc doesn't work with go1.22 by lifubang · Pull Request #4193 · opencontainers/runc

lifubang · 2024-02-07T15:54:39Z

As the description in #4233, there is a bug in glibc, pthread_self()
will return wrong info after we do clone(CLONE_PARENT) in libct/nsenter,
it will cause runc can't work in go 1.22.*. So we use fork(2) to replace
clone(2) in libct/nsenter, but there is a double-fork in nsenter, so we
need to use PR_SET_CHILD_SUBREAPER to let runc can reap grand child
process in libct/nsenter.

Fix #4233

lifubang · 2024-02-07T16:21:40Z

go 1.22.0 error msg:

DEBU[0000]libcontainer/dmz/cloned_binary_linux.go:202 libcontainer/dmz.IsCloned() F_GET_SEALS on /proc/self/exe failed: invalid argument 
DEBU[0000]libcontainer/dmz/cloned_binary_linux.go:177 libcontainer/dmz.CloneBinary() cloning runc-dmz binary (8736 bytes)         
DEBU[0000]libcontainer/container_linux.go:537 libcontainer.(*Container).newParentProcess() runc-dmz: using runc-dmz                     
DEBU[0000] nsexec[42599]: => nsexec container setup     
DEBU[0000] nsexec-0[42599]: ~> nsexec stage-0           
DEBU[0000] nsexec-0[42599]: spawn stage-1               
DEBU[0000] nsexec-0[42599]: -> stage-1 synchronisation loop 
DEBU[0000] nsexec-1[42600]: ~> nsexec stage-1           
DEBU[0000] nsexec-1[42600]: unshare remaining namespaces 
DEBU[0000] nsexec-1[42600]: spawn stage-2               
DEBU[0000] nsexec-1[42600]: request stage-0 to forward stage-2 pid (42601) 
DEBU[0000] nsexec-0[42599]: stage-1 requested pid to be forwarded 
DEBU[0000] nsexec-0[42599]: forward stage-1 (42600) and stage-2 (42601) pids to runc 
DEBU[0000] nsexec-2[1]: ~> nsexec stage-2               
DEBU[0000] nsexec-1[42600]: signal completion to stage-0 
DEBU[0000] nsexec-1[42600]: <~ nsexec stage-1           
DEBU[0000]libcontainer/process_linux.go:457 libcontainer.(*initProcess).goCreateMountSources.func1() mount source thread: successfully running in container mntns 
DEBU[0000] nsexec-0[42599]: stage-1 complete            
DEBU[0000] nsexec-0[42599]: <- stage-1 synchronisation loop 
DEBU[0000] nsexec-0[42599]: -> stage-2 synchronisation loop 
DEBU[0000] nsexec-0[42599]: signalling stage-2 to run   
DEBU[0000] nsexec-2[1]: signal completion to stage-0    
DEBU[0000] nsexec-2[1]: <= nsexec container setup       
DEBU[0000] nsexec-2[1]: booting up go runtime ...       
DEBU[0000] nsexec-0[42599]: stage-2 complete            
DEBU[0000] nsexec-0[42599]: <- stage-2 synchronisation loop 
DEBU[0000] nsexec-0[42599]: <~ nsexec stage-0           
DEBU[0000]libcontainer/sync.go:127 libcontainer.doReadSync() reading sync                                 
DEBU[0000] sync pipe closed                             
DEBU[0000] mount source thread: closing thread: context canceled 
ERRO[0000] runc run failed: unable to start container process: error during container init: procReady not received

kolyshkin · 2024-02-07T21:07:04Z

As go has released v1.22.0, so there is no 1.20.x in https://go.dev/dl/?mode=json anymore.

This can be fixed by adding &include=all (i.e. use https://go.dev/dl/?mode=json&include=all). I'll open a PR.

kolyshkin · 2024-02-07T21:46:11Z

Interestingly, both runc 1.1.12 and runc from git HEAD built with go1.22.0 work fine on my machine (all tests are passing).

kolyshkin · 2024-02-07T21:55:22Z

We also need to fix this for Go 1.22

# (in test file tests/integration/spec.bats, line 37)
#   `GO111MODULE=auto go get github.com/xeipuuv/gojsonschema' failed
# runc spec (status=0):
#
# Cloning into 'runtime-spec'...
# HEAD is now at 4fec88f merge #1219 into main
# go: go.mod file not found in current directory or any parent directory.
# 	'go get' is no longer supported outside a module.
# 	To build and install a command, use 'go install' with a version,
# 	like 'go install example.com/cmd@latest'
# 	For more information, see https://golang.org/doc/go-get-install-deprecation
# 	or run 'go help get' or 'go help install'.

I don't remember why I haven't switched to go install, guess it's not as easy as it seems.

lifubang · 2024-02-09T05:58:28Z

Interestingly, both runc 1.1.12 and runc from git HEAD built with go1.22.0 work fine on my machine (all tests are passing).

It seems that cgo may be broken with clone(2) in go1.22.0?
golang/go#65625
PTAL

kolyshkin · 2024-02-09T06:33:55Z

Interestingly, both runc 1.1.12 and runc from git HEAD built with go1.22.0 work fine on my machine (all tests are passing).

It seems that cgo may be broken with clone(2) in go1.22.0? golang/go#65625 PTAL

Again, I can't repro locally.

[kir@kir-tp1 cgoclone2]$ go version
go version go1.21.6 linux/amd64
[kir@kir-tp1 cgoclone2]$ go run main.go 
STAGE_PARENT
STAGE_CHILD
STAGE_INIT
This from nsexec
From main!
[kir@kir-tp1 cgoclone2]$ go1.22.0 version
go version go1.22.0 linux/amd64
[kir@kir-tp1 cgoclone2]$ go1.22.0 run main.go 
STAGE_PARENT
STAGE_CHILD
STAGE_INIT
This from nsexec
From main!

Maybe it's your kernel version @lifubang? Can you show uname -a?

kolyshkin · 2024-02-09T06:43:44Z

@lifubang also if you can repro that (alas I can not), you can git bisect golang between 1.21.0 and 1.22.0.

kolyshkin · 2024-02-09T06:48:28Z

Note in CI it happens with Ubuntu 20.04 but not Ubuntu 22.04. Will try to repro in a VM.

kolyshkin · 2024-02-09T07:41:54Z

On Ubuntu 20.04, when running the binary compiled with go 1.22, I am seeing a SIGSEGV:

--- SIGSEGV {si_signo=SIGSEGV, si_code=SEGV_MAPERR, si_addr=0xf8} ---

Can't yet figure out what's going on there; will continue tomorrow.

kolyshkin · 2024-02-09T08:18:21Z

@lifubang I did a bisect, here are the results: golang/go#65625 (comment)

Will continue tomorrow.

kolyshkin · 2024-02-16T03:26:42Z

It seems that cgo may be broken with clone(2) in go1.22.0?
golang/go#65625

So, to summarize the investigation done there -- it's a glibc bug, in fact, two bugs:

pthread_self() returns wrong info after we do what we do in libct/nsenter
pthread_getattr_np(pthread_self(), &attr) (which Go 1.22 calls internally) does a NULL pointer dereference, so the app gets SIGABRT.

These two bugs are apparently specific to glibc used by Ubuntu 20.04 (libc6 2.31-0ubuntu9.14) and maybe also Debian 10 (libc6 2.28-10+deb10u2), as I was able to reproduce on both. With Debian 10, it even prints error from free: free(): invalid pointer, maybe due to some extra Debian-specific patches, but still gets SIGABRT.

For some reason I was unable to repro on older Fedora (F32, glibc-2.31-2.fc32, F33, glibc-2.32-10.fc33) and Debian 11 (libc6 2.31-7).

The bad news is, every version of glibc has the bug 1 above, and https://go-review.googlesource.com/c/go/+/563379 may make it so go 1.22.x will fail runc init on every version of glibc.

Meaning, we need a workaround for that. Perhaps changing runc libct/nsenter logic in some radical way, so that pthread_self works.

Go 1.22 currently causes crashes on older Debian/Ubuntu systems. lxc/incus#497 golang/go#65625 opencontainers/runc#4193 Signed-off-by: Stéphane Graber <[email protected]>

AkihiroSuda · 2024-03-04T22:51:31Z

Meaning, we need a workaround for that. Perhaps changing runc libct/nsenter logic in some radical way, so that pthread_self works.

👍

kolyshkin · 2024-03-28T01:10:01Z

Rebasing this to re-run with Go 1.22.1

kolyshkin · 2024-03-28T01:17:57Z

Sorry @lifubang I've high-jacked your PR, needed to run it with Go 1.22.1 and added missing changes to go.sum to fix failing CI (https://github.com/opencontainers/runc/actions/runs/8460901105/job/23179867537)

kolyshkin · 2024-03-28T01:19:54Z

OK, Go 1.22.1 makes no difference. I guess we have to disable Go 1.22 for now.

lifubang · 2024-04-11T23:29:05Z

Do you think moving the c code in stage-0 and stage-2 to golang code could fix this issue or not? I don't know whether the setjmp and longjmp could work in go or not, and it can using together with C or not?
Does it worth to try?

lifubang · 2024-04-11T23:50:20Z

and it can using together with C or not?

I think maybe no, because after clone(2), it has already in go routine, it can’t longjmp to C.

So, it seems that there is no other way to fix this issue?

cyphar · 2024-04-13T15:56:37Z

This patch also works, while still allowing us to use CLONE_PARENT. Yes, I'm sure we agree it's not lovely, but IMHO using fork() is depending on glibc internals just as much as this is. If glibc stops using CLONE_CHILD_CLEARTID then fork() will also stop working. The only downside of this approach is that it only works with CONFIG_CHECKPOINT_RESTORE=y but I suspect most people running with containers have that enabled.

diff --git a/libcontainer/nsenter/nsexec.c b/libcontainer/nsenter/nsexec.c
index c771ac7e1165..319899bd9b71 100644
--- a/libcontainer/nsenter/nsexec.c
+++ b/libcontainer/nsenter/nsexec.c
@@ -15,6 +15,7 @@
 #include <stdbool.h>
 #include <string.h>
 #include <unistd.h>
+#include <pthread.h> /* _only_ used for pthread_self() in debug log */
 
 #include <sys/ioctl.h>
 #include <sys/prctl.h>
@@ -319,7 +320,41 @@ static int clone_parent(jmp_buf *env, int jmpval)
 		.jmpval = jmpval,
 	};
 
-	return clone(child_func, ca.stack_ptr, CLONE_PARENT | SIGCHLD, &ca);
+	/*
+	 * Since glibc 2.25 (see c579f48edba88380635ab98cb612030e3ed8691e),
+	 * glibc no longer updates the TLS state containing the current process
+	 * tid after clone(2). This results in stale TIDs being used when Go
+	 * 1.22 and later call pthread_gettattr_np(pthread_self()), resulting
+	 * in crashes on ancient glibcs and errors on newer glibcs.
+	 *
+	 * Luckily, because the same address is used for CLONE_PARENT_SETTID,
+	 * we can poke around in glibc's internal cache by getting the address
+	 * using PR_GET_TID_ADDRESS (only available in Linux >= 3.5, with
+	 * CONFIG_CHECKPOINT_RESTORE=y) and then overwriting it with
+	 * CLONE_CHILD_SETTID. CLONE_CHILD_CLEARTID is set to allow descendant
+	 * PR_GET_TID_ADDRESS calls to work, as well as matching what glibc
+	 * does in arch_fork().
+	 *
+	 * Yes, this is pretty horrific, but the core issue here is that we
+	 * need to run Go code ("runc init") in the child after fork(), which
+	 * is not allowed by glibc (see signal-safety(7)). We cannot exec to
+	 * solve the problem because we are in a security critical situation
+	 * here, and doing an exec would allow for container escapes (obvious
+	 * issues include that the shared libraries loaded from a re-exec would
+	 * come from the container, and doing an exec here would clear the bit
+	 * that makes non-dumpable flags effective for userns containers with
+	 * CAP_SYS_PTRACE).
+	 */
+	pid_t *tid_addr = NULL;
+	if (prctl(PR_GET_TID_ADDRESS, &tid_addr) < 0)
+		/* what should we do here... */;
+	write_log(DEBUG, "nsenter clone: get_tid_address gave us %p (pthread_self=%p)", tid_addr, (void *) pthread_self());
+	if (!tid_addr || *tid_addr != gettid())
+		write_log(WARNING, "nsenter clone: glibc private tid address is wrong: *%p %d != gettid() %d", tid_addr, tid_addr ? *tid_addr : -1, gettid());
+
+	return clone(child_func, ca.stack_ptr,
+		     CLONE_PARENT | CLONE_CHILD_SETTID | CLONE_CHILD_CLEARTID | SIGCHLD, &ca,
+		     NULL /* parent_tid */ , NULL /* tls */ , tid_addr);
 }
 
 /* Returns the clone(2) flag for a namespace, given the name of a namespace. */

@kolyshkin wdyt?

cyphar · 2024-04-13T15:58:27Z

@lifubang I can also take a look next week at whether we can somehow remove stage-1 so that we don't need a grandchild (which would remove the need for PR_SET_CHILD_SUBREAPER).

lifubang · 2024-05-15T01:00:21Z

This PR needs some refactor work, so convert it to draft state.

runc/exec.go

Line 184 in df04ed4

enableSubreaper: false,

Signed-off-by: lifubang <[email protected]>

lifubang · 2024-05-17T10:45:42Z

Welcome more suggestions.

As the description in opencontainers#4233, there is a bug in glibc, pthread_self() will return wrong info after we do `clone(CLONE_PARENT)` in libct/nsenter, it will cause runc can't work in `go 1.22.*`. So we use fork(2) to replace clone(2) in libct/nsenter, but there is a double-fork in nsenter, so we need to use `PR_SET_CHILD_SUBREAPER` to let runc can reap grand child process in libct/nsenter. Signed-off-by: lifubang <[email protected]>

This reverts commit ac31da6. Signed-off-by: lifubang <[email protected]>

This reverts commit e377e16. Signed-off-by: lifubang <[email protected]>

lifubang · 2024-05-18T01:40:36Z

signals.go

-			logrus.Warn(err)
-		}
-	}
+func newSignalHandler(notifySocket *notifySocket) *signalHandler {


As @kolyshkin pointed out in #4278 (comment)
This PR has done such things. But there is still one thing I can't determine whether we should do or not:

If we use PR_SET_CHILD_SUBREAPER and fork(2) to replace clone(CLONE_PARENT), I think we should move this signal handler to libcontainer. Or else someone use libcontainer directly in the code will have to write a signal handler by themselves.

kolyshkin · 2024-06-06T00:03:46Z

closing in favor of #4292

This comment was marked as outdated.

Sign in to view

kolyshkin mentioned this pull request Feb 8, 2024

Close sync pipe explicitly in exec #4192

Merged

This was referenced Feb 8, 2024

[ci] update go version to 1.21 in cirrus ci #4198

Merged

runtime: SIGSEGV after performing clone(CLONE_PARENT) via C constructor prior to runtime start golang/go#65625

Open

dtrudg mentioned this pull request Feb 26, 2024

SIGSEV (signal 11 error) with Go 1.22.0 and Ubuntu 20.04 / Debian 10 sylabs/singularity#2677

Closed

kolyshkin force-pushed the feat-go-1.21-1.22 branch 2 times, most recently from c54384f to 5907889 Compare March 28, 2024 01:16

lifubang mentioned this pull request Mar 28, 2024

[1.1] runc list: fix race with runc delete #4231

Merged

dongsupark mentioned this pull request Mar 28, 2024

[RFE] Update the default Go to 1.21 with exceptions of Docker/containerd flatcar/Flatcar#1382

Closed

3 tasks

cyphar mentioned this pull request Mar 29, 2024

runc doesn't work with go1.22 #4233

Closed

lifubang force-pushed the feat-go-1.21-1.22 branch 3 times, most recently from 3144923 to aebd5b5 Compare March 30, 2024 16:22

lifubang force-pushed the feat-go-1.21-1.22 branch from 7eac8b8 to df04ed4 Compare April 13, 2024 05:28

cyphar mentioned this pull request Apr 14, 2024

nsenter: overwrite glibc's internal tid cache on clone() #4247

Closed

lifubang mentioned this pull request Apr 29, 2024

[WIP] use go 1.22 #4262

Closed

lifubang mentioned this pull request May 12, 2024

Use go 1.22 #4278

Closed

lifubang marked this pull request as draft May 15, 2024 00:58

lifubang force-pushed the feat-go-1.21-1.22 branch from df04ed4 to 41d8548 Compare May 16, 2024 10:35

bump go to 1.22.x

6916425

Signed-off-by: lifubang <[email protected]>

lifubang force-pushed the feat-go-1.21-1.22 branch 2 times, most recently from 1253158 to bdec4c7 Compare May 17, 2024 10:07

lifubang marked this pull request as ready for review May 17, 2024 10:44

lifubang requested review from AkihiroSuda and cyphar May 17, 2024 10:45

lifubang added 3 commits May 18, 2024 07:11

Revert "ci/cross-i386: pin Go to 1.21.x"

2366a6b

This reverts commit ac31da6. Signed-off-by: lifubang <[email protected]>

Revert "[hotfix] nsenter: refuse to build with Go 1.22 on glibc"

1e60e59

This reverts commit e377e16. Signed-off-by: lifubang <[email protected]>

lifubang force-pushed the feat-go-1.21-1.22 branch from bdec4c7 to 1e60e59 Compare May 17, 2024 23:14

lifubang marked this pull request as draft May 17, 2024 23:38

lifubang marked this pull request as ready for review May 17, 2024 23:43

lifubang marked this pull request as draft May 18, 2024 00:03

lifubang commented May 18, 2024

View reviewed changes

kolyshkin closed this Jun 6, 2024

kolyshkin removed the backport/1.1-todo A PR in main branch which needs to be backported to release-1.1 label Jun 6, 2024

lifubang deleted the feat-go-1.21-1.22 branch October 15, 2024 05:46

Comments

Conversation

lifubang commented Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lifubang commented Feb 7, 2024

Uh oh!

kolyshkin commented Feb 7, 2024

Uh oh!

kolyshkin commented Feb 7, 2024

Uh oh!

This comment was marked as outdated.

kolyshkin commented Feb 7, 2024

Uh oh!

lifubang commented Feb 9, 2024

Uh oh!

kolyshkin commented Feb 9, 2024

Uh oh!

kolyshkin commented Feb 9, 2024

Uh oh!

kolyshkin commented Feb 9, 2024

Uh oh!

kolyshkin commented Feb 9, 2024

Uh oh!

kolyshkin commented Feb 9, 2024

Uh oh!

kolyshkin commented Feb 16, 2024

Uh oh!

AkihiroSuda commented Mar 4, 2024

Uh oh!

kolyshkin commented Mar 28, 2024

Uh oh!

kolyshkin commented Mar 28, 2024

Uh oh!

kolyshkin commented Mar 28, 2024

Uh oh!

lifubang commented Apr 11, 2024

Uh oh!

lifubang commented Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyphar commented Apr 13, 2024

Uh oh!

cyphar commented Apr 13, 2024

Uh oh!

lifubang commented May 15, 2024

Uh oh!

lifubang commented May 17, 2024

Uh oh!

lifubang May 18, 2024

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Jun 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lifubang commented Feb 7, 2024 •

edited

Loading

lifubang commented Apr 11, 2024 •

edited

Loading