After adding some debug info to cloned_binary.c I found out that
is_self_cloned() is not working right when runc binary is on tmpfs,
resulting in one extra re-exec of runc.
With some added debug:
$ mkdir bin
$ sudo mount -t tmpfs tmp bin
$ sudo cp runc bin
$ sudo ./bin/runc --debug exec xxx true
DEBU[0000] nsexec[763590]: => is_self_cloned
DEBU[0000] nsexec[763590]: got seals 1 (want 15)
DEBU[0000] nsexec[763590]: <= is_self_cloned, is_cloned = 0
DEBU[0000] nsexec[763590]: try_bindfd: 5
DEBU[0000] nsexec[763590]: re-exec itself...
DEBU[0000] nsexec[763590]: => is_self_cloned
DEBU[0000] nsexec[763590]: got seals 1 (want 15)
DEBU[0000] nsexec[763590]: <= is_self_cloned, is_cloned = 0
DEBU[0000] nsexec[763590]: try_bindfd: -1
DEBU[0000] nsexec[763590]: fallback to make_execfd: 5
DEBU[0000] nsexec[763590]: re-exec itself...
DEBU[0000] nsexec[763590]: => is_self_cloned
DEBU[0000] nsexec[763590]: got seals 15 (want 15)
DEBU[0000] nsexec[763590]: <= is_self_cloned, is_cloned = 1
From the above, it is seen that
- `is_self_cloned` returns 0,
- `try_bindfd` is called and succeeds,
- runc re-execs itself,
- the second call to `is_self_cloned` returns 0 again (because GET_SEALS returns 1),
- runc falls back to `make_execfd`, and re-execs again,
- finally, the third `is_self_cloned` returns 1.
I guess that the code relied on the following (quoting fcntl(2)):
> Currently, file seals can be applied only to a file descriptor
> returned by memfd_create(2) (if the MFD_ALLOW_SEALING was employed).
> On other filesystems, all fcntl() operations that operate on seals
> will return EINVAL.
It looks like in case of a file on tmpfs it returns 1 (F_SEAL_SEAL).
With the fix:
DEBU[0000] nsexec[768367]: => is_self_cloned
DEBU[0000] nsexec[768367]: got seals 1 (want 15)
DEBU[0000] nsexec[768367]: no CLONED_BINARY_ENV
DEBU[0000] nsexec[768367]: <= is_self_cloned, is_cloned = 0
DEBU[0000] nsexec[768367]: try_bindfd: 5
DEBU[0000] nsexec[768367]: re-exec itself...
DEBU[0000] nsexec[768367]: => is_self_cloned
DEBU[0000] nsexec[768367]: got seals 1 (want 15)
DEBU[0000] nsexec[768367]: fstatfs says ro = 1
DEBU[0000] nsexec[768367]: fstat says nlink = 1
DEBU[0000] nsexec[768367]: <= is_self_cloned, is_cloned = 1
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This was introduced in an initial commit, back in the day when criu was
a highly experimental thing. Today it's not; most users who need it have
it packaged by their distro vendor.
The usual way to run a binary is to look it up in directories listed in
$PATH. This is flexible enough and allows for multiple scenarios (custom
binaries, extra binaries, etc.). This is the way criu should be run.
Make --criu a hidden option (thus removing it from help). Remove the
option from man pages, integration tests, etc. Remove all traces of
CriuPath from data structures.
Add a warning that --criu is ignored and will be removed.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is needed since the future commits will touch this code, and then
the lint-extra CI job complains.
> libcontainer/factory.go#L245
> var-naming: var fdsJson should be fdsJSON (revive)
and
> libcontainer/init_linux.go#L181
> error-strings: error strings should not be capitalized or end with punctuation or a newline (revive)
and
> notify_socket.go#L94
> receiver-naming: receiver name n should be consistent with previous receiver name s for notifySocket (revive)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since the next commit is going to touch this structure, our CI
(lint-extra) is about to complain about improperly named field:
> Warning: var-naming: struct field ContainerId should be ContainerID (revive)
Make it happy.
Brought to use by gopls rename.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This method was removed earlier by commit 097c6d7425,
but the documentation was not updated. Fix it.
Fixes: 097c6d7425
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Those were added by commit 59c5c3ac0 back in Apr 2015, but AFAICS were
never used and are obsoleted by more generic container hooks (initially
added by commit 05567f2c94 in Sep 2015).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The utils.Annotations was used here before only because it made it
possible to distinguish between "key not found" and "empty value" cases.
With the previous commit, utils.SearchLabels can do that, and so it
makes sense to use it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Using strings.Split generates temporary strings for GC to collect.
Rewrite the function to not do that.
Also, add a second return value, so that the caller can distinguish
between an empty value found and no key found cases.
Fix the test accordingly.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since commit 551629417 we can (and should) use Info() to get access to
file stat. Do this.
While going over directory entries, a parallel runc delete can remove
an entry, and with the current code it results in a fatal error (which
was not observed in practice, but looks quite possible). To fix,
add a special case to continue on ErrNotExist.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In case of a read-only /dev, it's better to move on and let whatever is
run in a container to handle any possible errors.
This solves runc exec for a user with read-only /dev.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since we already called fstat, we know the current file uid. In case it
is the same as the one we want it to be, there's no point in trying
chown.
Remove the specific /dev/null check, as the above also covers it
(comparing /dev/null uid with itself is true).
This also fixes runc exec with read-only /dev for root user.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Use os/file Chown method instead of bare unix.Fchown as it already have
access to underlying fd, and produces nice-looking errors. This allows
us to remove our error wrapping and some linter annotations.
We still use unix.Fstat since os.Stat access to os-specific fields
like uid/gid is not very straightforward. The only change here is to use
file name (rather than fd) in the error text.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
For the Nth time I wanted to replace parsing mountinfo with
statfs and the check for superblock magic, but it is not possible
since some code relies of mount options check which can only
be obtained via mountinfo.
Add a note about it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 4d1d6185ab added this
nsenter_unsupported.go file in order for nsenter to be a valid (but
empty, non-functional) Go package on unsupported platforms.
As a result, runc can be build successfully without CGO, which results
in a non-working and hard-to-debug binary (see issue 3330).
As the functionality of being able to compile a package which is
definitely not working is questionable, and I can't think of any use
cases, let's remove the file.
With this, runc can no longer be build without CGO:
[kir@kir-rhat runc]$ CGO_ENABLED=0 make runc
go build -trimpath "-buildmode=pie" -tags "seccomp" -ldflags "-X main.gitCommit=v1.0.0-452-g00f56786-dirty -X main.version=1.1.0-rc.1+dev " -o runc .
go build github.com/opencontainers/runc/libcontainer/nsenter: build constraints exclude all Go files in /home/kir/go/src/github.com/opencontainers/runc/libcontainer/nsenter
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(on Go 1.18 this is actually an error)
> libcontainer/factory_linux.go:341:10: fmt.Errorf format %w has arg e of wrong type interface{}
Unfortunately, fixing it results in an errorlint warning:
> libcontainer/factory_linux.go#L344 non-wrapping format verb for fmt.Errorf. Use `%w` to format errors (errorlint)
so we have to silence that one.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since commit 12e99a0f8d we do require Go >= 1.16, so this file
is no longer needed.
Also, this actually ensures that go >= 1.16 is used (otherwise
libcontainer/cgroups/getallpids.go won't compile).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This will make releases much simpler. I've back-filled the changelog
with everything since runc 1.0.0 (there's not much point going further
back than that).
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Commit 7cfb107f2c started using unix.Geteuid(), unix.Getegid() as
uid/gid argument for chown. It seems that it should have removed chown
entirely, since, according to mkdir(2),
> The newly created directory will be owned by the effective user ID of
> the process. If the directory containing the file has the
> set-group-ID bit set, or if the filesystem is mounted with BSD
> group semantics (mount -o bsdgroups or, synonymously mount -o grpid),
> the new directory will inherit the group ownership from its parent;
> otherwise it will be > owned by the effective group ID of the process.
So, the only effect of the chown after mkdir is ignoring the sgid bit
on the parent directory (which is probably not the right thing to do).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This check was always broken, and it slipped through the cracks because
we never run it without additional architectures now.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Kir Kolyshkin