[LTP] [PATCH v4 0/2] Fix unlink09 test

[LTP] [PATCH v4 0/2] Fix unlink09 test

[Andrea Cervesato]

This will fix the 2cf78f47a6 and resolve issues on filesystems
which are not supporting inode attributes. Split unlink09 read-only
check and move it into unlink10.

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
Changes in v4:
- added exfat and fuse to .skip_filesystems
- Link to v3: https://lore.kernel.org/r/20240606-unlink09-v3-0-37cc612ce0fd@suse.com

Changes in v3:
- added .mount_device = 1
- added .skip_filesystems (vfat and ntfs)
- skip test execution when the tmpfs is used on kernel<6.0
- Link to v2: https://lore.kernel.org/r/20240605-unlink09-v2-0-bfa0bbdc8753@suse.com

Changes in v2:
- moved read-only FS check out of unlink09 and created unlink10
- check when FS doesn't support inode attributes and print a TBROK
- simplified inode attributes setup by adding a common function
- added .all_filesystems support
- Link to v1: https://lore.kernel.org/r/20240604-unlink09-v1-1-dfd8e3e1cb2b@suse.com

---
Andrea Cervesato (2):
      Fix unlink09 test
      Add unlink10 test

 runtest/syscalls                            |   1 +
 testcases/kernel/syscalls/unlink/.gitignore |   1 +
 testcases/kernel/syscalls/unlink/unlink09.c | 108 ++++++++++++++++------------
 testcases/kernel/syscalls/unlink/unlink10.c |  33 +++++++++
 4 files changed, 99 insertions(+), 44 deletions(-)
---
base-commit: 072f359ac54395af1d5ade4ca8cc347c5afe6310
change-id: 20240604-unlink09-dc4802f872f9

Best regards,
-- 
Andrea Cervesato <andrea.cervesato@suse.com>


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

[LTP] [PATCH v4 1/2] Fix unlink09 test

[Andrea Cervesato]

From: Andrea Cervesato <andrea.cervesato@suse.com>

This patch will fix unlink09 test by checking for filesystems which
are not supporting inode attributes. At the same time, it removes the
read-only filesystem unlink() check in order to add .all_filesystems
support.

Fixes: 2cf78f47a6 (unlink: Add error tests for EPERM and EROFS)
Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
 testcases/kernel/syscalls/unlink/unlink09.c | 108 ++++++++++++++++------------
 1 file changed, 64 insertions(+), 44 deletions(-)

diff --git a/testcases/kernel/syscalls/unlink/unlink09.c b/testcases/kernel/syscalls/unlink/unlink09.c
index cc4b4a07e..5e438569f 100644
--- a/testcases/kernel/syscalls/unlink/unlink09.c
+++ b/testcases/kernel/syscalls/unlink/unlink09.c
@@ -2,92 +2,103 @@
 /*
  * Copyright (c) 2024 FUJITSU LIMITED. All Rights Reserved.
  * Author: Yang Xu <xuyang2018.jy@fujitsu.com>
+ * Copyright (C) 2024 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
  */
 
 /*\
  * [Description]
  *
- * Verify that unlink(2) fails with
- *
- * - EPERM when target file is marked as immutable or append-only
- * - EROFS when target file is on a read-only filesystem.
+ * Verify that unlink(2) fails with EPERM when target file is marked as
+ * immutable or append-only.
  */
 
 #include <sys/ioctl.h>
 #include "tst_test.h"
 #include "lapi/fs.h"
 
-#define TEST_EPERM_IMMUTABLE "test_eperm_immutable"
-#define TEST_EPERM_APPEND_ONLY "test_eperm_append_only"
-#define DIR_EROFS "erofs"
-#define TEST_EROFS "erofs/test_erofs"
+#define MNTPOINT "mnt"
+#define TEST_EPERM_IMMUTABLE MNTPOINT"/test_eperm_immutable"
+#define TEST_EPERM_APPEND_ONLY MNTPOINT"/test_eperm_append_only"
 
