[LTP] [PATCH] mount03: Convert to new API

[LTP] [PATCH] mount03: Convert to new API

[chenhx.fnst]

Signed-off-by: Chen Hanxiao <chenhx.fnst@fujitsu.com>
---
 testcases/kernel/syscalls/mount/mount03.c | 462 ++++++++--------------
 1 file changed, 175 insertions(+), 287 deletions(-)

diff --git a/testcases/kernel/syscalls/mount/mount03.c b/testcases/kernel/syscalls/mount/mount03.c
index 25f99bbfc..905fcbbac 100644
--- a/testcases/kernel/syscalls/mount/mount03.c
+++ b/testcases/kernel/syscalls/mount/mount03.c
@@ -1,136 +1,97 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
+ * Copyright (c) Linux Test Project, 2022
  * Copyright (c) Wipro Technologies Ltd, 2002.  All Rights Reserved.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of version 2 of the GNU General Public License as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it would be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
- *
- * You should have received a copy of the GNU General Public License along
- * with this program; if not, write the Free Software Foundation, Inc.,
- * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
- *
  */
 
-/*
- * DESCRIPTION
- *	Check for basic mount(2) system call flags.
+/*\
+ * [Description]
+ *
+ * Check for basic mount(2) system call flags.
  *
- *	Verify that mount(2) syscall passes for each flag setting and validate
- *	the flags
- *	1) MS_RDONLY - mount read-only.
- *	2) MS_NODEV - disallow access to device special files.
- *	3) MS_NOEXEC - disallow program execution.
- *	4) MS_SYNCHRONOUS - writes are synced at once.
- *	5) MS_REMOUNT - alter flags of a mounted FS.
- *	6) MS_NOSUID - ignore suid and sgid bits.
- *	7) MS_NOATIME - do not update access times.
+ * Verify that mount(2) syscall passes for each flag setting and validate
+ * the flags
+ *
+ * - MS_RDONLY - mount read-only.
+ * - MS_NODEV - disallow access to device special files.
+ * - MS_NOEXEC - disallow program execution.
+ * - MS_SYNCHRONOUS - writes are synced at once.
+ * - MS_REMOUNT - alter flags of a mounted FS.
+ * - MS_NOSUID - ignore suid and sgid bits.
+ * - MS_NOATIME - do not update access times.
  */
 
 #ifndef _GNU_SOURCE
 #define _GNU_SOURCE
 #endif
 
-#include <sys/types.h>
-#include <sys/mount.h>
-#include <sys/stat.h>
-#include <sys/wait.h>
-#include <assert.h>
-#include <errno.h>
-#include <fcntl.h>
-#include <pwd.h>
-#include <unistd.h>
-
-#include "test.h"
-#include "safe_macros.h"
-
-static void setup(void);
-static void cleanup(void);
-static int test_rwflag(int, int);
-
-char *TCID = "mount03";
-int TST_TOTAL = 7;
-
 #define TEMP_FILE	"temp_file"
 #define FILE_MODE	(S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH)
-#define DIR_MODE	(S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP| \
-			 S_IXGRP|S_IROTH|S_IXOTH)
 #define SUID_MODE	(S_ISUID|S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH)
 
-static const char mntpoint[] = "mntpoint";
-static const char *device;
-static const char *fs_type;
-static int fildes;
+#include <stdio.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <linux/limits.h>
+#include <stdlib.h>
+#include <pwd.h>
+#include "tst_test.h"
+#include "tst_safe_file_ops.h"
+#include "lapi/mount.h"
 
+#define MNTPOINT        "mntpoint"
+#define FILE_PATH	MNTPOINT"/mount03_setuid_test"
+#define TCASE_ENTRY(_flags)	{.name = "Flag " #_flags, .flags = _flags}
+
+static int otfd;
 static char write_buffer[BUFSIZ];
 static char read_buffer[BUFSIZ];
-static char path_name[PATH_MAX];
 static char file[PATH_MAX];
