Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

syscall: Setgroups may hang in go 1.16+ when not using cgo #50113

Closed
weixiao-huang opened this issue Dec 12, 2021 · 35 comments
Closed

syscall: Setgroups may hang in go 1.16+ when not using cgo #50113

weixiao-huang opened this issue Dec 12, 2021 · 35 comments
Assignees
Labels
NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@weixiao-huang
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.16 linux/amd64

Does this issue reproduce with the latest release?

Yes, this problem will also exist in go 1.17.5, but not exist in go 1.15.x.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/demo/.cache/go-build"
GOENV="/home/demo/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/demo/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/demo/go"
GOPRIVATE=""
GOPROXY="https://goproxy.cn,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/demo/golang-setgroups-hang/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build3393189053=/tmp/go-build -gno-record-gcc-switches"

What did you do?

See https://github.com/weixiao-huang/golang-setgroups-hang

What did you expect to see?

client --key-path /.launch/key --server=localhost:2222 should not hang while compiled by golang 1.16.x and 1.17.x

What did you see instead?

client --key-path /.launch/key --server=localhost:2222 will hang while compiled by golang 1.16.x and 1.17.x

@weixiao-huang weixiao-huang changed the title affected/package: syscall.Setgroups may hang in go 1.16+ affected/package: syscall.Setgroups() may hang in go 1.16+ Dec 12, 2021
@seankhliao seankhliao changed the title affected/package: syscall.Setgroups() may hang in go 1.16+ syscall: Setgroups() may hang in go 1.16+ Dec 12, 2021
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 12, 2021
@ianlancetaylor ianlancetaylor changed the title syscall: Setgroups() may hang in go 1.16+ syscall: Setgroups may hang in go 1.16+ when not using cgo Dec 12, 2021
@ianlancetaylor
Copy link
Contributor

CC @AndrewGMorgan

  1. Does it make a difference if you remove the call to runtime.LockOSThread?
  2. You are doing a lot of operations in makeCommand that are normally done by setting fields in cmd.SysProcAttr. That is, instead of calling syscall.Setgroups, set cmd.SysProcAttr.Credential.Groups. To be clear, there may still be a bug here as syscall.Setgroups shouldn't hang, but it appears that the code would be better written if it used the SysProcAttr fields.

@ianlancetaylor ianlancetaylor added this to the Go1.19 milestone Dec 12, 2021
@AndrewGMorgan
Copy link
Contributor

Assign this to me. I'll try to figure out what is going on.

@AndrewGMorgan
Copy link
Contributor

First, go1.16 was buggy. The first version that had no known issues with things like runtime.AllThreadsSyscall() and/or the corresponding cgo redirected use of C.setgroups() was go1.16.4.

I'll have to figure out the docker thing - debugging in that environment might take some setup on my part.

@AndrewGMorgan
Copy link
Contributor

Based on the go env output CGO_ENABLED=1 which implies that the runtime.AllThreadsSyscall() support is not being used in this case. Instead, syscall.Setgroups() is being redirected to the C.setgroups() function in go1.16+.

