On 9/10/19 6:03 AM, Eugene Syromiatnikov wrote:

• tools/testing/selftests/clone3/clone3.c (CLONE3_ARGS_NO_TEST,

CLONE3_ARGS_ALL_0, CLONE3_ARGS_ALL_1): Change into an enum. (call_clone3): Change test_mode parameter type to enum test_mode; use switch statement for actions that dependent on test_mode selection. (test_clone3): Change test_mode parameter type to enum test_mode.

You don't need the file name in the commit log. Please describe what you are fixing/doing in the commit. Describing the actual code changes doesn't help.

Including why these changes are needed as opposed the actual changes will be helpful. I think I know why, I would like you to tell me why.

Signed-off-by: Eugene Syromiatnikov esyr@redhat.com

tools/testing/selftests/clone3/clone3.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/clone3/clone3.c b/tools/testing/selftests/clone3/clone3.c index a0f1989..7b65ee5 100644 --- a/tools/testing/selftests/clone3/clone3.c +++ b/tools/testing/selftests/clone3/clone3.c @@ -24,16 +24,18 @@ /* V1 includes set_tid */ #define CLONE3_ARGS_SIZE_V1 72 -#define CLONE3_ARGS_NO_TEST 0 -#define CLONE3_ARGS_ALL_0 1 -#define CLONE3_ARGS_ALL_1 2 +enum test_mode {

• CLONE3_ARGS_NO_TEST,
• CLONE3_ARGS_ALL_0,
• CLONE3_ARGS_ALL_1,