-
-long rwflags[] = {
-	MS_RDONLY,
-	MS_NODEV,
-	MS_NOEXEC,
-	MS_SYNCHRONOUS,
-	MS_RDONLY,
-	MS_NOSUID,
-	MS_NOATIME,
+static char nobody_uid[] = "nobody";
+static struct passwd *ltpuser;
+
+static struct tcase {
+	char *name;
+	unsigned int flags;
+} tcases[] = {
+	TCASE_ENTRY(MS_RDONLY),
+	TCASE_ENTRY(MS_NODEV),
+	TCASE_ENTRY(MS_NOEXEC),
+	TCASE_ENTRY(MS_SYNCHRONOUS),
+	TCASE_ENTRY(MS_RDONLY),
+	TCASE_ENTRY(MS_NOSUID),
+	TCASE_ENTRY(MS_NOATIME),
 };
 
-int main(int argc, char *argv[])
+static int test_ms_nosuid(void)
 {
-	int lc, i;
-
-	tst_parse_opts(argc, argv, NULL, NULL);
-
-	setup();
-
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
+	int pid, status;
 
-		tst_count = 0;
-
-		for (i = 0; i < TST_TOTAL; ++i) {
-
-			TEST(mount(device, mntpoint, fs_type, rwflags[i],
-				   NULL));
-
-			if (TEST_RETURN != 0) {
-				tst_resm(TFAIL | TTERRNO, "mount(2) failed");
-				continue;
-			}
-
-			/* Validate the rwflag */
-			if (test_rwflag(i, lc) == 1)
-				tst_resm(TFAIL, "mount(2) failed while"
-					 " validating %ld", rwflags[i]);
-			else
-				tst_resm(TPASS, "mount(2) passed with "
-					 "rwflag = %ld", rwflags[i]);
-
-			TEST(tst_umount(mntpoint));
-			if (TEST_RETURN != 0)
-				tst_brkm(TBROK | TTERRNO, cleanup,
-					 "umount(2) failed for %s", mntpoint);
+	switch (pid = SAFE_FORK()) {
+	case -1:
+		tst_brk(TFAIL | TERRNO, "fork failed");
+		break;
+	case 0:
+		SAFE_SETREUID(ltpuser->pw_uid, ltpuser->pw_uid);
+
+		execlp(file, basename(file), NULL);
+		exit(1);
+	default:
+		waitpid(pid, &status, 0);
+		if (WIFEXITED(status)) {
+			/* reset the setup_uid */
+			if (status)
+				return 0;
 		}
+		return 1;
 	}
-
-	cleanup();
-	tst_exit();
+	return 0;
 }
 
-/*
- * test_rwflag(int i, int cnt)
- * Validate the mount system call for rwflags.
- */
-int test_rwflag(int i, int cnt)
+static void test_rwflag(int i)
 {
-	int ret, fd, pid, status;
-	char nobody_uid[] = "nobody";
+	int ret;
 	time_t atime;
-	struct passwd *ltpuser;
 	struct stat file_stat;
 	char readbuf[20];
 
@@ -138,56 +99,42 @@ int test_rwflag(int i, int cnt)
 	case 0:
 		/* Validate MS_RDONLY flag of mount call */
 
-		snprintf(file, PATH_MAX, "%stmp", path_name);
-		fd = open(file, O_CREAT | O_RDWR, S_IRWXU);
-		if (fd == -1) {
-			if (errno == EROFS) {
-				return 0;
-			} else {
-				tst_resm(TWARN | TERRNO,
-					 "open didn't fail with EROFS");
-				return 1;
-			}
-		}
-		close(fd);
-		return 1;
+		snprintf(file, PATH_MAX, "%s/tmp", MNTPOINT);
+		TST_EXP_FAIL2(open(file, O_CREAT | O_RDWR, S_IRWXU),
+			      EROFS, "mount(2) passed with flag MS_RDONLY: "
+			      "open fail with EROFS as expected");
+
+		otfd = TST_RET;
+		break;
 	case 1:
 		/* Validate MS_NODEV flag of mount call */
 
-		snprintf(file, PATH_MAX, "%smynod_%d_%d", path_name, getpid(),
-			 cnt);
-		if (mknod(file, S_IFBLK | 0777, 0) == 0) {
-			fd = open(file, O_RDWR, S_IRWXU);
-			if (fd == -1) {
-				if (errno == EACCES) {
-					return 0;
-				} else {
-					tst_resm(TWARN | TERRNO,
-						 "open didn't fail with EACCES");
-					return 1;
-				}
-			}
-			close(fd);
-		} else {
-			tst_resm(TWARN | TERRNO, "mknod(2) failed to create %s",
-				 file);
-			return 1;
+		snprintf(file, PATH_MAX, "%s/mynod_%d", MNTPOINT, getpid());
+		if (SAFE_MKNOD(file, S_IFBLK | 0777, 0) == 0) {
+			TST_EXP_FAIL2(open(file, O_RDWR, S_IRWXU),
+				      EACCES, "mount(2) passed with flag MS_NODEV: "
+				      "open fail with EACCES as expected");
+			otfd = TST_RET;
 		}
-		return 1;
+		SAFE_UNLINK(file);
+		break;
 	case 2:
 		/* Validate MS_NOEXEC flag of mount call */
 
-		snprintf(file, PATH_MAX, "%stmp1", path_name);
-		fd = open(file, O_CREAT | O_RDWR, S_IRWXU);
-		if (fd == -1) {
-			tst_resm(TWARN | TERRNO, "opening %s failed", file);
-		} else {
-			close(fd);
+		snprintf(file, PATH_MAX, "%s/tmp1", MNTPOINT);
+		TST_EXP_FD_SILENT(open(file, O_CREAT | O_RDWR, S_IRWXU),
+				  "opening %s failed", file);
+		otfd = TST_RET;
+		if (otfd >= 0) {
+			SAFE_CLOSE(otfd);
 			ret = execlp(file, basename(file), NULL);
-			if ((ret == -1) && (errno == EACCES))
-				return 0;
+			if ((ret == -1) && (errno == EACCES)) {
+				tst_res(TPASS, "mount(2) passed with flag MS_NOEXEC");
+				break;
+			}
 		}
-		return 1;
+		tst_brk(TFAIL | TERRNO, "mount(2) failed with flag MS_NOEXEC");
+		break;
 	case 3:
 		/*
 		 * Validate MS_SYNCHRONOUS flag of mount call.
@@ -197,193 +144,134 @@ int test_rwflag(int i, int cnt)
 		strcpy(write_buffer, "abcdefghijklmnopqrstuvwxyz");
 
 		/* Creat a temporary file under above directory */
-		snprintf(file, PATH_MAX, "%s%s", path_name, TEMP_FILE);
-		fildes = open(file, O_RDWR | O_CREAT, FILE_MODE);
-		if (fildes == -1) {
-			tst_resm(TWARN | TERRNO,
-				 "open(%s, O_RDWR|O_CREAT, %#o) failed",
-				 file, FILE_MODE);
-			return 1;
-		}
+		snprintf(file, PATH_MAX, "%s/%s", MNTPOINT, TEMP_FILE);
+		TST_EXP_FD_SILENT(open(file, O_RDWR | O_CREAT, FILE_MODE),
+			   "open(%s, O_RDWR|O_CREAT, %#o) failed",
+			   file, FILE_MODE);
+		otfd = TST_RET;
 
 		/* Write the buffer data into file */
-		if (write(fildes, write_buffer, strlen(write_buffer)) !=
-		    (long)strlen(write_buffer)) {
-			tst_resm(TWARN | TERRNO, "writing to %s failed", file);
-			close(fildes);
-			return 1;
-		}
+		SAFE_WRITE(1, otfd, write_buffer, strlen(write_buffer));
 
 		/* Set the file ptr to b'nning of file */
-		if (lseek(fildes, 0, SEEK_SET) < 0) {
-			tst_resm(TWARN, "lseek() failed on %s, error="
-				 " %d", file, errno);
-			close(fildes);
-			return 1;
-		}
+		SAFE_LSEEK(otfd, 0, SEEK_SET);
 
 		/* Read the contents of file */
-		if (read(fildes, read_buffer, sizeof(read_buffer)) > 0) {
+		if (SAFE_READ(0, otfd, read_buffer, sizeof(read_buffer)) > 0) {
 			if (strcmp(read_buffer, write_buffer)) {
-				tst_resm(TWARN, "Data read from %s and written "
+				tst_brk(TFAIL, "Data read from %s and written "
 					 "mismatch", file);
-				close(fildes);
-				return 1;
 			} else {
-				close(fildes);
-				return 0;
+				SAFE_CLOSE(otfd);
+				tst_res(TPASS, "mount(2) passed with flag MS_SYNCHRONOUS");
+				break;
 			}
 		} else {
-			tst_resm(TWARN | TERRNO, "read() Fails on %s", file);
-			close(fildes);
-			return 1;
+			tst_brk(TFAIL | TERRNO, "read() Fails on %s", file);
 		}
-
+		break;
 	case 4:
 		/* Validate MS_REMOUNT flag of mount call */
 
-		TEST(mount(device, mntpoint, fs_type, MS_REMOUNT, NULL));
-		if (TEST_RETURN != 0) {
-			tst_resm(TWARN | TTERRNO, "mount(2) failed to remount");
-			return 1;
+		TEST(mount(tst_device->dev, MNTPOINT, tst_device->fs_type, MS_REMOUNT, NULL));
+		if (TST_RET != 0) {
+			tst_brk(TFAIL | TTERRNO, "mount(2) failed to remount");
 		} else {
-			snprintf(file, PATH_MAX, "%stmp2", path_name);
-			fd = open(file, O_CREAT | O_RDWR, S_IRWXU);
-			if (fd == -1) {
-				tst_resm(TWARN, "open(%s) on readonly "
+			snprintf(file, PATH_MAX, "%s/tmp2", MNTPOINT);
+			TEST(open(file, O_CREAT | O_RDWR, S_IRWXU));
+			otfd = TST_RET;
+			if (otfd == -1) {
+				tst_res(TFAIL, "open(%s) on readonly "
 					 "filesystem passed", file);
-				return 1;
-			} else {
-				close(fd);
-				return 0;
-			}
+			} else
+				tst_res(TPASS, "mount(2) passed with flag MS_REMOUNT");
 		}
+		break;
 	case 5:
 		/* Validate MS_NOSUID flag of mount call */
 
-		snprintf(file, PATH_MAX, "%smount03_setuid_test", path_name);
-
-		pid = fork();
-		switch (pid) {
-		case -1:
-			tst_resm(TBROK | TERRNO, "fork failed");
-			return 1;
-		case 0:
-			ltpuser = getpwnam(nobody_uid);
-			if (setreuid(ltpuser->pw_uid, ltpuser->pw_uid) == -1)
-				tst_resm(TWARN | TERRNO,
-					 "seteuid() failed to change euid to %d",
-					 ltpuser->pw_uid);
-
-			execlp(file, basename(file), NULL);
-			exit(1);
-		default:
-			waitpid(pid, &status, 0);
-			if (WIFEXITED(status)) {
-				/* reset the setup_uid */
-				if (status)
-					return 0;
-			}
-			return 1;
-		}
+		snprintf(file, PATH_MAX, "%s/mount03_setuid_test", MNTPOINT);
+		TEST(test_ms_nosuid());
+		if (TST_RET == 0)
+			tst_res(TPASS, "mount(2) passed with flag MS_NOSUID");
+		else
+			tst_res(TFAIL, "mount(2) failed with flag MS_NOSUID");
+		break;
 	case 6:
 		/* Validate MS_NOATIME flag of mount call */
 
-		snprintf(file, PATH_MAX, "%satime", path_name);
-		fd = open(file, O_CREAT | O_RDWR, S_IRWXU);
-		if (fd == -1) {
-			tst_resm(TWARN | TERRNO, "opening %s failed", file);
-			return 1;
-		}
+		snprintf(file, PATH_MAX, "%s/atime", MNTPOINT);
+		TST_EXP_FD_SILENT(open(file, O_CREAT | O_RDWR, S_IRWXU));
+		otfd = TST_RET;
 
-		if (write(fd, "TEST_MS_NOATIME", 15) != 15) {
-			tst_resm(TWARN | TERRNO, "write %s failed", file);
-			close(fd);
-			return 1;
-		}
+		SAFE_WRITE(1, otfd, "TEST_MS_NOATIME", 15);
 
-		if (fstat(fd, &file_stat) == -1) {
-			tst_resm(TWARN | TERRNO, "stat %s failed #1", file);
-			close(fd);
-			return 1;
-		}
+		SAFE_FSTAT(otfd, &file_stat);
 
 		atime = file_stat.st_atime;
 
 		sleep(1);
 
-		if (read(fd, readbuf, sizeof(readbuf)) == -1) {
-			tst_resm(TWARN | TERRNO, "read %s failed", file);
-			close(fd);
-			return 1;
-		}
+		SAFE_READ(0, otfd, readbuf, sizeof(readbuf));
 
-		if (fstat(fd, &file_stat) == -1) {
-			tst_resm(TWARN | TERRNO, "stat %s failed #2", file);
-			close(fd);
-			return 1;
-		}
-		close(fd);
+		SAFE_FSTAT(otfd, &file_stat);
 
 		if (file_stat.st_atime != atime) {
-			tst_resm(TWARN, "access time is updated");
-			return 1;
+			tst_res(TFAIL, "access time is updated");
+			break;
 		}
-		return 0;
+		tst_res(TPASS, "mount(2) passed with flag MS_NOATIME");
 	}
-	return 0;
 }
 
-static void setup(void)
+static void run(unsigned int n)
 {
-	char path[PATH_MAX];
-	struct stat file_stat;
-
-	tst_sig(FORK, DEF_HANDLER, cleanup);
-
-	tst_require_root();
-
-	tst_tmpdir();
-
-	fs_type = tst_dev_fs_type();
-	device = tst_acquire_device(cleanup);
-
-	if (!device)
-		tst_brkm(TCONF, cleanup, "Failed to obtain block device");
+	struct tcase *tc = &tcases[n];
 
-	tst_mkfs(cleanup, device, fs_type, NULL, NULL);
+	TEST(mount(tst_device->dev, MNTPOINT, tst_device->fs_type,
+		   tc->flags, NULL));
+	test_rwflag(n);
 
-	SAFE_MKDIR(cleanup, mntpoint, DIR_MODE);
-
-	if (getcwd(path_name, sizeof(path_name)) == NULL)
-		tst_brkm(TBROK, cleanup, "getcwd failed");
-
-	if (chmod(path_name, DIR_MODE) != 0)
-		tst_brkm(TBROK, cleanup, "chmod(%s, %#o) failed",
-			 path_name, DIR_MODE);
-
-	strncpy(path, path_name, PATH_MAX);
-	snprintf(path_name, PATH_MAX, "%s/%s/", path, mntpoint);
+	if (otfd >= 0)
+		SAFE_CLOSE(otfd);
+	if (tst_is_mounted(MNTPOINT))
+		SAFE_UMOUNT(MNTPOINT);
+}
 
-	SAFE_MOUNT(cleanup, device, mntpoint, fs_type, 0, NULL);
-	TST_RESOURCE_COPY(cleanup, "mount03_setuid_test", path_name);
+static void cleanup(void)
+{
+	if (otfd > -1)
+		SAFE_CLOSE(otfd);
+}
 
-	snprintf(file, PATH_MAX, "%smount03_setuid_test", path_name);
-	SAFE_STAT(cleanup, file, &file_stat);
+static void setup(void)
+{
+	struct stat file_stat = {0};
 
+	ltpuser = getpwnam(nobody_uid);
+	if (ltpuser == NULL)
+		tst_res(TFAIL, "getpwnam failed");
+	snprintf(file, PATH_MAX, "%s/mount03_setuid_test", MNTPOINT);
+	SAFE_STAT(MNTPOINT, &file_stat);
 	if (file_stat.st_mode != SUID_MODE &&
-	    chmod(file, SUID_MODE) < 0)
-		tst_brkm(TBROK, cleanup,
-			 "setuid for setuid_test failed");
-	SAFE_UMOUNT(cleanup, mntpoint);
-
-	TEST_PAUSE;
+	    chmod(MNTPOINT, SUID_MODE) < 0)
+		tst_res(TFAIL, "setuid for setuid_test failed");
 }
 
-static void cleanup(void)
-{
-	if (device)
-		tst_release_device(device);
+static const char *const resource_files[] = {
+	"mount03_setuid_test",
+	NULL,
+};
 
-	tst_rmdir();
-}
+static struct tst_test test = {
+	.tcnt = ARRAY_SIZE(tcases),
+	.test = run,
+	.setup = setup,
+	.cleanup = cleanup,
+	.needs_root = 1,
+	.format_device = 1,
+	.resource_files = resource_files,
+	.forks_child = 1,
+	.mntpoint = MNTPOINT,
+	.skip_filesystems = (const char *const []){"fuse", NULL},
+};
-- 
2.31.1

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] mount03: Convert to new API

[Petr Vorel]

Hi Chen,

quite nice work, but more cleanup is needed.

>  testcases/kernel/syscalls/mount/mount03.c | 462 ++++++++--------------
...
>  #define FILE_MODE	(S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH)
> -#define DIR_MODE	(S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP| \
> -			 S_IXGRP|S_IROTH|S_IXOTH)
>  #define SUID_MODE	(S_ISUID|S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH)

@Richie @Li @Metan: There are checkpatch.pl warnings. Yes, kernel folks does not
like permission warnings. Do we want to follow? Or should we remove these from
our checkpatch.pl fork (we use constants in many places)?

$ make check-mount03
mount03.c:29: WARNING: Symbolic permissions 'S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH' are not preferred. Consider using octal permissions '0644'.
mount03.c:30: WARNING: Symbolic permissions 'S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH' are not preferred. Consider using octal permissions '0511'.
mount03.c:50: WARNING: static char array declaration should probably be static const char
mount03.c:103: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.
mount03.c:114: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.
mount03.c:125: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.
mount03.c:181: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.
mount03.c:204: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.

...
> +#define MNTPOINT        "mntpoint"
...
> +static char nobody_uid[] = "nobody";
I'd remove constant and use "nobody" in the setup (the same like creat08.c).
If we want to have it defined, I'd use #define (like you use it in MNTPOINT).

Also your code produces warning:
mount03.c:50: WARNING: static char array declaration should probably be static const char

...
> +static int test_ms_nosuid(void)
...
> +	int pid, status;
pid_t pid; // don't use int for pid please.

This is not needed (-1 is handled by SAFE_FORK()), therefore you don't need
switch.

For a readability, your code (most of inherited from the original) is:
static int test_ms_nosuid(void)
{
	int pid, status;

	switch (pid = SAFE_FORK()) {
	case -1:
		tst_brk(TFAIL | TERRNO, "fork failed");
		break;
	case 0:
		SAFE_SETREUID(ltpuser->pw_uid, ltpuser->pw_uid);

		execlp(file, basename(file), NULL);
		exit(1);
	default:
		waitpid(pid, &status, 0);
		if (WIFEXITED(status)) {
			/* reset the setup_uid */
			if (status)
				return 0;
		}
		return 1;
	}
	return 0;
}

* I'm not sure why exit(1),
* we have SAFE_EXECLP. Therefore something like:


static void test_ms_nosuid(void)
{
	int status;
	pid_t pid;

	pid = SAFE_FORK();

	if (!pid) {
		SAFE_SETREUID(ltpuser->pw_uid, ltpuser->pw_uid);
		SAFE_EXECLP(file, basename(file), NULL);
	}

	SAFE_WAITPID(pid, &status, 0);
}

But SAFE_WAITPID() would not allow to print TPASS/TFAIL.

> -/*
> - * test_rwflag(int i, int cnt)
> - * Validate the mount system call for rwflags.
> - */
> -int test_rwflag(int i, int cnt)
> +static void test_rwflag(int i)
Instead of a crazy long switch which is in the original code, we prefer to
use functions which are put into struct tcase via function pointer. See e.g.
mq_open01.c for example.


...
> +		snprintf(file, PATH_MAX, "%s/mount03_setuid_test", MNTPOINT);
> +		TEST(test_ms_nosuid());
Please don't reuse TEST() macro for normal function. Just call it:

> +		if (TST_RET == 0)
> +			tst_res(TPASS, "mount(2) passed with flag MS_NOSUID");
> +		else
> +			tst_res(TFAIL, "mount(2) failed with flag MS_NOSUID");
There is TST_EXP_PASS() macro:
TST_EXP_PASS(test_ms_nosuid())
if (!TST_PASS)
		return;

But, please don't reuse TEST() macro for normal function. These are meant to be
for syscalls testing, here I'd print TPASS/TFAIL in test_ms_nosuid() and make it
void.

...
> +static void setup(void)
> +{
> +	struct stat file_stat = {0};

> +	ltpuser = getpwnam(nobody_uid);
ltpuser = SAFE_GETPWNAM("nobody");
(or use constant, but SAFE_GETPWNAM is required (no check, see creat08.c)

> +	if (ltpuser == NULL)
> +		tst_res(TFAIL, "getpwnam failed");
> +	snprintf(file, PATH_MAX, "%s/mount03_setuid_test", MNTPOINT);

> +	SAFE_STAT(MNTPOINT, &file_stat);
>  	if (file_stat.st_mode != SUID_MODE &&
> -	    chmod(file, SUID_MODE) < 0)
> -		tst_brkm(TBROK, cleanup,
> -			 "setuid for setuid_test failed");
> -	SAFE_UMOUNT(cleanup, mntpoint);
> -
> -	TEST_PAUSE;
> +	    chmod(MNTPOINT, SUID_MODE) < 0)
> +		tst_res(TFAIL, "setuid for setuid_test failed");
Please use tst_brk(TBROK, ...), original version has tst_brkm(TBROK, ...) for a
reason [1]:
1) tst_brk quits testing, which is usually required in setup function
2) TBROK is usually used instead of TFAIL in setup function

TBROK: Something has failed in test preparation phase.
TFAIL: Test has failed.

(This description should have been in LTP Test Writing Guidelines [2],
because it applies to both C and shell API.)


...
> +static struct tst_test test = {
> +	.tcnt = ARRAY_SIZE(tcases),
> +	.test = run,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.needs_root = 1,
> +	.format_device = 1,
> +	.resource_files = resource_files,

Please inline resource files like:
	.resource_files = (const char *const[]) {
		"mount03_setuid_test",
		NULL,
	},

It produces "mount03_setuid_test" in doc [3]:

| .resource_files | mount03_setuid_test |

But using .resource_files = resource_files produces just "resource_files":
| .resource_files | resource_files |

(to make doc locally: cd metadata && make # result is in ../docparse/metadata.html)

> +	.forks_child = 1,
> +	.mntpoint = MNTPOINT,
> +	.skip_filesystems = (const char *const []){"fuse", NULL},
Why you introduced skipping on FUSE? I don't see it in the old code, nothing
mentioned in commit message).

Also when I test it for all filesystems (which could be a good idea for mount
test) it fails for vfat, exfat and ntfs:

	.skip_filesystems = (const char *const []){"vfat", "exfat", "ntfs", NULL},
	.all_filesystems = 1,

Skipping just fuse does not help in my case:

tst_test.c:1599: TINFO: Testing on ntfs
tst_test.c:1064: TINFO: Formatting /dev/loop3 with ntfs opts='' extra opts=''
The partition start sector was not specified for /dev/loop3 and it could not be obtained automatically.  It has been set to 0.
The number of sectors per track was not specified for /dev/loop3 and it could not be obtained automatically.  It has been set to 0.
The number of heads was not specified for /dev/loop3 and it could not be obtained automatically.  It has been set to 0.
To boot from a device, Windows needs the 'partition start sector', the 'sectors per track' and the 'number of heads' to be set.
Windows will not be able to boot from this device.
mount03.c:102: TFAIL: mount(2) passed with flag MS_RDONLY: open fail with EROFS as expected succeeded
tst_device.c:394: TWARN: umount('mntpoint') failed with EINVAL
mount03.c:237: TBROK: umount(mntpoint) failed: EINVAL (22)

tst_test.c:1599: TINFO: Testing on exfat
tst_test.c:1064: TINFO: Formatting /dev/loop2 with exfat opts='' extra opts=''
mount03.c:102: TPASS: mount(2) passed with flag MS_RDONLY: open fail with EROFS as expected : EROFS (30)
mount03.c:112: TBROK: mknod() failed: EPERM (1)

tst_test.c:1599: TINFO: Testing on vfat
tst_test.c:1064: TINFO: Formatting /dev/loop1 with vfat opts='' extra opts=''
mount03.c:102: TPASS: mount(2) passed with flag MS_RDONLY: open fail with EROFS as expected : EROFS (30)
mount03.c:112: TBROK: mknod() failed: EPERM (1)

Kind regards,
Petr

[1] https://github.com/linux-test-project/ltp/wiki/C-Test-API#12-basic-test-interface
[2] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines
[3] http://linux-test-project.github.io/metadata/metadata.stable.html

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] mount03: Convert to new API

[Richard Palethorpe]

Hello Petr,

Petr Vorel <pvorel@suse.cz> writes:

> Hi Chen,
>
> quite nice work, but more cleanup is needed.
>
>>  testcases/kernel/syscalls/mount/mount03.c | 462 ++++++++--------------
> ...
>>  #define FILE_MODE	(S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH)
>> -#define DIR_MODE	(S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP| \
>> -			 S_IXGRP|S_IROTH|S_IXOTH)
>>  #define SUID_MODE	(S_ISUID|S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH)
>
> @Richie @Li @Metan: There are checkpatch.pl warnings. Yes, kernel folks does not
> like permission warnings. Do we want to follow? Or should we remove these from
> our checkpatch.pl fork (we use constants in many places)?

I'm not sure about the rule. However there is no reason we can't follow
it for new tests. We don't want to have arbitrary style differences
between LTP and the kernel.

Some rules don't make sense in userland for technical reasons, but this
one is purely about readability.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] mount03: Convert to new API

[Cyril Hrubis]

Hi!
> @Richie @Li @Metan: There are checkpatch.pl warnings. Yes, kernel folks does not
> like permission warnings. Do we want to follow? Or should we remove these from
> our checkpatch.pl fork (we use constants in many places)?
> 
> $ make check-mount03
> mount03.c:29: WARNING: Symbolic permissions 'S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH' are not preferred. Consider using octal permissions '0644'.
> mount03.c:30: WARNING: Symbolic permissions 'S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH' are not preferred. Consider using octal permissions '0511'.
> mount03.c:50: WARNING: static char array declaration should probably be static const char
> mount03.c:103: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.
> mount03.c:114: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.
> mount03.c:125: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.
> mount03.c:181: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.
> mount03.c:204: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.

To be honest I think Linus is right at this one, the single octal number
is way more readable than the bitwise or of four constants, so I would
be inclined to start following the kernel practice here.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] mount03: Convert to new API

