RFE: seccomp userspace notification (follow-up to #50)

[alban]

This PR is based on @rodnymolina's work from #50 and adds the following:

• Travis: compile with libseccomp-2.5.0 #58 to update Travis tests to libseccomp 2.5.0
• Address reviews
• Fix tests

cc @giuseppe

[giuseppe]

thanks for addressing all the comments. LGTM

[alban]

@pcmoore @drakenclimber should the commits be squashed into logical commits or would you do squash in a single commit when merging?

@rodnymolina @ctalledo some of the initial commits from #50 don't have the "Signed-off-by" line. Are you ok with adding it?

[rodnymolina]

@alban, thanks for picking this one up and addressing all the review comments, haven't had cycles to do it myself. Do you want us to add the signed-off to our original commits and re-submit them again? Or there's another (quicker) way to do this?

[alban]

I could just edit your commits in this PR with the following if you give your ok

Signed-off-by: Rodny Molina <rmolina@nestybox.com>

[rodnymolina]

Oh sure, please go ahead. Thanks!

[alban]

I forced-pushed the branch with the following changes:

• Add Signed-off-by tag on commitmsgs in agreement with Rodny and Cesar
• Add compatibility with old libseccomp (<2.5.0). This is done using the existing mechanism with #if SCMP_VER_* in seccomp_internal.go. In order to keep seccomp.go simple without additional cgo code, the new public API about notify added in seccomp.go is now a simple wrapper to internal functions in seccomp_internal.go.
• Travis CI: add a matrix to check that it works with 3 different versions of seccomp, back to 2.2.1
• Unit tests: skip TestNotif on old seccomp and add new test TestNotifUnsupported to check that error management is done correctly on old seccomp.
• Fix spelling of SCMP_ACT_NOTIFY (remove SCMP_ACT_USER_NOTIF)

With the changes above, I am able to use this libseccomp-golang branch successfully in runc (tested with Travis in CentOS 7, Ubuntu Bionic, Fedora 32).

[yvesf]

Documentation of UnlockOSThread says:

// Before calling UnlockOSThread, the caller must ensure that the OS
// thread is suitable for running other goroutines. If the caller made
// any permanent changes to the state of the thread that would affect
// other goroutines, it should not call this function and thus leave
// the goroutine locked to the OS thread until the goroutine (and
// hence the thread) exits.

Since we don't unload this filter (because it's not possible), I assume it would be correct to not call UnlockOSThread() here. Otherwise it could happen that by random choice this thread is re-used. In this test this seems improable but if it's true then it would be good to give the right example here. I'm not an expert and just reasoning from the docs, I have no proof of this.

[ctalledo]

Thanks, that's a good catch. I had missed that description of UnlockOSThread(), and I agree with your reasoning that we should not unlock the goroutine from the OS thread given that the seccomp filter is already acting on it and can't be unloaded.

[alban]

@yvesf Thanks for testing. I have the same error when trying -count 2 like you did. I tried to remove the call to UnlockOSThread but it didn't help.

I observed the following with the command:

strace -f -s 256 -e chdir,seccomp go test -v . -count 2 -v -run 'TestNotif$'

• First iteration of the test:

[pid 1901155] seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_NEW_LISTENER, {len=9, filter=0x1a70580}) = 6
[pid 1901155] chdir("/non-existent-path"    <unfinished ...>
[pid 1901155] <... chdir resumed>)      = -1 ENOENT (No such file or directory)

-> seccomp() returns the file descriptor correctly
-> chdir() returned ENOENT as expected

• Second iteration of the test:

[pid 1901157] seccomp(SECCOMP_SET_MODE_FILTER, 0, {len=9, filter=0x7f6948001b50}) = 0
[pid 1901157] chdir("/non-existent-path") = -1 ENOSYS (Function not implemented)

-> The pid is different, so removing UnlockOSThread worked
-> seccomp() is not passed the flag SECCOMP_FILTER_FLAG_NEW_LISTENER as expected (why?)
-> seccomp() fails to return the file descriptor
-> chdir() does not return ENOENT but ENOSYS.

I am continuing to investigate...

[yvesf]

Maybe someone else could verify if this affects only me:
When I run the test for notifications more than once: go test -v . -count 2 -v -run 'TestNotif$'
... it fails starting from the second run. Almost like the filter is not loaded. The notify-fd even returns "bad filedescriptor" when I try to close it. From the code it's not clear to me why it could fail silently like this.
(running debian buster with seccomp libs 2.5.0-1 from experimental in docker within kernel 5.8.13-zen1 on nixos 20.09).

[alban]

@yvesf I found the root cause: this is because sys_filter_load() (see code) uses global variable (state.notify_fd) to remember if a seccomp fd has already been set. There is a function exposed in the API to reset that global variable:

From seccomp_reset(3):

If seccomp_reset() is called with a NULL filter, it resets the library's global task state, including any notification file descriptors
retrieved by seccomp_notify_fd(3). Normally this is not needed, but it may be required to continue using the library after a fork() or clone() call to ensure the API level and user notification state is properly reset.

I could fix the issue in libseccomp-golang with the following patch:

diff --git a/seccomp.go b/seccomp.go
index 2c12662..68e7690 100644
--- a/seccomp.go
+++ b/seccomp.go
@@ -682,6 +682,14 @@ func (f *ScmpFilter) Reset(defaultAction ScmpAction) error {
        return nil
 }
 
+func GlobalReset() error {
+       if retCode := C.seccomp_reset(nil, 0); retCode != 0 {
+               return errRc(retCode)
+       }
+
+       return nil
+}
+
 // Release releases a filter context, freeing its memory. Should be called after
 // loading into the kernel, when the filter is no longer needed.
 // After calling this function, the given filter is no longer valid and cannot