In that and all prior versions of Go, calling C.setgroups() was observed to periodically hang (#42494), later in the go1.16 minor releases, we also found some other hang issues when interrupt handling was in use: #43149 and #44193 discuss aspects of this. This code appears to have both signal handling and the implicit use of the C.setgroups() function. As noted above, I would not expect this to work reliably before go1.16.4.

@ianlancetaylor gives the best advice for restructuring this code: that the SysProcAttr configuration for the cmd is really the way to execute the program with different credentials from the server. Setting up process credentials is really tricky with Go's runtime. I think your best bet is to go with Ian's advice.

As to the code as it stands, it looks like this code is trying to work with two models of privilege at the same time.

  • One is it uses its own Setuid() things in server/util.go, which are OS thread specific and will only work reliably if the running thread is runtime.LockOSThread()ed.
  • The other model is using syscall.Setgroups() which, is not thread specific but process-shared (changes the groups of all threads in the runtime).

If you really want to roll your own with Raw system calls, I think you would do well to write your own Setgroups() function so it only affects the running thread. So far as I am aware, things like syscall.Setrlimit() and os.Chdir() are process-wide so even in these cases, you will also have some interesting process/thread interactions.

@weixiao-huang
Copy link
Author

weixiao-huang commented Dec 13, 2021

Based on the go env output CGO_ENABLED=1

I'm sorry that I haven't set CGO_ENABLED=0 for this go env command. This problem won't happen while CGO_ENABLED=1. You can see I set CGO_ENABLED=0 in the docker build, which will cause this hanging problem.

@weixiao-huang
Copy link
Author

weixiao-huang commented Dec 13, 2021

First, go1.16 was buggy. The first version that had no known issues with things like runtime.AllThreadsSyscall() and/or the corresponding cgo redirected use of C.setgroups() was go1.16.4.

I'll have to figure out the docker thing - debugging in that environment might take some setup on my part.

This problem also happened in the latest go1.17.5. Also, I've tried just now, go1.16.12 has also hang.

@AndrewGMorgan
Copy link
Contributor

The io.Copy() angle is interesting. Assign this bug to me, I'll post an update when I can reproduce things. If you can find a smaller example that also hangs that would also be much appreciated.

@tklauser
Copy link
Member

@AndrewGMorgan I've assigned this issue to you per your request.

@AndrewGMorgan
Copy link
Contributor

There is a great deal of code in this combined system.

I've been trying to isolate the code actually tripping whatever the issue is and build a minimal reproduction by skipping lots of it. If I remove the pty stuff and hard code the client to not request session.RequestPty(), things seem to successfully syscall.Setgroups and progress beyond that:

2021/12/15 21:19:48 ========= 2 ==========: [10 18 54 1000]
2021/12/15 21:19:48 ========= 3 ==========: 1000
2021/12/15 21:19:48 ========= 4 ==========: 1000
2021/12/15 21:19:48 Execute

@weixiao-huang
Copy link
Author

Yes, it seems pty may influence the behavior of Setgroups, but I don't know why

@AndrewGMorgan
Copy link
Contributor

How reproducible is this failure mode? I'm not running under Docker since I want to debug in a familiar environment. Is this bug report claiming that Docker is a necessary ingredient to the failure mode?

Are you 100% sure they are not locking up somewhere in the log.WithError(...).Warn() ? The forest of dependencies and extra imports from the logrus thing make me worried the problem could be something else entirely...

When you say it reproduces with go1.17.5 #50113 (comment) I wonder if you could do the following while in the locked-up state?

$ sudo gdb ./name-of-server-binary
(gdb) attach <PID OF RUNNING SERVER BINARY>
(gdb) info th

I re-enabled the pty package, but completely replaced the logrus package use with the standard log.Print() and log.Printf() functions to cut down on the amount of code I need to follow, and again things appear to work just fine:

2021/12/16 17:22:39 ========= 2 ==========: [10 18 54 1000]
2021/12/16 17:22:39 ========= 3 ==========: 1000
2021/12/16 17:22:39 ========= 4 ==========: 1000
2021/12/16 17:22:39 Execute
2021/12/16 17:22:57 Exit successfully
2021/12/16 17:22:57 session quited
2021/12/16 17:22:57 [copy sshChan to pty] ~~~~~~~~~~ 1.1.2 ~~~~~~~~~

In your write-up, you say:

	log.Infof("========= 2 ==========: %+v", lr.GIDs)
	if len(lr.GIDs) != 0 {
		if err := syscall.Setgroups(lr.GIDs); err != nil { // <- Server hangs here
			log.WithError(err).Warn("Setgroups")
		}
	}

So, since things work for me when I run with golang's default log.Print(), I'm wondering if the problem is actually on the next line?

@AndrewGMorgan
Copy link
Contributor

AndrewGMorgan commented Dec 17, 2021

FWIW When I ran go mod tidy after reinserting the pty handling code it also included v1.1.8.

@AndrewGMorgan
Copy link
Contributor

I managed to reproduce a hang in a manner that I can debug a bit. It appears to involve the main.CopyWithContext.func1 () line io.Copy(a,b). That is the goroutine invoked with a=s.sshChan and b=s.pty.

func CopyWithContext(ctx context.Context, a, b io.ReadWriteCloser) {
	log.Infof("~~~~~~~~~~ 1 ~~~~~~~~~")
	ctx, cancel := context.WithCancel(ctx)
	go func() {
		log.Infof("[copy sshChan to pty] ~~~~~~~~~~ 1.1.1 ~~~~~~~~~")
		io.Copy(a, b)         <----- THIS LINE
		cancel()
		log.Infof("[copy sshChan to pty] ~~~~~~~~~~ 1.1.2 ~~~~~~~~~")
	}()
	go func() {
		log.Infof("[copy pty to sshChan] ~~~~~~~~~~ 1.2.1 ~~~~~~~~~")
		io.Copy(b, a)
		cancel()
		log.Infof("[copy pty to sshChan] ~~~~~~~~~~ 1.2.1 ~~~~~~~~~")
	}()
	<-ctx.Done()
	a.Close()
	b.Close()
	log.Infof("~~~~~~~~~~~~ 2 ~~~~~~~~~~~~~")
}

@weixiao-huang
Copy link
Author

Yes, this function may influence the behavior. But I don't how why this function may due to the hanging problem

@AndrewGMorgan
Copy link
Contributor

Thanks for this bug report and the reproducer.

Some code spelunking and web surfing later, this looks like a deadlock issue with blocking read system calls. It seems to share some underlying similarity to #38618 for example. However, the victim-vector here is that the syscall.AllThreadsSyscall() needs to execute code on every thread, but one thread (the one blocked reading s.pty) stubbornly refuses to exit the kernel, so the all threads syscall can never complete.

It appears that the core of the issue is that os.OpenFile() etc code doesn't treat s.pty in a pollable way. That is, the way the "github.com/kr/pty" package opens the pseudo terminal and operates on its s.pty.Fd(), it has blocking semantics. As explained in #38618 there is a workaround. I've implemented a sketch of this workaround with a patch (see below) to your server code (which drops the "github.com/kr/pty" package in favor of inlining the minimally required pty stuff for Linux).

This user code workaround is not that satisfying as a solution because it leaves open the possibility that the all threads syscall mechanism can deadlock in other situations like this. Hopefully, however, it can unblock (no pun intended!) your work.

I'd also urge you to reconsider using the fragile single-OS thread syscall.RawSyscall()'s to set uid and gid, but the syscall.AllThreadsSyscall() backed syscall.Setgroups() implementation for setting groups. This feels like crossing beams in a way that probably won't end well. You should take a look at using the SysProcAttr mechanism as @ianlancetaylor suggests above, or embrace something like the cap (shameless plug) package to support proper Linux privilege management within your server.

I'm next going to explore if I can create a more minimal reproducer for this deadlock and then explore the patch @ianlancetaylor briefly suggested in #38618 which seems like it will cover more corner cases than this user code workaround.

From 073948275eadec1b3de96195fed04e3edd54b9e0 Mon Sep 17 00:00:00 2001
From: "Andrew G. Morgan" <morgan@kernel.org>
Date: Sat, 15 Jan 2022 18:13:27 -0800
Subject: [PATCH] Inline a non-blocking pty configuration

This is a workaround for a deadlock in the syscall.AllThreadsSyscall()
implementation when one of the OS threads refuses to exit the kernel.

See https://github.com/golang/go/issues/50113 for details.

Signed-off-by: Andrew G. Morgan <morgan@kernel.org>
---
 server/pty.go | 94 ++++++++++++++++++++++++++++++++++++++++++++++-----
 server/tcp.go |  1 +
 2 files changed, 86 insertions(+), 9 deletions(-)

diff --git a/server/pty.go b/server/pty.go
index 9b6d951..50431d5 100644
--- a/server/pty.go
+++ b/server/pty.go
@@ -2,11 +2,12 @@ package main
 
 import (
 	"context"
+	"fmt"
 	"io"
+	"os"
 	"syscall"
 	"unsafe"
 
-	krpty "github.com/kr/pty"
 	log "github.com/sirupsen/logrus"
 	"golang.org/x/crypto/ssh"
 )
@@ -20,6 +21,85 @@ type PtyReq struct {
 	TermModes   string
 }
 
+// openNonBlocking returns a filedescriptor opened in non-blocking
+// mode and a file pointing to it.
+func openNonBlocking(path string, mode int) (fd uintptr, f *os.File, err error) {
+	var p int
+	p, err = syscall.Open("/dev/ptmx", mode, 0)
+	if err != nil {
+		return
+	}
+	fd = uintptr(p)
+
+	defer func() {
+		if err != nil {
+			if f != nil {
+				f.Close()
+				f = nil
+			} else {
+				syscall.Close(p)
+			}
+			fd = ^uintptr(0)
+		}
+	}()
+
+	err = syscall.SetNonblock(p, true)
+	if err != nil {
+		return
+	}
+
+	f = os.NewFile(fd, path)
+	return
+}
+
+var badFd = ^uintptr(0)
+
+//go:uintptrescapes
+func ioctl(a1, a2, a3 uintptr) (err error) {
+	_, _, errno := syscall.Syscall(syscall.SYS_IOCTL, a1, a2, a3)
+	if errno == 0 {
+		return nil
+	}
+	return errno
+}
+
+// open configures a pty for use by the server.
+func open() (fd uintptr, pty, tty *os.File, err error) {
+	if fd, pty, err = openNonBlocking("/dev/ptmx", syscall.O_RDWR); err != nil {
+		fd = badFd
+		return
+	}
+
+	// In case of error after this point, make sure we close the pty and tty.
+	defer func() {
+		if err != nil {
+			if pty != nil {
+				pty.Close()
+				pty = nil
+			}
+			if tty != nil {
+				tty.Close()
+				tty = nil
+			}
+			fd = badFd
+		}
+	}()
+
+	var n uint
+	if err = ioctl(fd, syscall.TIOCGPTN, uintptr(unsafe.Pointer(&n))); err != nil {
+		return
+	}
+	sname := fmt.Sprint("/dev/pts/", n)
+
+	var u int
+	if err = ioctl(fd, syscall.TIOCSPTLCK, uintptr(unsafe.Pointer(&u))); err != nil {
+		return
+	}
+
+	tty, err = os.OpenFile(sname, syscall.O_RDWR|syscall.O_NOCTTY, 0)
+	return
+}
+
 func (s *session) handlePtyReq(ctx context.Context, req *ssh.Request) bool {
 	if s.pty != nil {
 		return false
@@ -32,12 +112,12 @@ func (s *session) handlePtyReq(ctx context.Context, req *ssh.Request) bool {
 		return false
 	}
 
-	if s.pty, s.tty, err = krpty.Open(); err != nil {
-		log.WithError(err).Error("krpty.Open")
+	if s.ptyFd, s.pty, s.tty, err = open(); err != nil {
+		log.WithError(err).Error("pty open")
 		return false
 	}
 
-	if err = setWinsize(s.pty.Fd(), ptyReq.Width, ptyReq.Height); err != nil {
+	if err = setWinsize(s.ptyFd, ptyReq.Width, ptyReq.Height); err != nil {
 		log.WithError(err).Error("Set window initial size")
 		return false
 	}
@@ -55,11 +135,7 @@ func setWinsize(fd uintptr, w, h uint32) error {
 		y      uint16 // unused
 	}
 	ws := &Winsize{Width: uint16(w), Height: uint16(h), x: 0, y: 0}
-	_, _, errno := syscall.Syscall(syscall.SYS_IOCTL, fd, uintptr(syscall.TIOCSWINSZ), uintptr(unsafe.Pointer(ws)))
-	if errno != 0 {
-		return errno
-	}
-	return nil
+	return ioctl(fd, uintptr(syscall.TIOCSWINSZ), uintptr(unsafe.Pointer(ws)))
 }
 
 func CopyWithContext(ctx context.Context, a, b io.ReadWriteCloser) {
diff --git a/server/tcp.go b/server/tcp.go
index 880ab40..dc79da8 100644
--- a/server/tcp.go
+++ b/server/tcp.go
@@ -64,6 +64,7 @@ func prepared(req *ssh.Request, ch chan<- int) {
 type session struct {
 	sshConn  *ssh.ServerConn
 	sshChan  ssh.Channel
+	ptyFd    uintptr
 	pty, tty *os.File
 	cmd      *exec.Cmd
 }
-- 
2.34.1

@AndrewGMorgan
Copy link
Contributor

Perhaps a more elegant workaround is to completely avoid the need to ever call s.pty.Fd(). It is a side effect of this call that forces the reads to become blocking and non-pollable.

While we still inline the pseudo terminal setup to avoid the blocking triggered by "github.com/kr/pty", the following server patch implements the ioctl functionality as operating on the *os.File directly in such a way that it does not introduce non-pollable reads. That is, this doesn't require the rest of the code to ever pay attention to the raw file descriptor. As such, the code can use os.OpenFile() as per the original code:

From 12c4649cc5eb502cee3c21b387d40a07a62c9192 Mon Sep 17 00:00:00 2001
From: "Andrew G. Morgan" <morgan@kernel.org>
Date: Sat, 15 Jan 2022 22:08:07 -0800
Subject: [PATCH] Inline pollable pty configuration code

This workaround avoids calling s.pty.Fd() and thus avoids s.pty from
becoming non-pollable. In this way we avoid a deadlock in the
syscall.AllThreadsSyscall() implementation when one of the OS threads
refuses to exit the kernel.

See https://github.com/golang/go/issues/50113 for details.

Signed-off-by: Andrew G. Morgan <morgan@kernel.org>
---
 server/pty.go | 69 +++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 59 insertions(+), 10 deletions(-)

diff --git a/server/pty.go b/server/pty.go
index 9b6d951..dbb365a 100644
--- a/server/pty.go
+++ b/server/pty.go
@@ -2,11 +2,12 @@ package main
 
 import (
 	"context"
+	"fmt"
 	"io"
+	"os"
 	"syscall"
 	"unsafe"
 
-	krpty "github.com/kr/pty"
 	log "github.com/sirupsen/logrus"
 	"golang.org/x/crypto/ssh"
 )
@@ -20,6 +21,58 @@ type PtyReq struct {
 	TermModes   string
 }
 
+//go:uintptrescapes
+func ioctlFile(f *os.File, a1, a2 uintptr) error {
+	s, err := f.SyscallConn()
+	if err != nil {
+		return err
+	}
+	s.Control(func(fd uintptr) {
+		_, _, errno := syscall.Syscall(syscall.SYS_IOCTL, fd, a1, a2)
+		if errno == 0 {
+			return
+		}
+		err = errno
+	})
+	return err
+}
+
+// open configures a pty for use by the server.
+func open() (pty, tty *os.File, err error) {
+	if pty, err = os.OpenFile("/dev/ptmx", syscall.O_RDWR, 0); err != nil {
+		return
+	}
+
+	// In case of error after this point, make sure we close the
+	// pty and tty.
+	defer func() {
+		if err != nil {
+			if pty != nil {
+				pty.Close()
+				pty = nil
+			}
+			if tty != nil {
+				tty.Close()
+				tty = nil
+			}
+		}
+	}()
+
+	var n uint
+	if err = ioctlFile(pty, syscall.TIOCGPTN, uintptr(unsafe.Pointer(&n))); err != nil {
+		return
+	}
+	sname := fmt.Sprint("/dev/pts/", n)
+
+	var u int
+	if err = ioctlFile(pty, syscall.TIOCSPTLCK, uintptr(unsafe.Pointer(&u))); err != nil {
+		return
+	}
+
+	tty, err = os.OpenFile(sname, syscall.O_RDWR|syscall.O_NOCTTY, 0)
+	return
+}
+
 func (s *session) handlePtyReq(ctx context.Context, req *ssh.Request) bool {
 	if s.pty != nil {
 		return false
@@ -32,12 +85,12 @@ func (s *session) handlePtyReq(ctx context.Context, req *ssh.Request) bool {
 		return false
 	}
 
-	if s.pty, s.tty, err = krpty.Open(); err != nil {
-		log.WithError(err).Error("krpty.Open")
+	if s.pty, s.tty, err = open(); err != nil {
+		log.WithError(err).Error("pty open")
 		return false
 	}
 
-	if err = setWinsize(s.pty.Fd(), ptyReq.Width, ptyReq.Height); err != nil {
+	if err = setWinsize(s.pty, ptyReq.Width, ptyReq.Height); err != nil {
 		log.WithError(err).Error("Set window initial size")
 		return false
 	}
@@ -47,7 +100,7 @@ func (s *session) handlePtyReq(ctx context.Context, req *ssh.Request) bool {
 }
 
 // setWinsize sets the size of the given pty.
-func setWinsize(fd uintptr, w, h uint32) error {
+func setWinsize(f *os.File, w, h uint32) error {
 	type Winsize struct {
 		Height uint16
 		Width  uint16
@@ -55,11 +108,7 @@ func setWinsize(fd uintptr, w, h uint32) error {
 		y      uint16 // unused
 	}
 	ws := &Winsize{Width: uint16(w), Height: uint16(h), x: 0, y: 0}
-	_, _, errno := syscall.Syscall(syscall.SYS_IOCTL, fd, uintptr(syscall.TIOCSWINSZ), uintptr(unsafe.Pointer(ws)))
-	if errno != 0 {
-		return errno
-	}
-	return nil
+	return ioctlFile(f, uintptr(syscall.TIOCSWINSZ), uintptr(unsafe.Pointer(ws)))
 }
 
 func CopyWithContext(ctx context.Context, a, b io.ReadWriteCloser) {
-- 
2.34.1

@ianlancetaylor
Copy link
Contributor

It seems like a general problem if a blocking system call can block syscall.Setgroups (although not necessarily a very important one). Would it be OK to have a ragged edge ending the call, in which Setgroups can return even though not every thread has yet executed the call, as long as the thread is sure to execute the call before returning to Go code? If not, then I guess we could use a signal, although that seems like a lot of code for a very rare case.

@AndrewGMorgan
Copy link
Contributor

The problem with delaying a syscall like this it that while this thread is blocked there may be another call to something that also needs to run a privileged operation on all threads. Typically, these sorts of things come in two or more back to back privilege manipulation sequences - each building on the last. I think we'd end up needing to manage a queue of pending changes per thread for that to have a hope. Also, there are some strange kernel ABI issues with stracing and other things when the privilege of a process' threads get out of sync.

I don't much like the complexity of augmenting a signal but it is unclear to me how else to break out of the syscall.

First, going to develop a simpler test case. Then I'll explore ways to address this. We can see how minimal a fix develops. Fortunately, there is a work around for this case. Hopefully, that can be adapted for any others that emerge while investigating...

@AndrewGMorgan
Copy link
Contributor

Here is a minimal reproducer. No privilege needed, no non-standard packages. It needs to be compiled CGO_ENABLED=0 (since syscall.AllThreadsSyscall() always fails when compiled CGO_ENABLED=1):

package main

import (
	"log"
	"os"
	"syscall"
	"time"
)

const prSetKeepCaps = 8

func main() {
	r, w, err := os.Pipe()
	if err != nil {
		log.Fatalf("failed to obtain pipe: %v", err)
	}
	data := make([]byte, 2+r.Fd())
	go r.Read(data)
	time.Sleep(500 * time.Millisecond)
	syscall.AllThreadsSyscall(syscall.SYS_PRCTL, prSetKeepCaps, 1, 0)
	w.Close()
	r.Close()
}

@AndrewGMorgan
Copy link
Contributor

This change to go (at HEAD) makes that minimal test run without deadlocking:

$ git diff --ignore-space-change
diff --git a/src/runtime/proc.go b/src/runtime/proc.go
index 7509f7632f..eb40ed3af4 100644
--- a/src/runtime/proc.go
+++ b/src/runtime/proc.go
@@ -1758,6 +1758,11 @@ func syscall_runtime_doAllThreadsSyscall(fn func(bool) bool) {
                                // next real wakeup will occur after
                                // startTheWorldGC() is called.
                                notewakeup(&mp.park)
+                               if mp.curg != nil {
+                                       // Thread blocked in syscall?
+                                       // (See golang.org/issue/50113.)
+                                       preemptM(mp)
+                               }
                        }
                        unlock(&mp.mFixup.lock)
                }
diff --git a/src/runtime/signal_unix.go b/src/runtime/signal_unix.go
index 08f266cc67..4f6d6652f3 100644
--- a/src/runtime/signal_unix.go
+++ b/src/runtime/signal_unix.go
@@ -616,13 +616,20 @@ func sighandler(sig uint32, info *siginfo, ctxt unsafe.Pointer, gp *g) {
                return
        }
 
-       if sig == sigPreempt && debug.asyncpreemptoff == 0 {
+       if sig == sigPreempt {
+               if mDoFixup() {
+                       // Preempted to unblock thread out of syscall
+                       // from syscall_runtime_doAllThreadsSyscall().
+                       return
+               }
+               if debug.asyncpreemptoff == 0 {
                        // Might be a preemption signal.
                        doSigPreempt(gp, c)
                        // Even if this was definitely a preemption signal, it
                        // may have been coalesced with another signal, so we
                        // still let it through to the application.
                }
+       }
 
        flags := int32(_SigThrow)
        if sig < uint32(len(sigtable)) {

@AndrewGMorgan
Copy link
Contributor

The original code runs for a while with the above patch to go, but eventually crashes. I'm still investigating.

@prattmic
Copy link
Member

I think we do want something like the patch in #50113 (comment). The ability for a blocking system call anywhere in the program to block execution is AllThreadsSyscall is a quite painful limitation, particularly since most users won't have the full-program awareness to know if there program might ever execute a blocking system call.

(Some system calls may not even be expected to block indefinitely, but expect to be woken by an action on another thread. But since we have stopped the world, that action never occurs and this thread remains blocked forever, causing a deadlock).

cc @aclements

@prattmic prattmic added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 27, 2022
@prattmic
Copy link
Member

A couple more notes:

  1. I think the potential for AllThreadsSyscall and its users (e.g., Setgroups) to hang indefinitely warrants not only a fix, but also a backport to previous releases.
  2. The patch in syscall: Setgroups may hang in go 1.16+ when not using cgo #50113 (comment) seems like a reasonable approach for a fix, though as @AndrewGMorgan notes it doesn't quite work. I haven't dug into it much, though I suspect we don't want to use preemptM itself, as that has extra synchronization with preemption that we don't care about. Perhaps just signalM directly?
  3. This is too large for a backport, but for 1.19 I think we should consider switching the current approach with one that depends entirely on signaling every thread and running the fixup in the signal handler. Especially with this fix where we might end up sending signals anyways, it seems like this could significantly simplify the implementation.

@prattmic prattmic modified the milestones: Go1.19, Go1.17.7 Jan 27, 2022
@gopherbot
Copy link

Change https://golang.org/cl/381534 mentions this issue: syscall: Fix deadlock for syscall.AllThreadsSyscall()

@AndrewGMorgan
Copy link
Contributor

Re point 3 of #50113 (comment) there is a corner case that I can't quite get my head around. Namely, when the code is racing thread creation. That is, when the mp entry is waiting for its procid to be filled in, there is nothing to direct the signal at. Currently, we call the fixup just after mp.procid is set. So, peppering explicit fixup calls is sort of needed for cases like this.

@AndrewGMorgan
Copy link
Contributor

For the above cl, I've included a slightly more stressful variant of the minimal test case #50113 (comment) . I've also validated that it the deadlocking workload pointed to at the top of this bug (https://github.com/weixiao-huang/golang-setgroups-hang) also works without deadlock when this change is applied.

@aclements
Copy link
Member

@gopherbot, please backport to Go 1.17

@aclements
Copy link
Member

@gopherbot, please backport to Go 1.16

@gopherbot
Copy link

Backport issue(s) opened: #50976 (for 1.16).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@aclements
Copy link
Member

@gopherbot, please backport to Go 1.17

@gopherbot
Copy link

Change https://golang.org/cl/383434 mentions this issue: syscall, runtime: reimplement AllThreadsSyscall using only signals.

@cherrymui
Copy link
Member

1.17 backport issue #51077

@gopherbot
Copy link

Change https://go.dev/cl/383996 mentions this issue: runtime: move doAllThreadsSyscall to os_linux.go

@gopherbot
Copy link

Change https://go.dev/cl/383999 mentions this issue: runtime/internal/syscall: new package for linux

gopherbot pushed a commit that referenced this issue Feb 15, 2022
syscall_runtime_doAllThreadsSyscall is only used on Linux. In
preparation of a follow-up CL that will modify the function to use other
Linux-only functions, move it to os_linux.go with no changes.

For #50113.

Change-Id: I348b6130038603aa0a917be1f1debbca5a5a073f
Reviewed-on: https://go-review.googlesource.com/c/go/+/383996
Trust: Michael Pratt <mpratt@google.com>
Reviewed-by: Andrew G. Morgan <agm@google.com>
Reviewed-by: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Feb 15, 2022
Add a generic syscall package for use by the runtime. Eventually we'd
like to clean up system calls in the runtime to use more code generation
and be moved out of the main runtime package.

The implementations of the assembly functions are based on copies of
syscall.RawSyscall6, modified slightly for more consistency between
arches. e.g., renamed trap to num, always set syscall num register
first.

For now, this package is just the bare minimum needed for
doAllThreadsSyscall to make an arbitrary syscall.

For #51087.
For #50113.

Change-Id: Ibecb5e6303279ce15286759e1cd6a2ddc52f7c72
Reviewed-on: https://go-review.googlesource.com/c/go/+/383999
Trust: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
@AndrewGMorgan
Copy link
Contributor

We ended up with:

The Go team has opted to abandon the current implementation (leaving this present bug in 1.16, 1.17), and adopt the substantial rewrite in 1.18.

For this present issue, for earlier releases, the following options are available:

  • a user code workaround (related to not using .Fd())
  • to compile CGO_ENABLED=1
  • for posterity, a workaround patch for go 1.16 and 1.17 is attached here: c871275.diff.zip.

@prattmic prattmic self-assigned this Feb 17, 2022
thekevinday pushed a commit to thekevinday/kernel.org-libcap that referenced this issue Apr 23, 2022
The CGO_ENABLED=0 failure mode is discussed in:

  golang/go#50113

At the present time, this only passes when the psx package is compiled
CGO_ENABLED=1. The problem being that a blocking read cannot be
interrupted by the CGO_ENABLED=0 build of package "psx". It does not
deadlock when compiled CGO_ENABLED=1 because the psx signal wakes the
reading thread up back into user space.

Signed-off-by: Andrew G. Morgan <morgan@kernel.org>
@prattmic prattmic self-assigned this Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

10 participants