[Petr Vorel]

> Hi!
> > @Richie @Li @Metan: There are checkpatch.pl warnings. Yes, kernel folks does not
> > like permission warnings. Do we want to follow? Or should we remove these from
> > our checkpatch.pl fork (we use constants in many places)?

> > $ make check-mount03
> > mount03.c:29: WARNING: Symbolic permissions 'S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH' are not preferred. Consider using octal permissions '0644'.
> > mount03.c:30: WARNING: Symbolic permissions 'S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH' are not preferred. Consider using octal permissions '0511'.
> > mount03.c:50: WARNING: static char array declaration should probably be static const char
> > mount03.c:103: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.
> > mount03.c:114: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.
> > mount03.c:125: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.
> > mount03.c:181: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.
> > mount03.c:204: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.

> To be honest I think Linus is right at this one, the single octal number
> is way more readable than the bitwise or of four constants, so I would
> be inclined to start following the kernel practice here.

Agree with the rule, numbers are indeed much readable and I'm for using it in
LTP source.

My concern was different: aren't these constants part of POSIX? See man from
<sys/stat.h> from 1997 [1]. There might be a test for these constants but
it has much lower priority than tests for new kernel functionality and CVE.

Kind regards,
Petr

[1] https://pubs.opengroup.org/onlinepubs/007908799/xsh/sysstat.h.html

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] mount03: Convert to new API