diff --git a/seccomp_test.go b/seccomp_test.go
index 2b82835..5ca9781 100644
--- a/seccomp_test.go
+++ b/seccomp_test.go
@@ -743,6 +743,8 @@ func notifHandler(ch chan error, fd ScmpFd, tests []notifTest) {
 }
 
 func TestNotif(t *testing.T) {
+       GlobalReset()
+
        // seccomp notification requires API level >= 5
        api, err := GetAPI()
        if err != nil {
@@ -857,7 +859,9 @@ func TestNotif(t *testing.T) {
                // Lock this goroutine to it's current kernel thread; otherwise the go runtime may
                // switch us to a different OS thread, bypassing the seccomp notification filter.
                runtime.LockOSThread()
-               defer runtime.UnlockOSThread()
+
+               // Don't call runtime.UnlockOSThread() to ensure that the thread with the
+               // seccomp filter applied will not be reused in another test.
 
                err = filter.Load()
                if err != nil {

I think libseccomp API is not ideal with this global state. Although it can make sense in single-threaded programs, it could cause difficulties if a multi-threaded program ever wants to setup different filters for different threads. I would like if libseccomp API could be improved, but in order for libseccomp-golang to support libseccomp-2.5.0, I would add this GlobalReset() in the Go API. What do you think?

[alban]

Even with the fix from above, we can't run some tests with filter.SetTsync(false) (TestNotif) and some tests with filter.SetTsync(true) (TestLogAct) because once the threads are not using the same seccomp filters anymore, the kernel will refuse to use SECCOMP_FILTER_FLAG_TSYNC:

If any thread cannot synchronize to the same filter tree, the call will not attach the new seccomp filter, and will fail

This can reproduce with:

go test -v . -count 2 -v -run 'TestLogAct|TestNotif$'

For this, the tests using tsync and the tests not using tsync should be executed separately (in different processes).

[yvesf]

Even with the fix from above, we can't run some tests with filter.SetTsync(false) (TestNotif) and some tests with filter.SetTsync(true) (TestLogAct) because once the threads are not using the same seccomp filters anymore
...
For this, the tests using tsync and the tests not using tsync should be executed separately (in different processes).

Not sure if it's too much of a hack but we could execute the test in subprocesses, go's syscall/syscall_linux_test.go does something similar.

func TestLogAct(t *testing.T) {
	execInSubprocess(t, func(t *testing.T) {
		expectedPid := syscall.Getpid()

execInSubprocess(t, func)

// execInSubprocess calls the go test binary again for the same test.
// This must be only top-level statment in the test function. Do not nest this.
// It will slightly defect the test log output as the test is entered twice
func execInSubprocess(t *testing.T, f func(t *testing.T)) {
	const subprocessEnvKey = `GO_SUBPROCESS_KEY`
	if testIDString, ok := os.LookupEnv(subprocessEnvKey); ok && testIDString == "1" {
		t.Run(`subprocess`, f)
		return
	}

	cmd := exec.Command(os.Args[0])
	cmd.Args = []string{os.Args[0], "-test.run=" + t.Name() + "$", "-test.v=true"}
	for _, arg := range os.Args {
		if strings.HasPrefix(arg, `-test.testlogfile=`) {
			cmd.Args = append(cmd.Args, arg)
		}
	}
	cmd.Env = []string{subprocessEnvKey + "=1"}
	cmd.Stdin = os.Stdin
	cmd.Stdout = os.Stdout
	cmd.Stderr = os.Stderr

	err := cmd.Start()
	if err != nil {
		t.Error("failed to spawn test in sub-process", err)
		t.FailNow()
	}

	err = cmd.Wait()
	if err != nil {
		if err, ok := err.(*exec.ExitError); ok {
			t.Errorf("Test failed with status %d", err.ExitCode())
		}
		t.Error(`test failed`)
		t.FailNow()
	}
}

[alban]

@yvesf Thanks, it works for me! I pushed a new commit using your execInSubprocess function. I changed stdout / stderr so that the recursive output is aligned more nicely. I didn't find the same code pattern in syscall/syscall_linux_test.go though. Do I reference the code correctly in the commitmsg?

[yvesf]

I didn't find the same code pattern in syscall/syscall_linux_test.go though

It's not the same pattern, just similar. TestLinuxDeathSignal for instance calls itself with GO_DEATHSIG_PARENT env variable set. In the TestMain this env variable is detected and execution redirected to deathSignalChild().

[alban]

Gentle reminder about this PR. Thanks!

[pcmoore]

Wow, 27 patches ... that's something. Especially considering I think you could count the associated main library patches to do enable userspace notification on one hand :)

I'm not sure yet if it is the case with this PR, but for future submissions please take the approach of simply updating flawed patches in the PR if the PR is still open and not merged. I generally dislike merging commits which are known to be broken; it both makes bisection difficult and the git log overly noisy.

All that said, I'll start taking a look at this now, but given the number of patches it make take some time.

[ctalledo]

Yes, thanks to all that made the merge possible!

[AkihiroSuda]

Can we have v0.9.2 with this soon?

[rata]

@AkihiroSuda maybe you can create an issue for this? I see the 0.9.2 milestone exists and has more things to do, but will be great if a new release is out and other stuff are moved to 0.9.3 :)

[pcmoore]

Two quick comments:

1. Please don't comment on closed issues unless your comment directly relates to the issue. The comments above do not in my opinion relate to the original issue.
2. The obvious answer to helping get v0.9.2 released quicker is to help resolve the outstanding issues in the milestone; deferring issues to a later release is strongly discouraged and asking for that it not going to be received well by @drakenclimber or myself.