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

[LTP] [PATCH v2 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 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 | 94 +++++++++++++++--------------
 testcases/kernel/syscalls/unlink/unlink10.c | 33 ++++++++++
 4 files changed, 85 insertions(+), 44 deletions(-)
---
base-commit: 66517b89141fc455ed807f3b95e5260dcf9fb90f
change-id: 20240604-unlink09-dc4802f872f9

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


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

[LTP] [PATCH v2 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 | 94 +++++++++++++++--------------
 1 file changed, 50 insertions(+), 44 deletions(-)

diff --git a/testcases/kernel/syscalls/unlink/unlink09.c b/testcases/kernel/syscalls/unlink/unlink09.c
index cc4b4a07e..220a810cf 100644
--- a/testcases/kernel/syscalls/unlink/unlink09.c
+++ b/testcases/kernel/syscalls/unlink/unlink09.c
@@ -2,92 +2,97 @@
 /*
  * 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);
+	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 +101,8 @@ 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,
+	.all_filesystems = 1,
+	.format_device = 1,
 };

-- 
2.35.3


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

[LTP] [PATCH v2 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 cf06ee563..b59b64314 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -1655,6 +1655,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.35.3


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

Re: [LTP] [PATCH v2 2/2] Add unlink10 test

[Wei Gao via ltp]

On Wed, Jun 05, 2024 at 04:40:39PM +0200, Andrea Cervesato wrote:
> 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 cf06ee563..b59b64314 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -1655,6 +1655,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,
> +};
Reviewed-by: Wei Gao <wegao@suse.com>
> 
> -- 
> 2.35.3
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

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

Re: [LTP] [PATCH v2 2/2] Add unlink10 test

[Wei Gao via ltp]

On Wed, Jun 05, 2024 at 04:40:39PM +0200, Andrea Cervesato wrote:
> 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 cf06ee563..b59b64314 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -1655,6 +1655,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,
> +};
Reviewed-by: Wei Gao <wegao@suse.com>
> 
> -- 
> 2.35.3
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

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

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

[Petr Vorel]

Hi Andrea,

> 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.

...
> -	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);

FYI, when forcing NTFS, we get EINVAL, reported by Avinesh:

LTP_SINGLE_FS_TYPE=ntfs ./unlink09
tst_test.c:1120: TINFO: Mounting /dev/loop2 to /tmp/LTP_unlqPsIkB/mnt fstyp=ntfs flags=0
tst_test.c:1120: TINFO: Trying FUSE...
unlink09.c:73: TBROK: ioctl(3,((((1U) << (((0+8)+8)+14)) | ((('f')) << (0+8)) | (((2)) << 0) | ((((sizeof(long)))) << ((0+8)+8)))),...) failed: EINVAL (22)
unlink09.c:42: TWARN: ioctl(3,((((2U) << (((0+8)+8)+14)) | ((('f')) << (0+8)) | (((1)) << 0) | ((((sizeof(long)))) << ((0+8)+8)))),...) failed: EINVAL (22)
unlink09.c:49: TWARN: ioctl(3,((((1U) << (((0+8)+8)+14)) | ((('f')) << (0+8)) | (((2)) << 0) | ((((sizeof(long)))) << ((0+8)+8)))),...) failed: EINVAL (22)

I tried SAFE_OPEN with O_RDWR | O_CREAT (patch from Avinesh), but it did not help.
https://lore.kernel.org/ltp/20240603124653.31967-1-akumar@suse.de/
https://patchwork.ozlabs.org/project/ltp/patch/20240601195149.17570-1-akumar@suse.de/

It's not ENOTTY, thus it's not checked by inode attributes check.

> +		setup_inode_flag(*(tc->fd), tc->flag, 0);
>  	}
>  }

> @@ -96,7 +101,8 @@ 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,
> +	.all_filesystems = 1,
> +	.format_device = 1,

Test fails on exfat (on various kernel versions):

tst_test.c:1694: TINFO: === Testing on exfat ===
tst_test.c:1107: TINFO: Formatting /dev/loop0 with exfat opts='' extra opts=''
tst_test.c:1121: TINFO: Mounting /dev/loop0 to /tmp/LTP_unl7cHxvL/mnt fstyp=exfat flags=0
unlink09.c:69: TBROK: Inode attributes not supported by 'exfat'

i.e. different error than on ntfs, but the same as on vfat (which is also
disabled). I wonder why NTFS is different. Or is it fuse relevant?

With added exfat LGTM (I can change this before merge.
Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

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

Re: [LTP] [PATCH v2 2/2] Add unlink10 test

[Petr Vorel]

Hi Andrea, Wei,

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

Test LGTM, but I would prefer to merge it together with unlink09.c change (first
patch).

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

Kind regards,
Petr

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