[xuyang2018.jy]

Hi All

>> Hi!
>>> @Richie @Li @Metan: There are checkpatch.pl warnings. Yes, kernel folks does not
>>> like permission warnings. Do we want to follow? Or should we remove these from
>>> our checkpatch.pl fork (we use constants in many places)?
> 
>>> $ make check-mount03
>>> mount03.c:29: WARNING: Symbolic permissions 'S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH' are not preferred. Consider using octal permissions '0644'.
>>> mount03.c:30: WARNING: Symbolic permissions 'S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH' are not preferred. Consider using octal permissions '0511'.
>>> mount03.c:50: WARNING: static char array declaration should probably be static const char
>>> mount03.c:103: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.
>>> mount03.c:114: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.
>>> mount03.c:125: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.
>>> mount03.c:181: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.
>>> mount03.c:204: WARNING: Symbolic permissions 'S_IRWXU' are not preferred. Consider using octal permissions '0700'.
> 
>> To be honest I think Linus is right at this one, the single octal number
>> is way more readable than the bitwise or of four constants, so I would
>> be inclined to start following the kernel practice here.
> 
> Agree with the rule, numbers are indeed much readable and I'm for using it in
> LTP source.

Linux checkpatch.pl only think perms should use octal number, but not 
inlcude sticky bits, set-uid  bit setgid bit and file types[1].