+}; static pid_t raw_clone(struct clone_args *args, size_t size) { return syscall(__NR_clone3, args, size); } -static int call_clone3(int flags, size_t size, int test_mode) +static int call_clone3(int flags, size_t size, enum test_mode test_mode) { struct clone_args args = {0}; pid_t ppid = -1; @@ -46,7 +48,8 @@ static int call_clone3(int flags, size_t size, int test_mode) if (size == 0) size = sizeof(struct clone_args);

• if (test_mode == CLONE3_ARGS_ALL_0) {

• switch (test_mode) {
• case CLONE3_ARGS_ALL_0: args.flags = 0; args.pidfd = 0; args.child_tid = 0;

@@ -56,7 +59,9 @@ static int call_clone3(int flags, size_t size, int test_mode) args. stack_size = 0; args.tls = 0; args.set_tid = 0;

• } else if (test_mode == CLONE3_ARGS_ALL_1) {

• break;
  

• case CLONE3_ARGS_ALL_1: args.flags = 1; args.pidfd = 1; args.child_tid = 1;

@@ -66,6 +71,7 @@ static int call_clone3(int flags, size_t size, int test_mode) args. stack_size = 1; args.tls = 1; args.set_tid = 1;

• break;
  

  }

pid = raw_clone(&args, size); @@ -91,7 +97,8 @@ static int call_clone3(int flags, size_t size, int test_mode) return 0; } -static int test_clone3(int flags, size_t size, int expected, int test_mode) +static int test_clone3(int flags, size_t size, int expected,

•        enum test_mode test_mode)
  

  { int ret;

thanks, -- Shuah

On 9/10/19 6:03 AM, Eugene Syromiatnikov wrote:

Check that the kernel fails calls with exit_signal with non-zero highest 32 bits.

Describe what you are testing:

"Add a test case for clone3() non-zero highest 32 bits behavior. It should fail with exit_signal value??"

Add checks for unsupported cases. Handle unsupported architectures and configurations with skip

• tools/testing/selftests/clone3/clone3.c (enum test_mode): Add

CLONE3_ARGS_BIG_EXIT_SIGNAL. (call_clone3): Add args.exit_signal initialisation in case test_mode == CLONE3_ARGS_BIG_EXIT_SIGNAL. (main): Add test_clone3 clone check with test_mode == CLONE3_ARGS_BIG_EXIT_SIGNAL.

Please don't include pseudo code in the commit log.

Signed-off-by: Eugene Syromiatnikov esyr@redhat.com

tools/testing/selftests/clone3/clone3.c | 9 +++++++++ 1 file changed, 9 insertions(+)

diff --git a/tools/testing/selftests/clone3/clone3.c b/tools/testing/selftests/clone3/clone3.c index 7b65ee5..4f23a0c 100644 --- a/tools/testing/selftests/clone3/clone3.c +++ b/tools/testing/selftests/clone3/clone3.c @@ -28,6 +28,7 @@ enum test_mode { CLONE3_ARGS_NO_TEST, CLONE3_ARGS_ALL_0, CLONE3_ARGS_ALL_1,

• CLONE3_ARGS_BIG_EXIT_SIGNAL, };

static pid_t raw_clone(struct clone_args *args, size_t size) @@ -72,6 +73,10 @@ static int call_clone3(int flags, size_t size, enum test_mode test_mode) args.tls = 1; args.set_tid = 1; break;

• case CLONE3_ARGS_BIG_EXIT_SIGNAL:

• args.exit_signal = 0xbadc0ded00000000;
  

Please add a comment to indicate what this is. I am assuming this bad sig val.

• break;
  

  }

pid = raw_clone(&args, size); @@ -146,6 +151,10 @@ int main(int argc, char *argv[]) /* Do a clone3() with all members set to 1 */ if (test_clone3(0, CLONE3_ARGS_SIZE_V0, -EINVAL, CLONE3_ARGS_ALL_1)) goto on_error;

• /* Do a clone3() with exit_signal having highest 32 bits non-zero */
• if (test_clone3(0, CLONE3_ARGS_SIZE_V0, -EINVAL,

• 	CLONE3_ARGS_BIG_EXIT_SIGNAL))
  

• goto on_error;
  

  /*
  • Do a clone3() with sizeof(struct clone_args) + 8
  • and all members set to 0.

thanks, -- Shuah

On 9/10/19 6:03 AM, Eugene Syromiatnikov wrote:

Flags parameter in both userspace and kernel clone args is 64-bit wide, there's little reason to have it signed and 32-bit in tests.

So what are doing? You are stating the problem here, how are you fixing it?

• tools/testing/selftests/clone3/clone3.c: Include <inttypes.h> and

<stdint.h>. (call_clone3): Change flags parameter type from int to uint64_t. (test_clone3): Change flags parameter type from int to uint64_t; change the format string that prints it accordingly.

I am not going to say this again. Please don't include pseudo code in the commit log.

Signed-off-by: Eugene Syromiatnikov esyr@redhat.com

tools/testing/selftests/clone3/clone3.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/clone3/clone3.c b/tools/testing/selftests/clone3/clone3.c index 4f23a0c..1746a9b 100644 --- a/tools/testing/selftests/clone3/clone3.c +++ b/tools/testing/selftests/clone3/clone3.c @@ -4,8 +4,10 @@ #define _GNU_SOURCE #include <errno.h> +#include <inttypes.h> #include <linux/types.h> #include <linux/sched.h> +#include <stdint.h> #include <stdio.h> #include <stdlib.h> #include <sys/syscall.h> @@ -36,7 +38,7 @@ static pid_t raw_clone(struct clone_args *args, size_t size) return syscall(__NR_clone3, args, size); } -static int call_clone3(int flags, size_t size, enum test_mode test_mode) +static int call_clone3(uint64_t flags, size_t size, enum test_mode test_mode) { struct clone_args args = {0}; pid_t ppid = -1; @@ -102,12 +104,13 @@ static int call_clone3(int flags, size_t size, enum test_mode test_mode) return 0; } -static int test_clone3(int flags, size_t size, int expected, +static int test_clone3(uint64_t flags, size_t size, int expected, enum test_mode test_mode) { int ret;

• ksft_print_msg("[%d] Trying clone3() with flags 0x%x (size %d)\n",

• ksft_print_msg("[%d] Trying clone3() with flags %#" PRIx64 " (size %d)"

• 	"\n",
  getpid(), flags, size);
  

  ret = call_clone3(flags, size, test_mode); ksft_print_msg("[%d] clone3() with flags says :%d expected %d\n",

thanks, -- Shuah

On 9/10/19 6:03 AM, Eugene Syromiatnikov wrote:

• tools/testing/selftests/clone3/clone3.c (test_clone3): Change format

qualifier for printing size field from %d to %zu; place colon right after the word "says".

Please drop the file name. The rest looks good. I am assuming there is a reason for doing this. Please include compile warns if applicable.

Signed-off-by: Eugene Syromiatnikov esyr@redhat.com

tools/testing/selftests/clone3/clone3.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/clone3/clone3.c b/tools/testing/selftests/clone3/clone3.c index 1746a9b..7b8d927 100644 --- a/tools/testing/selftests/clone3/clone3.c +++ b/tools/testing/selftests/clone3/clone3.c @@ -109,11 +109,11 @@ static int test_clone3(uint64_t flags, size_t size, int expected, { int ret;

• ksft_print_msg("[%d] Trying clone3() with flags %#" PRIx64 " (size %d)"

• ksft_print_msg("[%d] Trying clone3() with flags %#" PRIx64 " (size %zu)" "\n", getpid(), flags, size); ret = call_clone3(flags, size, test_mode);

• ksft_print_msg("[%d] clone3() with flags says :%d expected %d\n",

• ksft_print_msg("[%d] clone3() with flags says: %d expected %d\n", getpid(), ret, expected); if (ret != expected) ksft_exit_fail_msg(

thanks, -- Shuah

On 9/10/19 6:03 AM, Eugene Syromiatnikov wrote:

clone3() is available on most architectures, so there's no reason to restrict the respective self-tests to x86_64.

Is it missing on any? Please key off of the return value and exit with skip if unsupported.

• tools/testing/selftests/clone3/Makefile (TEST_GEN_PROGS): Set always,

not only ifeq ($(ARCH),x86_64).

Please - no file names in commit log.

Signed-off-by: Eugene Syromiatnikov esyr@redhat.com

tools/testing/selftests/clone3/Makefile | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/testing/selftests/clone3/Makefile b/tools/testing/selftests/clone3/Makefile index 4efcf45..faa069c 100644 --- a/tools/testing/selftests/clone3/Makefile +++ b/tools/testing/selftests/clone3/Makefile @@ -4,8 +4,6 @@ ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/i386/) CFLAGS += -I../../../../usr/include/ -ifeq ($(ARCH),x86_64)

• TEST_GEN_PROGS := clone3 clone3_set_tid

-endif +TEST_GEN_PROGS := clone3 clone3_set_tid include ../lib.mk

This is fine. Where is the code to handle unsupported case?

thanks, -- Shuah

On 9/10/19 6:03 AM, Eugene Syromiatnikov wrote:

• tools/testing/selftests/Makefile (TARGETS): Add clone3.

Again. No filenames in the commit log. Please add more detail.

"Adding clone3() tests to kselftest default run and details on what this tests"

This will be helpful to users.

Signed-off-by: Eugene Syromiatnikov esyr@redhat.com

tools/testing/selftests/Makefile | 1 + 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index 25b43a8c..05163e4 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -4,6 +4,7 @@ TARGETS += bpf TARGETS += breakpoints TARGETS += capabilities TARGETS += cgroup +TARGETS += clone3 TARGETS += cpufreq TARGETS += cpu-hotplug TARGETS += drivers/dma-buf

Please make sure the test doesn't hang and all the use-cases listed in the kselftest.rst are supported.

make kselftest make -C tools/testing/selftests O= and KBUILD_OUTPUT cases as well as running make directly in the clode3 dir.

Please include test output and usage instructions if any to this commit log as this is the patch that enables it in the default run.

thanks, -- Shuah