libpod: leave thread locked on errors

[giuseppe]

if the SELinux label could not be restored correctly, leave the OS
thread locked so that it is terminated once it returns to the threads
pool.

[NO NEW TESTS NEEDED] the failure is hard to reproduce

Signed-off-by: Giuseppe Scrivano [email protected]

[adrianreber]

Sounds very reasonable to me. LGTM.

[flouthoc]

LGTM

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [giuseppe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[rhatdan]

I am not sure what this buys you, but there is other code in oci-conmon-linux that this should apply to also.
SetProcessLabel.

[giuseppe]

If we unlock the thread on errors then it keeps running with the wrong label and we don't what other tasks it will be running

[Luap99]

/lgtm

[rhatdan]

The second call is just setting back the default label, it does only effects any new sockets that get created. If the "" call fails, then Podman will continue creating sockets with the label of the previous container.
In both SELinux calls, we are setting the label for future sockets or future processes. Calling with "" tells the kernel to remove the transition rules, meaning sockets and fork/exec processes will get set with the current process label. (I'm simplifying it for clarity).

[rhatdan]

BTW the chance of "" failing and "...container_t..." succeeded is just about impossible.

[giuseppe]

The second call is just setting back the default label, it does only effects any new sockets that get created. If the "" call fails, then Podman will continue creating sockets with the label of the previous container.
In both SELinux calls, we are setting the label for future sockets or future processes. Calling with "" tells the kernel to remove the transition rules, meaning sockets and fork/exec processes will get set with the current process label. (I'm simplifying it for clarity).

if we fail to set it back to "" in CheckpointContainer, won't it be kept as the default label for the thread, so every new process created by that thread will inherit it (e.g., common, slirp4netns) and create sockets with that label? I think it would cause the same issues we've seen in #12388 since the thread will be scheduled to take care of other goroutines.

[rhatdan]

Sadly these are not thread settings, but process settings. So we only want to set them right before we exec and then setting it back to default right afterwards. we don't set the label confined label on slirp4netns or on conmon.

 $ ps -eZ | grep slirp
unconfined_u:system_r:container_runtime_t:s0-s0:c0.c1023 682337 pts/2 00:00:00 slirp4netns
$ ps -eZ | grep conmon
unconfined_u:system_r:container_runtime_t:s0 682340 ? 00:00:00 conmon
$ ps -eZ | grep top
system_u:system_r:container_t:s0:c121,c653 682349 ? 00:00:00 top

The labels are not set for the ociruntime either, the OCI runtimes set the label just for the container.
Other tools like conmon and slirp4netns and crun need a lot more privs then we give to the container.

[giuseppe]

Sadly these are not thread settings, but process settings.

if that is the case, then don't we need to fork+exec each time we want to set any of those? runtime.LockOSThread only prevents the current OS thread to be re-used by other goroutines while it is locked, but it doesn't prevent other threads from running.

How can we prevent other threads for using the wrong label while it is set?

we don't set the label confined label on slirp4netns or on conmon.

right. But if we fail to reset to the default label, then they will inherit the one we set.

[giuseppe]

Sadly these are not thread settings, but process settings.

one good news: selinux.Set*Label uses /proc/thread-self/attr instead of /proc/self and it seems to affect only the calling thread.

I've tested it with this program:

package main

import (
	"time"
	"runtime"
	"io/ioutil"

	"github.com/opencontainers/selinux/go-selinux/label"
)

func main() {
	runtime.LockOSThread()
	if err := label.SetFileCreateLabel("system_u:system_r:container_t:s0:c285,c463"); err != nil {
		panic(err)
	}

	if err := ioutil.WriteFile("file", []byte(""), 600); err != nil {
		panic(err)
	}

	go func() {
		ioutil.WriteFile("file-2", []byte(""), 600)		
	}()
	
	time.Sleep(time.Second)
}

and I get:

$ go build && ./test && ls -1Z file*
system_u:system_r:container_t:s0:c285,c463 file
       unconfined_u:object_r:user_tmp_t:s0 file-2

I can confirm it by reading all the /proc/$PID/task/$TID/attr/ files and they are different per-thread.

[rhatdan]

Great, I thought this was per process but having it per thread is much better.