Should we modify checkpatch.pl to print only three bit instead of four 
bits because setgid bit setuid bit sticky bit are stil keep symbolic names .

[1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v6.0-rc1&id=f90774e1fd2700de4a6e0d62866d34a26c544bd0

Best Regards
Yang Xu

> 
> My concern was different: aren't these constants part of POSIX? See man from
> <sys/stat.h> from 1997 [1]. There might be a test for these constants but
> it has much lower priority than tests for new kernel functionality and CVE.
> 
> Kind regards,
> Petr
> 
> [1] https://pubs.opengroup.org/onlinepubs/007908799/xsh/sysstat.h.html
> 

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] 回复: [PATCH] mount03: Convert to new API

[chenhx.fnst]

> -----邮件原件-----
> 
> Hi Chen,
> 
> quite nice work, but more cleanup is needed.
> 
> >  testcases/kernel/syscalls/mount/mount03.c | 462
> > ++++++++--------------
> ...
> >  #define FILE_MODE	(S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH)
> > -#define DIR_MODE	(S_IRUSR|S_IWUSR|S_IXUSR|S_IRGRP| \
> > -			 S_IXGRP|S_IROTH|S_IXOTH)
> >  #define SUID_MODE	(S_ISUID|S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH)
> 
> @Richie @Li @Metan: There are checkpatch.pl warnings. Yes, kernel folks does
> not like permission warnings. Do we want to follow? Or should we remove these
> from our checkpatch.pl fork (we use constants in many places)?
> 
OK, will be fixed.