-static int fd_immutable;
-static int fd_append_only;
+static int fd_immutable = -1;
+static int fd_append_only = -1;
 
 static struct test_case_t {
 	char *filename;
 	int *fd;
 	int flag;
-	int expected_errno;
 	char *desc;
 } tcases[] = {
-	{TEST_EPERM_IMMUTABLE, &fd_immutable, FS_IMMUTABLE_FL, EPERM,
+	{TEST_EPERM_IMMUTABLE, &fd_immutable, FS_IMMUTABLE_FL,
 		"target file is immutable"},
-	{TEST_EPERM_APPEND_ONLY, &fd_append_only, FS_APPEND_FL, EPERM,
+	{TEST_EPERM_APPEND_ONLY, &fd_append_only, FS_APPEND_FL,
 		"target file is append-only"},
-	{TEST_EROFS, NULL, 0, EROFS, "target file in read-only filesystem"},
 };
 
+static void setup_inode_flag(const int fd, const int flag, const int reset)
+{
+	int attr;
+
+	SAFE_IOCTL(fd, FS_IOC_GETFLAGS, &attr);
+
+	if (reset)
+		attr &= ~flag;
+	else
+		attr |= flag;
+
+	SAFE_IOCTL(fd, FS_IOC_SETFLAGS, &attr);
+}
+
 static void setup(void)
 {
 	int attr;
 
-	fd_immutable = SAFE_OPEN(TEST_EPERM_IMMUTABLE, O_CREAT, 0600);
-	SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr);
+	/* inode attributes in tmpfs are supported from kernel 6.0
+	 * https://lore.kernel.org/all/20220715015912.2560575-1-tytso@mit.edu/
+	 */
+	if (!strcmp(tst_device->fs_type, "tmpfs") && tst_kvercmp(6, 0, 0) < 0)
+		tst_brk(TCONF, "FS_IOC_GETFLAGS on tmpfs not supported for kernel<6.0");
+
+	fd_immutable = SAFE_CREAT(TEST_EPERM_IMMUTABLE, 0600);
+	TEST(ioctl(fd_immutable, FS_IOC_GETFLAGS, &attr));
+
+	if (TST_RET == -1 && TST_ERR == ENOTTY) {
+		SAFE_CLOSE(fd_immutable);
+
+		tst_brk(TBROK, "Inode attributes not supported by '%s'",
+			tst_device->fs_type);
+	}
+
 	attr |= FS_IMMUTABLE_FL;
 	SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr);
 
-	fd_append_only = SAFE_OPEN(TEST_EPERM_APPEND_ONLY, O_CREAT, 0600);
-	SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr);
-	attr |= FS_APPEND_FL;
-	SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr);
+	fd_append_only = SAFE_CREAT(TEST_EPERM_APPEND_ONLY, 0600);
+	setup_inode_flag(fd_append_only, FS_APPEND_FL, 0);
 }
 
 static void cleanup(void)
 {
-	int attr;
-
-	SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr);
-	attr &= ~FS_IMMUTABLE_FL;
-	SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr);
-	SAFE_CLOSE(fd_immutable);
+	if (fd_immutable != -1) {
+		setup_inode_flag(fd_immutable, FS_IMMUTABLE_FL, 1);
+		SAFE_CLOSE(fd_immutable);
+	}
 
-	SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr);
-	attr &= ~FS_APPEND_FL;
-	SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr);
-	SAFE_CLOSE(fd_append_only);
+	if (fd_append_only != -1) {
+		setup_inode_flag(fd_append_only, FS_APPEND_FL, 1);
+		SAFE_CLOSE(fd_append_only);
+	}
 }
 
 static void verify_unlink(unsigned int i)
 {
 	struct test_case_t *tc = &tcases[i];
-	int attr;
 
-	TST_EXP_FAIL(unlink(tc->filename), tc->expected_errno, "%s", tc->desc);
+	TST_EXP_FAIL(unlink(tc->filename), EPERM, "%s", tc->desc);
 
 	/* If unlink() succeeded unexpectedly, test file should be restored. */
 	if (!TST_RET) {
-		if (tc->fd) {
-			*(tc->fd) = SAFE_OPEN(tc->filename, O_CREAT, 0600);
-			if (tc->flag) {
-				SAFE_IOCTL(*(tc->fd), FS_IOC_GETFLAGS, &attr);
-				attr |= tc->flag;
-				SAFE_IOCTL(*(tc->fd), FS_IOC_SETFLAGS, &attr);
-			}
-		} else {
-			SAFE_TOUCH(tc->filename, 0600, 0);
-		}
+		*(tc->fd) = SAFE_CREAT(tc->filename, 0600);
+		setup_inode_flag(*(tc->fd), tc->flag, 0);
 	}
 }
 
@@ -96,7 +107,16 @@ static struct tst_test test = {
 	.tcnt = ARRAY_SIZE(tcases),
 	.cleanup = cleanup,
 	.test = verify_unlink,
-	.needs_rofs = 1,
-	.mntpoint = DIR_EROFS,
+	.mntpoint = MNTPOINT,
 	.needs_root = 1,
+	.format_device = 1,
+	.mount_device = 1,
+	.all_filesystems = 1,
+	.skip_filesystems = (const char *const[]) {
+		"fuse",
+		"exfat",
+		"vfat",
+		"ntfs",
+		NULL
+	},
 };

