diff mbox series

[v2,1/8] Rewrite userns01.c using new LTP API

Message ID 20220315122351.8556-2-andrea.cervesato@suse.de
State Superseded
Headers show
Series Rewrite userns testing suite using new LTP API | expand

Commit Message

Andrea Cervesato March 15, 2022, 12:23 p.m. UTC 
Added common.h to be used instead of userns_helper.h by all userns
tests.

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.de>
---
 testcases/kernel/containers/userns/common.h   |  58 +++++++++
 testcases/kernel/containers/userns/userns01.c | 120 ++++++++----------
 2 files changed, 113 insertions(+), 65 deletions(-)
 create mode 100644 testcases/kernel/containers/userns/common.h

Comments

Petr Vorel March 23, 2022, 10:17 a.m. UTC | #1 
Hi Andrea,

Reviewed-by: Petr Vorel <pvorel@suse.cz>

nit: I'd sometimes use underscore to make names more readable
i.e. update_map, parent_map_pid, OVERFLOW_UID_PATH, OVERFLOW_GID_PATH

> +static inline int check_newuser(void)
> +{
> +	int pid, status;
> +
> +	if (tst_kvercmp(3, 8, 0) < 0)
> +		tst_brk(TCONF, "CLONE_NEWUSER not supported");
Is this limitation needed, when we have the same check with ltp_clone_quick()?
> +
> +	pid = ltp_clone_quick(CLONE_NEWUSER | SIGCHLD, dummy_child, NULL);
> +	if (pid == -1)
> +		tst_brk(TCONF | TTERRNO, "CLONE_NEWUSER not supported");
> +
> +	SAFE_WAIT(&status);
> +
> +	return 0;
> +}
> +
> +static inline void updatemap(int cpid, bool type, int idnum, int parentmappid)
> +{
> +	char path[BUFSIZ];
> +	char content[BUFSIZ];
> +	int fd;
> +
> +	if (type == UID_MAP)
> +		sprintf(path, "/proc/%d/uid_map", cpid);
> +	else if (type == GID_MAP)
> +		sprintf(path, "/proc/%d/gid_map", cpid);
> +	else
> +		tst_brk(TBROK, "invalid type parameter");

nit: maybe switch would be more readable.

...

Kind regards,
Petr
Petr Vorel March 24, 2022, 8:31 p.m. UTC | #2 
Hi Andrea,

>  testcases/kernel/containers/userns/common.h   |  58 +++++++++
...
> +static inline int check_newuser(void)
check_newuser() always return 0, it's return value is not used in any test.
Maybe pid was planned to be used in original implementation, but for now I'd
change return type to void.

> +{
> +	int pid, status;
> +
> +	if (tst_kvercmp(3, 8, 0) < 0)
> +		tst_brk(TCONF, "CLONE_NEWUSER not supported");
> +
> +	pid = ltp_clone_quick(CLONE_NEWUSER | SIGCHLD, dummy_child, NULL);
> +	if (pid == -1)
> +		tst_brk(TCONF | TTERRNO, "CLONE_NEWUSER not supported");
> +
> +	SAFE_WAIT(&status);
> +
> +	return 0;
> +}

Kind regards,
Petr
Petr Vorel March 24, 2022, 8:50 p.m. UTC | #3 
Hi Andrea,

...
> +static inline void updatemap(int cpid, bool type, int idnum, int parentmappid)
nit: This header should #include <stdbool.h>, not all tests (they use UID_MAP,
thus they don't need it).

Kind regards,
Petr

> +{
> +	char path[BUFSIZ];
> +	char content[BUFSIZ];
> +	int fd;
> +
> +	if (type == UID_MAP)
> +		sprintf(path, "/proc/%d/uid_map", cpid);
> +	else if (type == GID_MAP)
> +		sprintf(path, "/proc/%d/gid_map", cpid);
> +	else
> +		tst_brk(TBROK, "invalid type parameter");
> +
> +	sprintf(content, "%d %d 1", idnum, parentmappid);
> +
> +	fd = SAFE_OPEN(path, O_WRONLY, 0644);
> +	SAFE_WRITE(1, fd, content, strlen(content));
> +	SAFE_CLOSE(fd);
> +}
Andrea Cervesato March 25, 2022, 9:18 a.m. UTC | #4 
Hi Petr,

On 3/24/22 21:50, Petr Vorel wrote:
> Hi Andrea,
>
> ...
>> +static inline void updatemap(int cpid, bool type, int idnum, int parentmappid)
> nit: This header should #include <stdbool.h>, not all tests (they use UID_MAP,
> thus they don't need it).
I think this will be replaced with integer, according also with the 
if/else statement which is below and use switch to check its value as well.
> Kind regards,
> Petr
>
>> +{
>> +	char path[BUFSIZ];
>> +	char content[BUFSIZ];
>> +	int fd;
>> +
>> +	if (type == UID_MAP)
>> +		sprintf(path, "/proc/%d/uid_map", cpid);
>> +	else if (type == GID_MAP)
>> +		sprintf(path, "/proc/%d/gid_map", cpid);
>> +	else
>> +		tst_brk(TBROK, "invalid type parameter");
>> +
>> +	sprintf(content, "%d %d 1", idnum, parentmappid);
>> +
>> +	fd = SAFE_OPEN(path, O_WRONLY, 0644);
>> +	SAFE_WRITE(1, fd, content, strlen(content));
>> +	SAFE_CLOSE(fd);
>> +}
Petr Vorel March 25, 2022, 10:06 a.m. UTC | #5 
Hi Andrea,

> > > +static inline void updatemap(int cpid, bool type, int idnum, int parentmappid)
> > nit: This header should #include <stdbool.h>, not all tests (they use UID_MAP,
> > thus they don't need it).
> I think this will be replaced with integer, according also with the if/else
> statement which is below and use switch to check its value as well.

+1

Kind regards,
Petr
diff mbox series

Patch

diff --git a/testcases/kernel/containers/userns/common.h b/testcases/kernel/containers/userns/common.h
new file mode 100644
index 000000000..95160c8cc
--- /dev/null
+++ b/testcases/kernel/containers/userns/common.h
@@ -0,0 +1,58 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) Huawei Technologies Co., Ltd., 2015
+ * Copyright (C) 2022 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
+ */
+
+#ifndef COMMON_H
+#define COMMON_H
+
+#include "tst_test.h"
+#include "lapi/namespaces_constants.h"
+
+#define UID_MAP 0
+#define GID_MAP 1
+
+static int dummy_child(void *v)
+{
+	(void)v;
+	return 0;
+}
+
+static inline int check_newuser(void)
+{
+	int pid, status;
+
+	if (tst_kvercmp(3, 8, 0) < 0)
+		tst_brk(TCONF, "CLONE_NEWUSER not supported");
+
+	pid = ltp_clone_quick(CLONE_NEWUSER | SIGCHLD, dummy_child, NULL);
+	if (pid == -1)
+		tst_brk(TCONF | TTERRNO, "CLONE_NEWUSER not supported");
+
+	SAFE_WAIT(&status);
+
+	return 0;
+}
+
+static inline void updatemap(int cpid, bool type, int idnum, int parentmappid)
+{
+	char path[BUFSIZ];
+	char content[BUFSIZ];
+	int fd;
+
+	if (type == UID_MAP)
+		sprintf(path, "/proc/%d/uid_map", cpid);
+	else if (type == GID_MAP)
+		sprintf(path, "/proc/%d/gid_map", cpid);
+	else
+		tst_brk(TBROK, "invalid type parameter");
+
+	sprintf(content, "%d %d 1", idnum, parentmappid);
+
+	fd = SAFE_OPEN(path, O_WRONLY, 0644);
+	SAFE_WRITE(1, fd, content, strlen(content));
+	SAFE_CLOSE(fd);
+}
+
+#endif
diff --git a/testcases/kernel/containers/userns/userns01.c b/testcases/kernel/containers/userns/userns01.c
index 1c8cf570d..9e52ebece 100644
--- a/testcases/kernel/containers/userns/userns01.c
+++ b/testcases/kernel/containers/userns/userns01.c
@@ -1,115 +1,105 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
  * Copyright (c) Huawei Technologies Co., Ltd., 2015
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- *  (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
- * the GNU General Public License for more details.
+ * Copyright (C) 2022 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
  */
 
-/*
- * Verify that:
- *  If a user ID has no mapping inside the namespace, user ID and group
- * ID will be the value defined in the file /proc/sys/kernel/overflowuid(65534)
- * and /proc/sys/kernel/overflowgid(65534). A child process has a full set
- * of permitted and effective capabilities, even though the program was
- * run from an unprivileged account.
+/*\
+ * [Description]
+ *
+ * Verify that if a user ID has no mapping inside the namespace, user ID and
+ * group ID will be the value defined in the file /proc/sys/kernel/overflowuid(65534)
+ * and /proc/sys/kernel/overflowgid(65534). A child process has a full set of
+ * permitted and effective capabilities, even though the program was run from an
+ * unprivileged account.
  */
 
+#include "tst_test.h"
+
+#ifdef HAVE_LIBCAP
 #define _GNU_SOURCE
-#include <sys/wait.h>
-#include <assert.h>
+
 #include <stdio.h>
-#include <stdlib.h>
-#include <unistd.h>
-#include <string.h>
-#include <errno.h>
-#include "userns_helper.h"
-#include "test.h"
+#include <stdbool.h>
+#include "common.h"
 #include "config.h"
-#if HAVE_SYS_CAPABILITY_H
 #include <sys/capability.h>
-#endif
 
 #define OVERFLOWUIDPATH "/proc/sys/kernel/overflowuid"
 #define OVERFLOWGIDPATH "/proc/sys/kernel/overflowgid"
 
-char *TCID = "user_namespace1";
-int TST_TOTAL = 1;
-
 static long overflowuid;
 static long overflowgid;
 
 /*
  * child_fn1() - Inside a new user namespace
  */
-static int child_fn1(void *arg LTP_ATTRIBUTE_UNUSED)
+static int child_fn1(LTP_ATTRIBUTE_UNUSED void *arg)
 {
-	int exit_val = 0;
 	int uid, gid;
-#ifdef HAVE_LIBCAP
 	cap_t caps;
 	int i, last_cap;
 	cap_flag_value_t flag_val;
-#endif
 
 	uid = geteuid();
 	gid = getegid();
 
-	tst_resm(TINFO, "USERNS test is running in a new user namespace.");
+	tst_res(TINFO, "USERNS test is running in a new user namespace.");
 
-	if (uid != overflowuid || gid != overflowgid) {
-		printf("Got unexpected result of uid=%d gid=%d\n", uid, gid);
-		exit_val = 1;
-	}
+	if (uid != overflowuid || gid != overflowgid)
+		tst_res(TFAIL, "got unexpected uid=%d gid=%d", uid, gid);
+	else
+		tst_res(TPASS, "got expected uid and gid");
 
-#ifdef HAVE_LIBCAP
 	caps = cap_get_proc();
-	SAFE_FILE_SCANF(NULL, "/proc/sys/kernel/cap_last_cap", "%d", &last_cap);
+
+	SAFE_FILE_SCANF("/proc/sys/kernel/cap_last_cap", "%d", &last_cap);
+
 	for (i = 0; i <= last_cap; i++) {
 		cap_get_flag(caps, i, CAP_EFFECTIVE, &flag_val);
-		if (flag_val == 0)
+		if (!flag_val)
 			break;
+
 		cap_get_flag(caps, i, CAP_PERMITTED, &flag_val);
-		if (flag_val == 0)
+		if (!flag_val)
 			break;
 	}
 
-	if (flag_val == 0) {
-		printf("unexpected effective/permitted caps at %d\n", i);
-		exit_val = 1;
-	}
-#else
-	printf("System is missing libcap.\n");
-#endif
-	return exit_val;
+	if (!flag_val)
+		tst_res(TFAIL, "unexpected effective/permitted caps at %d", i);
+	else
+		tst_res(TPASS, "expected capabilities");
+
+	return 0;
 }
 
 static void setup(void)
 {
 	check_newuser();
-	SAFE_FILE_SCANF(NULL, OVERFLOWUIDPATH, "%ld", &overflowuid);
-	SAFE_FILE_SCANF(NULL, OVERFLOWGIDPATH, "%ld", &overflowgid);
+
+	SAFE_FILE_SCANF(OVERFLOWUIDPATH, "%ld", &overflowuid);
+	SAFE_FILE_SCANF(OVERFLOWGIDPATH, "%ld", &overflowgid);
 }
 
-int main(int argc, char *argv[])
+static void run(void)
 {
-	int lc;
+	int pid;
 
-	tst_parse_opts(argc, argv, NULL, NULL);
-	setup();
+	pid = ltp_clone_quick(CLONE_NEWUSER | SIGCHLD, child_fn1, NULL);
+	if (pid < 0)
+		tst_brk(TBROK | TTERRNO, "clone failed");
+}
 
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-		TEST(do_clone_unshare_test(T_CLONE, CLONE_NEWUSER,
-			child_fn1, NULL));
+static struct tst_test test = {
+	.setup = setup,
+	.test_all = run,
+	.needs_root = 1,
+	.needs_kconfigs = (const char *[]) {
+		"CONFIG_USER_NS",
+		NULL,
+	},
+};
 
-		if (TEST_RETURN == -1)
-			tst_brkm(TFAIL | TTERRNO, NULL, "clone failed");
-		tst_record_childstatus(NULL, -1);
-	}
-	tst_exit();
-}
+#else
+TST_TEST_TCONF("System is missing libcap");
+#endif