> $ make check-mount03
> mount03.c:29: WARNING: Symbolic permissions
> 'S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH' are not preferred. Consider using octal
> permissions '0644'.
> mount03.c:30: WARNING: Symbolic permissions
> 'S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH' are not preferred. Consider using octal
> permissions '0511'.
> mount03.c:50: WARNING: static char array declaration should probably be
> static const char
> mount03.c:103: WARNING: Symbolic permissions 'S_IRWXU' are not preferred.
> Consider using octal permissions '0700'.
> mount03.c:114: WARNING: Symbolic permissions 'S_IRWXU' are not preferred.
> Consider using octal permissions '0700'.
> mount03.c:125: WARNING: Symbolic permissions 'S_IRWXU' are not preferred.
> Consider using octal permissions '0700'.
> mount03.c:181: WARNING: Symbolic permissions 'S_IRWXU' are not preferred.
> Consider using octal permissions '0700'.
> mount03.c:204: WARNING: Symbolic permissions 'S_IRWXU' are not preferred.
> Consider using octal permissions '0700'.
> 
> ...
> > +#define MNTPOINT        "mntpoint"
> ...
> > +static char nobody_uid[] = "nobody";
> I'd remove constant and use "nobody" in the setup (the same like creat08.c).
> If we want to have it defined, I'd use #define (like you use it in MNTPOINT).
> 
> Also your code produces warning:
> mount03.c:50: WARNING: static char array declaration should probably be
> static const char
> 
> ...
> > +static int test_ms_nosuid(void)
> ...
> > +	int pid, status;
> pid_t pid; // don't use int for pid please.
> 
> This is not needed (-1 is handled by SAFE_FORK()), therefore you don't need
> switch.
> 
> For a readability, your code (most of inherited from the original) is:
> static int test_ms_nosuid(void)
> {
> 	int pid, status;
> 
> 	switch (pid = SAFE_FORK()) {
> 	case -1:
> 		tst_brk(TFAIL | TERRNO, "fork failed");
> 		break;
> 	case 0:
> 		SAFE_SETREUID(ltpuser->pw_uid, ltpuser->pw_uid);
> 
> 		execlp(file, basename(file), NULL);
> 		exit(1);
> 	default:
> 		waitpid(pid, &status, 0);
> 		if (WIFEXITED(status)) {
> 			/* reset the setup_uid */
> 			if (status)
> 				return 0;
> 		}
> 		return 1;
> 	}
> 	return 0;
> }
> 
> * I'm not sure why exit(1),
> * we have SAFE_EXECLP. Therefore something like:
> 
> 
> static void test_ms_nosuid(void)
> {
> 	int status;
> 	pid_t pid;
> 
> 	pid = SAFE_FORK();
> 
> 	if (!pid) {
> 		SAFE_SETREUID(ltpuser->pw_uid, ltpuser->pw_uid);
> 		SAFE_EXECLP(file, basename(file), NULL);
> 	}
> 
> 	SAFE_WAITPID(pid, &status, 0);
> }
> 
> But SAFE_WAITPID() would not allow to print TPASS/TFAIL.

