From 18e286261ec6e94bb9ccaee2bbc91fc63bf08f89 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 19 Jan 2022 16:05:30 -0800 Subject: [PATCH] libct/nsenter: fix extra runc re-exec on tmpfs 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 --- CHANGELOG.md | 5 +++++ libcontainer/nsenter/cloned_binary.c | 8 +++----- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cffc9e2f6..8f58dbee4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * When Intel RDT feature is not available, its initialization is skipped, resulting in slightly faster `runc exec` and `runc run`. (#3306) +### Fixed + + * In case the runc binary resides on tmpfs, `runc init` no longer re-execs + itself twice. (#3342) + ## [1.1.0] - 2022-01-14 > A plan depends as much upon execution as it does upon concept. diff --git a/libcontainer/nsenter/cloned_binary.c b/libcontainer/nsenter/cloned_binary.c index 4268ebda9..73c0a27b5 100644 --- a/libcontainer/nsenter/cloned_binary.c +++ b/libcontainer/nsenter/cloned_binary.c @@ -137,7 +137,7 @@ static void *must_realloc(void *ptr, size_t size) */ static int is_self_cloned(void) { - int fd, ret, is_cloned = 0; + int fd, is_cloned = 0; struct stat statbuf = { }; struct statfs fsbuf = { }; @@ -153,11 +153,9 @@ static int is_self_cloned(void) * sharing it isn't a bad thing -- and an admin could bind-mount a sealed * memfd to /usr/bin/runc to allow re-use). */ - ret = fcntl(fd, F_GET_SEALS); - if (ret >= 0) { - is_cloned = (ret == RUNC_MEMFD_SEALS); + is_cloned = (fcntl(fd, F_GET_SEALS) == RUNC_MEMFD_SEALS); + if (is_cloned) goto out; - } /* * All other forms require CLONED_BINARY_ENV, since they are potentially