-- 
2.43.0


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

[LTP] [PATCH v4 2/2] Add unlink10 test

[Andrea Cervesato]

From: Andrea Cervesato <andrea.cervesato@suse.com>

This test verifies that unlink(2) fails with EROFS when target file
is on a read-only filesystem.

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
 runtest/syscalls                            |  1 +
 testcases/kernel/syscalls/unlink/.gitignore |  1 +
 testcases/kernel/syscalls/unlink/unlink10.c | 33 +++++++++++++++++++++++++++++
 3 files changed, 35 insertions(+)

diff --git a/runtest/syscalls b/runtest/syscalls
index 44a577db3..b6cadb2df 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -1658,6 +1658,7 @@ unlink05 unlink05
 unlink07 unlink07
 unlink08 unlink08
 unlink09 unlink09
+unlink10 unlink10
 
 #unlinkat test cases
 unlinkat01 unlinkat01
diff --git a/testcases/kernel/syscalls/unlink/.gitignore b/testcases/kernel/syscalls/unlink/.gitignore
index 6038cc29d..4fc24059a 100644
--- a/testcases/kernel/syscalls/unlink/.gitignore
+++ b/testcases/kernel/syscalls/unlink/.gitignore
@@ -2,3 +2,4 @@
 /unlink07
 /unlink08
 /unlink09
+/unlink10
diff --git a/testcases/kernel/syscalls/unlink/unlink10.c b/testcases/kernel/syscalls/unlink/unlink10.c
new file mode 100644
index 000000000..861f24a50
--- /dev/null
+++ b/testcases/kernel/syscalls/unlink/unlink10.c
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2024 FUJITSU LIMITED. All Rights Reserved.
+ * Author: Yang Xu <xuyang2018.jy@fujitsu.com>
+ * Copyright (C) 2024 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * Verify that unlink(2) fails with EROFS when target file is on a read-only
+ * filesystem.
+ */
+
+#include <sys/ioctl.h>
+#include "tst_test.h"
+#include "lapi/fs.h"
+
+#define MNTPOINT "erofs"
+#define FILENAME MNTPOINT"/file"
+
+static void run(void)
+{
+	TST_EXP_FAIL(unlink(FILENAME), EROFS,
+		"%s", "target file in read-only filesystem");
+}
+
+static struct tst_test test = {
+	.test_all = run,
+	.needs_rofs = 1,
+	.needs_root = 1,
+	.mntpoint = MNTPOINT,
+};

-- 
2.43.0


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

Re: [LTP] [PATCH v4 1/2] Fix unlink09 test

[Cyril Hrubis]

Hi!
> +static void setup_inode_flag(const int fd, const int flag, const int reset)

I find it a bit strange when we pass reset flag, since I would expect
that if we pass non-zero to this function the flag would be set not
reset.

If you are find with it I will change the reset flag in to set flag and
reverse the logic before pushing the changes...

> +{
> +	int attr;
> +
> +	SAFE_IOCTL(fd, FS_IOC_GETFLAGS, &attr);
> +
> +	if (reset)
> +		attr &= ~flag;
> +	else
> +		attr |= flag;
> +
> +	SAFE_IOCTL(fd, FS_IOC_SETFLAGS, &attr);
> +}

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v4 1/2] Fix unlink09 test

[Andrea Cervesato via ltp]

Hi,

feel free to add the modification.

On 7/2/24 15:57, Cyril Hrubis wrote:
> Hi!
>> +static void setup_inode_flag(const int fd, const int flag, const int reset)
> I find it a bit strange when we pass reset flag, since I would expect
> that if we pass non-zero to this function the flag would be set not
> reset.
>
> If you are find with it I will change the reset flag in to set flag and
> reverse the logic before pushing the changes...
>
>> +{
>> +	int attr;
>> +
>> +	SAFE_IOCTL(fd, FS_IOC_GETFLAGS, &attr);
>> +
>> +	if (reset)
>> +		attr &= ~flag;
>> +	else
>> +		attr |= flag;
>> +
>> +	SAFE_IOCTL(fd, FS_IOC_SETFLAGS, &attr);
>> +}

Andrea


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

Re: [LTP] [PATCH v4 1/2] Fix unlink09 test

[Cyril Hrubis]

Hi!
Pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

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