Maybe we can use the original waitpid + tst_res logic.

> 
> > -/*
> > - * test_rwflag(int i, int cnt)
> > - * Validate the mount system call for rwflags.
> > - */
> > -int test_rwflag(int i, int cnt)
> > +static void test_rwflag(int i)
> Instead of a crazy long switch which is in the original code, we prefer to use
> functions which are put into struct tcase via function pointer. See e.g.
> mq_open01.c for example.
> 
Thanks for the advice, will be fixed.

> 
> ...
> > +		snprintf(file, PATH_MAX, "%s/mount03_setuid_test", MNTPOINT);
> > +		TEST(test_ms_nosuid());
> Please don't reuse TEST() macro for normal function. Just call it:
> 
> > +		if (TST_RET == 0)
> > +			tst_res(TPASS, "mount(2) passed with flag MS_NOSUID");
> > +		else
> > +			tst_res(TFAIL, "mount(2) failed with flag MS_NOSUID");
> There is TST_EXP_PASS() macro:
> TST_EXP_PASS(test_ms_nosuid())
> if (!TST_PASS)
> 		return;
> 
> But, please don't reuse TEST() macro for normal function. These are meant to
> be for syscalls testing, here I'd print TPASS/TFAIL in test_ms_nosuid() and make
> it void.
> 
OK.

