What used to be godoc.org is now pkg.go.dev, and while the old URLs
still work, they might be broken in the future.
Updated badges are generated via https://pkg.go.dev/badge/
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Fix function docs. In particular, remove the part
which is not true ("verifies that the user isn't trying to set up any
mounts they don't have the rights to do"), and fix the part that
says "that doesn't resolve to root" (which is no longer true since
commit d8b669400a).
2. Replace fmt.Sscanf (which is slow and does lots of allocations)
with strings.TrimPrefix and strconv.Atoi.
3. Add a benchmark for rootlessEUIDMount. Comparing the old and the new
implementations:
name old time/op new time/op delta
RootlessEUIDMount-4 1.01µs ± 2% 0.16µs ± 1% -84.15% (p=0.008 n=5+5)
name old alloc/op new alloc/op delta
RootlessEUIDMount-4 224B ± 0% 80B ± 0% -64.29% (p=0.008 n=5+5)
name old allocs/op new allocs/op delta
RootlessEUIDMount-4 7.00 ± 0% 1.00 ± 0% -85.71% (p=0.008 n=5+5)
Note this code is already tested (in rootless_test.go).
Fixes: d8b669400a
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Apparently, "systemctl --user --no-pager show-environment" is useless
without DBUS_SESSION_BUS_ADDRESS or XDG_RUNTIME_DIR set:
$ echo $DBUS_SESSION_BUS_ADDRESS, $XDG_RUNTIME_DIR
unix:path=/run/user/1000/bus, /run/user/1000
$ systemctl --user --no-pager show-environment | grep DBUS_SESS
DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1000/bus
$ unset DBUS_SESSION_BUS_ADDRESS
$ systemctl --user --no-pager show-environment | grep DBUS_SESS
DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1000/bus
$ unset XDG_RUNTIME_DIR
$ systemctl --user --no-pager show-environment | grep DBUS_SESS
Failed to connect to bus: $DBUS_SESSION_BUS_ADDRESS and $XDG_RUNTIME_DIR not defined (consider using --machine=<user>@.host --user to connect to bus of other user)
So, it does not make sense to try it to get the address.
Also, it does not make sense to suggest "systemctl --user start dbus"
either, for the same reason, so remove that suggestion from the error
message text.
Since DBUS_SESSION_BUS_ADDRESS environment variable, on which the code
relies, is et by dbus-run-session (or dbus-launch, or something similar
that is supposed to be run during the login process), add a suggestion
to re-login.
Finally, fix the following linter warning:
> error-strings: error strings should not be capitalized or end with punctuation or a newline (revive)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Apparently, not all files listed in /sys/kernel/cgroup/delegate must
exist in every cgroup, so we should ignore ENOENT.
Dot not ignore ENOENT on the directory itself though.
Change cgroupFilesToChown to not return ".", and refactor it to not do
any dynamic slice appending in case we're using the default built-in
list of files.
Fixes: 35d20c4e0
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Don't require CAT or MBA because we don't detect those correctly (we
don't support L2 or L3DATA/L3CODE for example, and in the future
possibly even more). With plain "ClosId mode" we don't really care: we
assign the container to a pre-configured CLOS without trying to do
anything smarter.
Moreover, this was a duplicate/redundant check anyway, as for CAT and
MBA there is another specific sanity check that is done if L3 or MB
is specified in the config.
Signed-off-by: Markus Lehtonen <markus.lehtonen@intel.com>
Remove intelrtd.Manager interface, since we only have a single
implementation, and do not expect another one.
Rename intelRdtManager to Manager, and modify its users accordingly.
Remove NewIntelRdtManager from factory.
Remove IntelRdtfs. Instead, make intelrdt.NewManager return nil if the
feature is not available.
Remove TestFactoryNewIntelRdt as it is now identical to TestFactoryNew.
Add internal function newManager to be used for tests (to make sure
some testing is done even when the feature is not available in
kernel/hardware).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
TestGetContainerStats test a function that is smaller than the test
itself, and only calls a couple of other functions (which are
represented by mocks). It does not make sense to have it.
mockIntelRdtManager is only needed for TestGetContainerStats
and TestGetContainerState, which basically tests that Path
is called. Also, it does not make much sense to have it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since we are looking up the path to newuidmap/newgidmap in one context,
and executing those in another (libct/nsenter), it might make sense to
use a stricter rules for looking up path to those binaries.
Practically it means that if someone wants to use custom newuidmap and
newgidmap binaries from $PATH, it would be impossible to use these from
the current directory by means of PATH=.:$PATH; instead one would have
to do something like PATH=$(pwd):$PATH.
See https://go.dev/blog/path-security for background.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
These were introduced in commit d8b669400 back in 2017, with a TODO
of "make binary names configurable". Apparently, everyone is happy with
the hardcoded names. In fact, they *are* configurable (by prepending the
PATH with a directory containing own version of newuidmap/newgidmap).
Now, these binaries are only needed in a few specific cases (when
rootless is set etc.), so let's look them up only when needed.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
We only have one implementation of config validator, which is always
used. It makes no sense to have Validator interface.
Having validate.Validator field in Factory does not make sense for all
the same reasons.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Those are *always* /proc/self/exe init, and it does not make sense
to ever change these. More to say, if InitArgs option func (removed
by this commit) is used to change these parameters, it will break
things, since "init" is hardcoded elsewhere.
Remove this.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 029b73c1b replaced a regular expression with code checking the
characters. Despite what the comment said about ASCII, the check was
performed rune by rune, not byte by byte.
Note the check was still correct, basically comparing int32's, but the
byte by byte way is a tad faster and more straightforward. The change
also fixes the issue of a misleading comment.
Benchmark before/after shows a modest improvement:
name old time/op new time/op delta
CheckPropertyName-4 164ns ± 2% 123ns ± 2% -24.73% (p=0.029 n=4+4)
name old alloc/op new alloc/op delta
CheckPropertyName-4 96.0B ± 0% 64.0B ± 0% -33.33% (p=0.029 n=4+4)
name old allocs/op new allocs/op delta
CheckPropertyName-4 6.00 ± 0% 4.00 ± 0% -33.33% (p=0.029 n=4+4)
Fixes: 029b73c1b
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 643f8a2b40 renamed isValidName to checkPropertyName, but fell
short of renaming its test and benchmark. Fix that.
Fixes: 643f8a2b40
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
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>
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>
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>
Kir Kolyshkin