From: Andrei Vagin <[email protected]>
* check that a child process is in parent's time namespace after vfork.
* check that a child process is in the target namespace after exec.
Output on success:
1..5
ok 1 parent before vfork
ok 2 child before exec
ok 3 child after exec
ok 4 wait for child
ok 5 parent after vfork
# Totals: pass:5 fail:0 xfail:0 xpass:0 skip:0 error:0
Signed-off-by: Andrei Vagin <[email protected]>
---
tools/testing/selftests/timens/.gitignore | 1 +
tools/testing/selftests/timens/Makefile | 2 +-
tools/testing/selftests/timens/vfork_exec.c | 132 ++++++++++++++++++++
3 files changed, 134 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/timens/vfork_exec.c
diff --git a/tools/testing/selftests/timens/.gitignore b/tools/testing/selftests/timens/.gitignore
index fe1eb8271b35..cae8dca0fbff 100644
--- a/tools/testing/selftests/timens/.gitignore
+++ b/tools/testing/selftests/timens/.gitignore
@@ -8,3 +8,4 @@ procfs
timens
timer
timerfd
+vfork_exec
diff --git a/tools/testing/selftests/timens/Makefile b/tools/testing/selftests/timens/Makefile
index 3a5936cc10ab..f0d51d4d2c87 100644
--- a/tools/testing/selftests/timens/Makefile
+++ b/tools/testing/selftests/timens/Makefile
@@ -1,4 +1,4 @@
-TEST_GEN_PROGS := timens timerfd timer clock_nanosleep procfs exec futex
+TEST_GEN_PROGS := timens timerfd timer clock_nanosleep procfs exec futex vfork_exec
TEST_GEN_PROGS_EXTENDED := gettime_perf
CFLAGS := -Wall -Werror -pthread
diff --git a/tools/testing/selftests/timens/vfork_exec.c b/tools/testing/selftests/timens/vfork_exec.c
new file mode 100644
index 000000000000..9fd8a64d25a9
--- /dev/null
+++ b/tools/testing/selftests/timens/vfork_exec.c
@@ -0,0 +1,132 @@
+// SPDX-License-Identifier: GPL-2.0
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <sched.h>
+#include <stdio.h>
+#include <stdbool.h>
+#include <sys/stat.h>
+#include <sys/syscall.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <time.h>
+#include <unistd.h>
+#include <string.h>
+#include <pthread.h>
+
+#include "log.h"
+#include "timens.h"
+
+#define OFFSET (36000)
+
+static void *tcheck(void *arg)
+{
+ struct timespec *now = arg, tst;
+ int i;
+
+ for (i = 0; i < 2; i++) {
+ _gettime(CLOCK_MONOTONIC, &tst, i);
+ if (abs(tst.tv_sec - now->tv_sec) > 5) {
+ pr_fail("thread: unexpected value: %ld (%ld)\n",
+ tst.tv_sec, now->tv_sec);
+ return (void *)1UL;
+ }
+ }
+ return NULL;
+}
+
+static int check_in_thread(struct timespec *now)
+{
+ pthread_t th;
+ void *retval;
+
+ if (pthread_create(&th, NULL, tcheck, now))
+ return pr_perror("thread");
+ if (pthread_join(th, &retval))
+ return pr_perror("pthread_join");
+ return !(retval == NULL);
+}
+
+static int check(char *tst_name, struct timespec *now)
+{
+ struct timespec tst;
+ int i;
+
+ for (i = 0; i < 2; i++) {
+ _gettime(CLOCK_MONOTONIC, &tst, i);
+ if (abs(tst.tv_sec - now->tv_sec) > 5)
+ return pr_fail("%s: unexpected value: %ld (%ld)\n",
+ tst.tv_sec, now->tv_sec);
+ }
+ if (check_in_thread(now))
+ return 1;
+ ksft_test_result_pass("%s\n", tst_name);
+ return 0;
+}
+
+int main(int argc, char *argv[])
+{
+ struct timespec now;
+ int status;
+ pid_t pid;
+
+ if (argc > 1) {
+ char *endptr;
+
+ ksft_cnt.ksft_pass = 2;
+ now.tv_sec = strtoul(argv[1], &endptr, 0);
+ if (*endptr != 0)
+ return pr_perror("strtoul");
+
+ return check("child after exec", &now);
+ }
+
+ nscheck();
+
+ ksft_set_plan(5);
+
+ clock_gettime(CLOCK_MONOTONIC, &now);
+
+ if (unshare_timens())
+ return 1;
+
+ if (_settime(CLOCK_MONOTONIC, OFFSET))
+ return 1;
+
+ if (check("parent before vfork", &now))
+ return 1;
+
+ pid = vfork();
+ if (pid < 0)
+ return pr_perror("fork");
+
+ if (pid == 0) {
+ char now_str[64];
+ char *cargv[] = {"exec", now_str, NULL};
+ char *cenv[] = {NULL};
+
+ // Check that we are still in the source timens.
+ if (check("child before exec", &now))
+ return 1;
+
+ /* Check for proper vvar offsets after execve. */
+ snprintf(now_str, sizeof(now_str), "%ld", now.tv_sec + OFFSET);
+ execve("/proc/self/exe", cargv, cenv);
+ return pr_perror("execve");
+ }
+
+ if (waitpid(pid, &status, 0) != pid)
+ return pr_perror("waitpid");
+
+ if (status)
+ ksft_exit_fail();
+ ksft_inc_pass_cnt();
+ ksft_test_result_pass("wait for child\n");
+
+ // Check that we are still in the source timens.
+ if (check("parent after vfork", &now))
+ return 1;
+
+ ksft_exit_pass();
+ return 0;
+}
--
2.37.3.968.ga6b4b080e4-goog
On 2022-09-21 03:31, Andrei Vagin wrote:
> From: Andrei Vagin <[email protected]>
>
> * check that a child process is in parent's time namespace after vfork.
> * check that a child process is in the target namespace after exec.
>
> Output on success:
> 1..5
> ok 1 parent before vfork
> ok 2 child before exec
> ok 3 child after exec
> ok 4 wait for child
> ok 5 parent after vfork
> # Totals: pass:5 fail:0 xfail:0 xpass:0 skip:0 error:0
>
> Signed-off-by: Andrei Vagin <[email protected]>
> ---
> tools/testing/selftests/timens/.gitignore | 1 +
> tools/testing/selftests/timens/Makefile | 2 +-
> tools/testing/selftests/timens/vfork_exec.c | 132 ++++++++++++++++++++
> 3 files changed, 134 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/timens/vfork_exec.c
>
> diff --git a/tools/testing/selftests/timens/.gitignore
> b/tools/testing/selftests/timens/.gitignore
> index fe1eb8271b35..cae8dca0fbff 100644
> --- a/tools/testing/selftests/timens/.gitignore
> +++ b/tools/testing/selftests/timens/.gitignore
> @@ -8,3 +8,4 @@ procfs
> timens
> timer
> timerfd
> +vfork_exec
> diff --git a/tools/testing/selftests/timens/Makefile
> b/tools/testing/selftests/timens/Makefile
> index 3a5936cc10ab..f0d51d4d2c87 100644
> --- a/tools/testing/selftests/timens/Makefile
> +++ b/tools/testing/selftests/timens/Makefile
> @@ -1,4 +1,4 @@
> -TEST_GEN_PROGS := timens timerfd timer clock_nanosleep procfs exec
> futex
> +TEST_GEN_PROGS := timens timerfd timer clock_nanosleep procfs exec
> futex vfork_exec
> TEST_GEN_PROGS_EXTENDED := gettime_perf
>
> CFLAGS := -Wall -Werror -pthread
> diff --git a/tools/testing/selftests/timens/vfork_exec.c
> b/tools/testing/selftests/timens/vfork_exec.c
> new file mode 100644
> index 000000000000..9fd8a64d25a9
> --- /dev/null
> +++ b/tools/testing/selftests/timens/vfork_exec.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define _GNU_SOURCE
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <sched.h>
> +#include <stdio.h>
> +#include <stdbool.h>
> +#include <sys/stat.h>
> +#include <sys/syscall.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
> +#include <time.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <pthread.h>
> +
> +#include "log.h"
> +#include "timens.h"
> +
> +#define OFFSET (36000)
> +
> +static void *tcheck(void *arg)
> +{
> + struct timespec *now = arg, tst;
> + int i;
> +
> + for (i = 0; i < 2; i++) {
> + _gettime(CLOCK_MONOTONIC, &tst, i);
> + if (abs(tst.tv_sec - now->tv_sec) > 5) {
> + pr_fail("thread: unexpected value: %ld (%ld)\n",
> + tst.tv_sec, now->tv_sec);
> + return (void *)1UL;
> + }
> + }
> + return NULL;
> +}
> +
> +static int check_in_thread(struct timespec *now)
> +{
> + pthread_t th;
> + void *retval;
> +
> + if (pthread_create(&th, NULL, tcheck, now))
> + return pr_perror("thread");
> + if (pthread_join(th, &retval))
> + return pr_perror("pthread_join");
> + return !(retval == NULL);
> +}
> +
> +static int check(char *tst_name, struct timespec *now)
> +{
> + struct timespec tst;
> + int i;
> +
> + for (i = 0; i < 2; i++) {
> + _gettime(CLOCK_MONOTONIC, &tst, i);
> + if (abs(tst.tv_sec - now->tv_sec) > 5)
> + return pr_fail("%s: unexpected value: %ld (%ld)\n",
> + tst.tv_sec, now->tv_sec);
There is a missing argument for "%s" in pr_fail(). I'm actually
surprised that there is no __attribute__((format)) on
ksft_test_result_fail() that would allow GCC to catch this.
> + }
> + if (check_in_thread(now))
> + return 1;
> + ksft_test_result_pass("%s\n", tst_name);
> + return 0;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + struct timespec now;
> + int status;
> + pid_t pid;
> +
> + if (argc > 1) {
> + char *endptr;
> +
> + ksft_cnt.ksft_pass = 2;
> + now.tv_sec = strtoul(argv[1], &endptr, 0);
> + if (*endptr != 0)
> + return pr_perror("strtoul");
> +
> + return check("child after exec", &now);
> + }
> +
> + nscheck();
> +
> + ksft_set_plan(5);
> +
> + clock_gettime(CLOCK_MONOTONIC, &now);
> +
> + if (unshare_timens())
> + return 1;
> +
> + if (_settime(CLOCK_MONOTONIC, OFFSET))
> + return 1;
> +
> + if (check("parent before vfork", &now))
> + return 1;
> +
> + pid = vfork();
> + if (pid < 0)
> + return pr_perror("fork");
> +
> + if (pid == 0) {
> + char now_str[64];
> + char *cargv[] = {"exec", now_str, NULL};
> + char *cenv[] = {NULL};
> +
> + // Check that we are still in the source timens.
> + if (check("child before exec", &now))
> + return 1;
I know this is just a test, but...
Creating threads in a vfork()-child is quite dangerous (like most other
things that touch the libc state, which is shared with the parent
process). Here it works probably only because pthread_create() followed
by pthread_join() restores everything into more-or-less the original
state before returning control to the parent, but this is something that
libcs don't guarantee and that can break at any moment.
Also, returning from a vfork()-child is explicitly forbidden by the
vfork() contract because the parent would then return to an invalid
stack frame that could be arbitrarily clobbered by code executed in the
child after main() returned. Moreover, if I'm not mistaken, on x86 with
Intel CET-enabled glibc (assuming the support for CET is ever merged
into the kernel) such return would cause the parent to always trap
because the shadow stack will become inconsistent with the normal stack.
Instead, _exit() should be used here...
> +
> + /* Check for proper vvar offsets after execve. */
> + snprintf(now_str, sizeof(now_str), "%ld", now.tv_sec + OFFSET);
> + execve("/proc/self/exe", cargv, cenv);
> + return pr_perror("execve");
...and here.
> + }
> +
> + if (waitpid(pid, &status, 0) != pid)
> + return pr_perror("waitpid");
> +
> + if (status)
> + ksft_exit_fail();
> + ksft_inc_pass_cnt();
> + ksft_test_result_pass("wait for child\n");
> +
> + // Check that we are still in the source timens.
> + if (check("parent after vfork", &now))
> + return 1;
> +
> + ksft_exit_pass();
> + return 0;
> +}
Otherwise, both patches look good to me, thanks!
Sorry for being late,
Alexey
From: Andrei Vagin <[email protected]>
* check that a child process is in parent's time namespace after vfork.
* check that a child process is in the target namespace after exec.
Output on success:
1..4
ok 1 parent before vfork
ok 2 child after exec
ok 3 wait for child
ok 4 parent after vfork
# Totals: pass:4 fail:0 xfail:0 xpass:0 skip:0 error:0
Signed-off-by: Andrei Vagin <[email protected]>
---
v2: - don't create threads from a vfork-ed process.
- use _exit to exit from a vfork-ed process.
tools/testing/selftests/timens/.gitignore | 1 +
tools/testing/selftests/timens/Makefile | 2 +-
tools/testing/selftests/timens/vfork_exec.c | 139 ++++++++++++++++++++
3 files changed, 141 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/timens/vfork_exec.c
diff --git a/tools/testing/selftests/timens/.gitignore b/tools/testing/selftests/timens/.gitignore
index fe1eb8271b35..cae8dca0fbff 100644
--- a/tools/testing/selftests/timens/.gitignore
+++ b/tools/testing/selftests/timens/.gitignore
@@ -8,3 +8,4 @@ procfs
timens
timer
timerfd
+vfork_exec
diff --git a/tools/testing/selftests/timens/Makefile b/tools/testing/selftests/timens/Makefile
index 3a5936cc10ab..f0d51d4d2c87 100644
--- a/tools/testing/selftests/timens/Makefile
+++ b/tools/testing/selftests/timens/Makefile
@@ -1,4 +1,4 @@
-TEST_GEN_PROGS := timens timerfd timer clock_nanosleep procfs exec futex
+TEST_GEN_PROGS := timens timerfd timer clock_nanosleep procfs exec futex vfork_exec
TEST_GEN_PROGS_EXTENDED := gettime_perf
CFLAGS := -Wall -Werror -pthread
diff --git a/tools/testing/selftests/timens/vfork_exec.c b/tools/testing/selftests/timens/vfork_exec.c
new file mode 100644
index 000000000000..fe3d0e15aa7e
--- /dev/null
+++ b/tools/testing/selftests/timens/vfork_exec.c
@@ -0,0 +1,139 @@
+// SPDX-License-Identifier: GPL-2.0
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <sched.h>
+#include <stdio.h>
+#include <stdbool.h>
+#include <sys/stat.h>
+#include <sys/syscall.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <time.h>
+#include <unistd.h>
+#include <string.h>
+#include <pthread.h>
+
+#include "log.h"
+#include "timens.h"
+
+#define OFFSET (36000)
+
+struct thread_args {
+ char *tst_name;
+ struct timespec *now;
+};
+
+static void *tcheck(void *_args)
+{
+ struct thread_args *args = _args;
+ struct timespec *now = args->now, tst;
+ int i;
+
+ for (i = 0; i < 2; i++) {
+ _gettime(CLOCK_MONOTONIC, &tst, i);
+ if (abs(tst.tv_sec - now->tv_sec) > 5) {
+ pr_fail("%s: in-thread: unexpected value: %ld (%ld)\n",
+ args->tst_name, tst.tv_sec, now->tv_sec);
+ return (void *)1UL;
+ }
+ }
+ return NULL;
+}
+
+static int check_in_thread(char *tst_name, struct timespec *now)
+{
+ struct thread_args args = {
+ .tst_name = tst_name,
+ .now = now,
+ };
+ pthread_t th;
+ void *retval;
+
+ if (pthread_create(&th, NULL, tcheck, &args))
+ return pr_perror("thread");
+ if (pthread_join(th, &retval))
+ return pr_perror("pthread_join");
+ return !(retval == NULL);
+}
+
+static int check(char *tst_name, struct timespec *now)
+{
+ struct timespec tst;
+ int i;
+
+ for (i = 0; i < 2; i++) {
+ _gettime(CLOCK_MONOTONIC, &tst, i);
+ if (abs(tst.tv_sec - now->tv_sec) > 5)
+ return pr_fail("%s: unexpected value: %ld (%ld)\n",
+ tst_name, tst.tv_sec, now->tv_sec);
+ }
+ if (check_in_thread(tst_name, now))
+ return 1;
+ ksft_test_result_pass("%s\n", tst_name);
+ return 0;
+}
+
+int main(int argc, char *argv[])
+{
+ struct timespec now;
+ int status;
+ pid_t pid;
+
+ if (argc > 1) {
+ char *endptr;
+
+ ksft_cnt.ksft_pass = 1;
+ now.tv_sec = strtoul(argv[1], &endptr, 0);
+ if (*endptr != 0)
+ return pr_perror("strtoul");
+
+ return check("child after exec", &now);
+ }
+
+ nscheck();
+
+ ksft_set_plan(4);
+
+ clock_gettime(CLOCK_MONOTONIC, &now);
+
+ if (unshare_timens())
+ return 1;
+
+ if (_settime(CLOCK_MONOTONIC, OFFSET))
+ return 1;
+
+ if (check("parent before vfork", &now))
+ return 1;
+
+ pid = vfork();
+ if (pid < 0)
+ return pr_perror("fork");
+
+ if (pid == 0) {
+ char now_str[64];
+ char *cargv[] = {"exec", now_str, NULL};
+ char *cenv[] = {NULL};
+
+ /* Check for proper vvar offsets after execve. */
+ snprintf(now_str, sizeof(now_str), "%ld", now.tv_sec + OFFSET);
+ execve("/proc/self/exe", cargv, cenv);
+ pr_perror("execve");
+ _exit(1);
+ }
+
+ if (waitpid(pid, &status, 0) != pid)
+ return pr_perror("waitpid");
+
+ if (status)
+ ksft_exit_fail();
+ ksft_inc_pass_cnt();
+ ksft_test_result_pass("wait for child\n");
+
+ /* Check that we are still in the source timens. */
+ if (check("parent after vfork", &now))
+ return 1;
+
+ ksft_exit_pass();
+ return 0;
+}
--
2.38.0.413.g74048e4d9e-goog
On Sun, Oct 9, 2022 at 9:10 AM Alexey Izbyshev <[email protected]> wrote:
>
> On 2022-09-21 03:31, Andrei Vagin wrote:
> > From: Andrei Vagin <[email protected]>
<snip>
> > + if (pid == 0) {
> > + char now_str[64];
> > + char *cargv[] = {"exec", now_str, NULL};
> > + char *cenv[] = {NULL};
> > +
> > + // Check that we are still in the source timens.
> > + if (check("child before exec", &now))
> > + return 1;
>
> I know this is just a test, but...
>
> Creating threads in a vfork()-child is quite dangerous (like most other
> things that touch the libc state, which is shared with the parent
> process). Here it works probably only because pthread_create() followed
> by pthread_join() restores everything into more-or-less the original
> state before returning control to the parent, but this is something that
> libcs don't guarantee and that can break at any moment.
>
> Also, returning from a vfork()-child is explicitly forbidden by the
> vfork() contract because the parent would then return to an invalid
> stack frame that could be arbitrarily clobbered by code executed in the
> child after main() returned. Moreover, if I'm not mistaken, on x86 with
> Intel CET-enabled glibc (assuming the support for CET is ever merged
> into the kernel) such return would cause the parent to always trap
> because the shadow stack will become inconsistent with the normal stack.
> Instead, _exit() should be used here...
>
Hi Alexey,
You are right, it isn't a good idea to create threads from the vfork-ed
process. Now, vfork isn't a special case in the kernel code, so I think
we can just remove the check() call from here. I have sent an updated
version of this patch, pls take a look at it.
Thanks,
Andrei
On 2022-10-13 20:46, Andrei Vagin wrote:
> On Sun, Oct 9, 2022 at 9:10 AM Alexey Izbyshev <[email protected]>
> wrote:
>>
>> On 2022-09-21 03:31, Andrei Vagin wrote:
>> > From: Andrei Vagin <[email protected]>
>
> <snip>
>
>> > + if (pid == 0) {
>> > + char now_str[64];
>> > + char *cargv[] = {"exec", now_str, NULL};
>> > + char *cenv[] = {NULL};
>> > +
>> > + // Check that we are still in the source timens.
>> > + if (check("child before exec", &now))
>> > + return 1;
>>
>> I know this is just a test, but...
>>
>> Creating threads in a vfork()-child is quite dangerous (like most
>> other
>> things that touch the libc state, which is shared with the parent
>> process). Here it works probably only because pthread_create()
>> followed
>> by pthread_join() restores everything into more-or-less the original
>> state before returning control to the parent, but this is something
>> that
>> libcs don't guarantee and that can break at any moment.
>>
>> Also, returning from a vfork()-child is explicitly forbidden by the
>> vfork() contract because the parent would then return to an invalid
>> stack frame that could be arbitrarily clobbered by code executed in
>> the
>> child after main() returned. Moreover, if I'm not mistaken, on x86
>> with
>> Intel CET-enabled glibc (assuming the support for CET is ever merged
>> into the kernel) such return would cause the parent to always trap
>> because the shadow stack will become inconsistent with the normal
>> stack.
>> Instead, _exit() should be used here...
>>
>
> Hi Alexey,
>
> You are right, it isn't a good idea to create threads from the vfork-ed
> process. Now, vfork isn't a special case in the kernel code, so I think
> we can just remove the check() call from here. I have sent an updated
> version of this patch, pls take a look at it.
>
Hi, Andrei,
While I think you could just skip check_in_thread() in the vfork()-child
instead of removing check() completely (the rest of the code in check()
is unlikely to mess up the libc state), I agree that the test is still
able to catch problems unconditionally affecting all CLONE_VM tasks
thanks to check_in_thread() in the parent, so I don't see much point in
holding it up further. Your v2 patch looks good enough to me, thanks!
Alexey