Merge pull request #227 from kolyshkin/fix-flake

Improve SetKeyCreate error reporting, fix test flakes

Author: rhatdan

.github/workflows/validate.yml

@@ -140,6 +140,12 @@ jobs:
         continue-on-error: true
         run: lima make -C /tmp/selinux GOARCH=386 test
 
+      # https://github.com/opencontainers/selinux/issues/222
+      # https://github.com/opencontainers/selinux/issues/225
+      - name: "racy test"
+        continue-on-error: true
+        run: lima bash -c 'cd /tmp/selinux && go test -timeout 10m -count 100000 ./go-selinux'
+
       - name: "Show AVC denials"
         run: lima sudo ausearch -m AVC,USER_AVC || true
 

go-selinux/selinux.go

@@ -41,6 +41,10 @@ var (
 	// ErrVerifierNil is returned when a context verifier function is nil.
 	ErrVerifierNil = errors.New("verifier function is nil")
 
+	// ErrNotTGLeader is returned by [SetKeyLabel] if the calling thread
+	// is not the thread group leader.
+	ErrNotTGLeader = errors.New("calling thread is not the thread group leader")
+
 	// CategoryRange allows the upper bound on the category range to be adjusted
 	CategoryRange = DefaultCategoryRange
 
@@ -180,10 +184,14 @@ func PeerLabel(fd uintptr) (string, error) {
 }
 
 // SetKeyLabel takes a process label and tells the kernel to assign the
-// label to the next kernel keyring that gets created. Calls to SetKeyLabel
-// should be wrapped in runtime.LockOSThread()/runtime.UnlockOSThread() until
-// the kernel keyring is created to guarantee another goroutine does not migrate
-// to the current thread before execution is complete.
+// label to the next kernel keyring that gets created.
+//
+// Calls to SetKeyLabel should be wrapped in
+// runtime.LockOSThread()/runtime.UnlockOSThread() until the kernel keyring is
+// created to guarantee another goroutine does not migrate to the current
+// thread before execution is complete.
+//
+// Only the thread group leader can set key label.
 func SetKeyLabel(label string) error {
 	return setKeyLabel(label)
 }

go-selinux/selinux_linux.go

@@ -735,6 +735,9 @@ func setKeyLabel(label string) error {
 	if label == "" && errors.Is(err, os.ErrPermission) {
 		return nil
 	}
+	if errors.Is(err, unix.EACCES) && unix.Getuid() != unix.Gettid() {
+		return ErrNotTGLeader
+	}
 	return err
 }
 

go-selinux/selinux_linux_test.go

@@ -7,9 +7,12 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strconv"
 	"strings"
 	"testing"
+
+	"golang.org/x/sys/unix"
 )
 
 func TestSetFileLabel(t *testing.T) {
@@ -187,6 +190,12 @@ func TestSocketLabel(t *testing.T) {
 		t.Skip("SELinux not enabled, skipping.")
 	}
 
+	// Ensure the thread stays the same for duration of the test.
+	// Otherwise Go runtime can switch this to a different thread,
+	// which results in EACCES in call to SetSocketLabel.
+	runtime.LockOSThread()
+	defer runtime.UnlockOSThread()
+
 	label := "system_u:object_r:container_t:s0:c1,c2"
 	if err := SetSocketLabel(label); err != nil {
 		t.Fatal(err)
@@ -205,6 +214,16 @@ func TestKeyLabel(t *testing.T) {
 		t.Skip("SELinux not enabled, skipping.")
 	}
 
+	// Ensure the thread stays the same for duration of the test.
+	// Otherwise Go runtime can switch this to a different thread,
+	// which results in EACCES in call to SetKeyLabel.
+	runtime.LockOSThread()
+	defer runtime.UnlockOSThread()
+
+	if unix.Getpid() != unix.Gettid() {
+		t.Skip(ErrNotTGLeader)
+	}
+
 	label := "system_u:object_r:container_t:s0:c1,c2"
 	if err := SetKeyLabel(label); err != nil {
 		t.Fatal(err)
@@ -235,6 +254,12 @@ func TestSELinux(t *testing.T) {
 		t.Skip("SELinux not enabled, skipping.")
 	}
 
+	// Ensure the thread stays the same for duration of the test.
+	// Otherwise Go runtime can switch this to a different thread,
+	// which results in EACCES in call to SetFSCreateLabel.
+	runtime.LockOSThread()
+	defer runtime.UnlockOSThread()
+
 	var (
 		err            error
 		plabel, flabel string
@@ -259,21 +284,17 @@ func TestSELinux(t *testing.T) {
 	ReleaseLabel(plabel)
 
 	pid := os.Getpid()
-	t.Logf("PID:%d MCS:%s\n", pid, intToMcs(pid, 1023))
+	t.Logf("PID:%d MCS:%s", pid, intToMcs(pid, 1023))
 	err = SetFSCreateLabel("unconfined_u:unconfined_r:unconfined_t:s0")
-	if err == nil {
-		t.Log(FSCreateLabel())
-	} else {
-		t.Log("SetFSCreateLabel failed", err)
-		t.Fatal(err)
+	if err != nil {
+		t.Fatal("SetFSCreateLabel failed:", err)
 	}
+	t.Log(FSCreateLabel())
 	err = SetFSCreateLabel("")
-	if err == nil {
-		t.Log(FSCreateLabel())
-	} else {
-		t.Log("SetFSCreateLabel failed", err)
-		t.Fatal(err)
+	if err != nil {
+		t.Fatal("SetFSCreateLabel failed:", err)
 	}
+	t.Log(FSCreateLabel())
 	t.Log(PidLabel(1))
 }