> ...
> > +static void setup(void)
> > +{
> > +	struct stat file_stat = {0};
> 
> > +	ltpuser = getpwnam(nobody_uid);
> ltpuser = SAFE_GETPWNAM("nobody");
> (or use constant, but SAFE_GETPWNAM is required (no check, see creat08.c)
> 
> > +	if (ltpuser == NULL)
> > +		tst_res(TFAIL, "getpwnam failed");
> > +	snprintf(file, PATH_MAX, "%s/mount03_setuid_test", MNTPOINT);
> 
> > +	SAFE_STAT(MNTPOINT, &file_stat);
> >  	if (file_stat.st_mode != SUID_MODE &&
> > -	    chmod(file, SUID_MODE) < 0)
> > -		tst_brkm(TBROK, cleanup,
> > -			 "setuid for setuid_test failed");
> > -	SAFE_UMOUNT(cleanup, mntpoint);
> > -
> > -	TEST_PAUSE;
> > +	    chmod(MNTPOINT, SUID_MODE) < 0)
> > +		tst_res(TFAIL, "setuid for setuid_test failed");
> Please use tst_brk(TBROK, ...), original version has tst_brkm(TBROK, ...) for a
> reason [1]:
> 1) tst_brk quits testing, which is usually required in setup function
> 2) TBROK is usually used instead of TFAIL in setup function
> 
> TBROK: Something has failed in test preparation phase.
> TFAIL: Test has failed.
> 
> (This description should have been in LTP Test Writing Guidelines [2], because it
> applies to both C and shell API.)
> 
Sure, I'll read the docs more carefully.

> 
> ...
> > +static struct tst_test test = {
> > +	.tcnt = ARRAY_SIZE(tcases),
> > +	.test = run,
> > +	.setup = setup,
> > +	.cleanup = cleanup,
> > +	.needs_root = 1,
> > +	.format_device = 1,
> > +	.resource_files = resource_files,
> 
> Please inline resource files like:
> 	.resource_files = (const char *const[]) {
> 		"mount03_setuid_test",
> 		NULL,
> 	},
> 
> It produces "mount03_setuid_test" in doc [3]:
> 
> | .resource_files | mount03_setuid_test |
> 
> But using .resource_files = resource_files produces just "resource_files":
> | .resource_files | resource_files |
> 
> (to make doc locally: cd metadata && make # result is
> in ../docparse/metadata.html)
> 
> > +	.forks_child = 1,
> > +	.mntpoint = MNTPOINT,
> > +	.skip_filesystems = (const char *const []){"fuse", NULL},
> Why you introduced skipping on FUSE? I don't see it in the old code, nothing
> mentioned in commit message).
> 
> Also when I test it for all filesystems (which could be a good idea for mount
> test) it fails for vfat, exfat and ntfs:

My test machine(Centos Stream) ntfs is provided by fuse, so it was skipped...
I'll add them to skip list.

Regards, 
- Chen

> 
> 	.skip_filesystems = (const char *const []){"vfat", "exfat", "ntfs", NULL},
> 	.all_filesystems = 1,
> 
> Skipping just fuse does not help in my case:
> 
> tst_test.c:1599: TINFO: Testing on ntfs
> tst_test.c:1064: TINFO: Formatting /dev/loop3 with ntfs opts='' extra opts=''
> The partition start sector was not specified for /dev/loop3 and it could not be
> obtained automatically.  It has been set to 0.
> The number of sectors per track was not specified for /dev/loop3 and it could
> not be obtained automatically.  It has been set to 0.
> The number of heads was not specified for /dev/loop3 and it could not be
> obtained automatically.  It has been set to 0.
> To boot from a device, Windows needs the 'partition start sector', the 'sectors
> per track' and the 'number of heads' to be set.
> Windows will not be able to boot from this device.
> mount03.c:102: TFAIL: mount(2) passed with flag MS_RDONLY: open fail with
> EROFS as expected succeeded
> tst_device.c:394: TWARN: umount('mntpoint') failed with EINVAL
> mount03.c:237: TBROK: umount(mntpoint) failed: EINVAL (22)
> 
> tst_test.c:1599: TINFO: Testing on exfat
> tst_test.c:1064: TINFO: Formatting /dev/loop2 with exfat opts='' extra opts=''
> mount03.c:102: TPASS: mount(2) passed with flag MS_RDONLY: open fail with
> EROFS as expected : EROFS (30)
> mount03.c:112: TBROK: mknod() failed: EPERM (1)
> 
> tst_test.c:1599: TINFO: Testing on vfat
> tst_test.c:1064: TINFO: Formatting /dev/loop1 with vfat opts='' extra opts=''
> mount03.c:102: TPASS: mount(2) passed with flag MS_RDONLY: open fail with
> EROFS as expected : EROFS (30)
> mount03.c:112: TBROK: mknod() failed: EPERM (1)
> 
> Kind regards,
> Petr
> 
> [1]
> https://github.com/linux-test-project/ltp/wiki/C-Test-API#12-basic-test-interfac
> e
> [2] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines
> [3] http://linux-test-project.github.io/metadata/metadata.stable.html

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp