[LTP] [PATCH v5 0/5] Add fchmodat2 testing suite

[LTP] [PATCH v5 0/5] Add fchmodat2 testing suite

[Andrea Cervesato]

This is a patch-set that implements fchmodat2() syscall coverage.
fchmodat2() has been added in kernel 6.6 in order to support
AT_SYMLINK_NOFOLLOW and AT_EMPTY_PATH in fchmodat().
There's no man pages yet, so please take the following links as
main documentation along with kernel source code:

https://www.phoronix.com/news/fchmodat2-For-Linux-6.6
https://lore.kernel.org/lkml/20230824-frohlocken-vorabend-725f6fdaad50@brauner/

***********
* WARNING *
***********

fchmodat2_02 fails with EOPNOTSUPP because of missing feature.
According to documentation, the feature has been implemented in
kernel 6.6, but __in reality__ AT_SYMLINK_NOFOLLOW is not working
on symbolic files. Also kselftests, which are meant to test the
functionality, are not working and they are treating fchmodat2()
syscall failure as SKIP. Please take a look at the following code
before reviewing:

https://github.com/torvalds/linux/blob/8f6a15f095a63a83b096d9b29aaff4f0fbe6f6e6/tools/testing/selftests/fchmodat2/fchmodat2_test.c#L123

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
Changes in v5:
- fchmodat2_01: check for EOPNOTSUPP, added tag and removed kernel
  version in order to let backported feature to be available for testing
- Link to v4: https://lore.kernel.org/r/20240801-fchmodat2-v4-0-7f2f11a53a09@suse.com

Changes in v4:
- add SAFE_FCHMODAT2
- Link to v3: https://lore.kernel.org/r/20240724-fchmodat2-v3-0-1dc7cfc634b8@suse.com

Changes in v3:
- removed fchmodat2.h
- Link to v2: https://lore.kernel.org/r/20240723-fchmodat2-v2-0-e658a98b113e@suse.com

Changes in v2:
- merge first 3 tests into a unique one
- move fchmodat2 in lapi/stat.h
- add test for error checking
- Link to v1: https://lore.kernel.org/r/20240521-fchmodat2-v1-0-191b4a986202@suse.com

---
Andrea Cervesato (5):
      Add SAFE_SYMLINKAT macro
      Add fchmodat2 syscalls definitions
      Add fchmodat2 fallback definition
      Add fchmodat2_01 test
      Add fchmodat2_02 test

 include/lapi/stat.h                                |  16 +++
 include/lapi/syscalls/aarch64.in                   |   1 +
 include/lapi/syscalls/arc.in                       |   1 +
 include/lapi/syscalls/arm.in                       |   1 +
 include/lapi/syscalls/hppa.in                      |   1 +
 include/lapi/syscalls/i386.in                      |   1 +
 include/lapi/syscalls/ia64.in                      |   1 +
 include/lapi/syscalls/loongarch.in                 |   1 +
 include/lapi/syscalls/mips_n32.in                  |   1 +
 include/lapi/syscalls/mips_n64.in                  |   1 +
 include/lapi/syscalls/mips_o32.in                  |   1 +
 include/lapi/syscalls/powerpc.in                   |   1 +
 include/lapi/syscalls/powerpc64.in                 |   1 +
 include/lapi/syscalls/s390.in                      |   1 +
 include/lapi/syscalls/s390x.in                     |   1 +
 include/lapi/syscalls/sh.in                        |   1 +
 include/lapi/syscalls/sparc.in                     |   1 +
 include/lapi/syscalls/sparc64.in                   |   1 +
 include/lapi/syscalls/x86_64.in                    |   1 +
 include/safe_macros_fn.h                           |   4 +
 include/tst_safe_macros.h                          |   3 +
 lib/safe_macros.c                                  |  20 ++++
 runtest/syscalls                                   |   3 +
 testcases/kernel/syscalls/fchmodat2/.gitignore     |   2 +
 testcases/kernel/syscalls/fchmodat2/Makefile       |   7 ++
 testcases/kernel/syscalls/fchmodat2/fchmodat2_01.c | 123 +++++++++++++++++++++
 testcases/kernel/syscalls/fchmodat2/fchmodat2_02.c |  68 ++++++++++++
 27 files changed, 264 insertions(+)
---
base-commit: 8422d4680b21e6576da63c677b5d49f46b477df0
change-id: 20240517-fchmodat2-5b82867d71fc

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


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

[LTP] [PATCH v5 1/5] Add SAFE_SYMLINKAT macro

[Andrea Cervesato]

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

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
 include/safe_macros_fn.h  |  4 ++++
 include/tst_safe_macros.h |  3 +++
 lib/safe_macros.c         | 20 ++++++++++++++++++++
 3 files changed, 27 insertions(+)

diff --git a/include/safe_macros_fn.h b/include/safe_macros_fn.h
index d256091b7..6d9e72e4f 100644
--- a/include/safe_macros_fn.h
+++ b/include/safe_macros_fn.h
@@ -122,6 +122,10 @@ int safe_symlink(const char *file, const int lineno,
                  void (cleanup_fn)(void), const char *oldpath,
                  const char *newpath);
 
+int safe_symlinkat(const char *file, const int lineno,
+                 void (cleanup_fn)(void), const char *oldpath,
+                 const int newdirfd, const char *newpath);
+
 ssize_t safe_write(const char *file, const int lineno,
 		   void (cleanup_fn)(void), enum safe_write_opts len_strict,
 		   int fildes, const void *buf, size_t nbyte);
diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h
index 92b9bc119..47a6f99df 100644
--- a/include/tst_safe_macros.h
+++ b/include/tst_safe_macros.h
@@ -191,6 +191,9 @@ int safe_getgroups(const char *file, const int lineno, int size, gid_t list[]);
 #define SAFE_SYMLINK(oldpath, newpath) \
 	safe_symlink(__FILE__, __LINE__, NULL, (oldpath), (newpath))
 
+#define SAFE_SYMLINKAT(oldpath, newdirfd, newpath) \
+	safe_symlinkat(__FILE__, __LINE__, NULL, (oldpath), (newdirfd), (newpath))
+
 #define SAFE_WRITE(len_strict, fildes, buf, nbyte) \
 	safe_write(__FILE__, __LINE__, NULL, (len_strict), (fildes), (buf), (nbyte))
 
diff --git a/lib/safe_macros.c b/lib/safe_macros.c
index 109268587..f91e77022 100644
--- a/lib/safe_macros.c
+++ b/lib/safe_macros.c
@@ -527,6 +527,26 @@ int safe_symlink(const char *file, const int lineno,
 	return rval;
 }
 
+int safe_symlinkat(const char *file, const int lineno,
+                 void (cleanup_fn)(void), const char *oldpath,
+				 const int newdirfd, const char *newpath)
+{
+	int rval;
+
+	rval = symlinkat(oldpath, newdirfd, newpath);
+
+	if (rval == -1) {
+		tst_brkm_(file, lineno, TBROK | TERRNO, cleanup_fn,
+			"symlinkat(%s,%d,%s) failed", oldpath, newdirfd, newpath);
+	} else if (rval) {
+		tst_brkm_(file, lineno, TBROK | TERRNO, cleanup_fn,
+			"Invalid symlinkat(%s,%d,%s) return value %d", oldpath,
+			newdirfd, newpath, rval);
+	}
+
+	return rval;
+}
+
 ssize_t safe_write(const char *file, const int lineno, void (cleanup_fn) (void),
 		   enum safe_write_opts len_strict, int fildes, const void *buf,
 		   size_t nbyte)

-- 
2.43.0


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

[LTP] [PATCH v5 2/5] Add fchmodat2 syscalls definitions

[Andrea Cervesato]

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

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
 include/lapi/syscalls/aarch64.in   | 1 +
 include/lapi/syscalls/arc.in       | 1 +
 include/lapi/syscalls/arm.in       | 1 +
 include/lapi/syscalls/hppa.in      | 1 +
 include/lapi/syscalls/i386.in      | 1 +
 include/lapi/syscalls/ia64.in      | 1 +
 include/lapi/syscalls/loongarch.in | 1 +
 include/lapi/syscalls/mips_n32.in  | 1 +
 include/lapi/syscalls/mips_n64.in  | 1 +
 include/lapi/syscalls/mips_o32.in  | 1 +
 include/lapi/syscalls/powerpc.in   | 1 +
 include/lapi/syscalls/powerpc64.in | 1 +
 include/lapi/syscalls/s390.in      | 1 +
 include/lapi/syscalls/s390x.in     | 1 +
 include/lapi/syscalls/sh.in        | 1 +
 include/lapi/syscalls/sparc.in     | 1 +
 include/lapi/syscalls/sparc64.in   | 1 +
 include/lapi/syscalls/x86_64.in    | 1 +
 18 files changed, 18 insertions(+)

diff --git a/include/lapi/syscalls/aarch64.in b/include/lapi/syscalls/aarch64.in
index ef0aa04a3..31ea5a8c2 100644
--- a/include/lapi/syscalls/aarch64.in
+++ b/include/lapi/syscalls/aarch64.in
@@ -301,4 +301,5 @@ landlock_add_rule 445
 landlock_restrict_self 446
 futex_waitv 449
 cachestat 451
+fchmodat2 452
 _sysctl 1078
diff --git a/include/lapi/syscalls/arc.in b/include/lapi/syscalls/arc.in
index 3eaa6a8f1..5a00c6cf7 100644
--- a/include/lapi/syscalls/arc.in
+++ b/include/lapi/syscalls/arc.in
@@ -321,3 +321,4 @@ landlock_add_rule 445
 landlock_restrict_self 446
 futex_waitv 449
 cachestat 451
+fchmodat2 452
diff --git a/include/lapi/syscalls/arm.in b/include/lapi/syscalls/arm.in
index b52a32b6b..41b6fe733 100644
--- a/include/lapi/syscalls/arm.in
+++ b/include/lapi/syscalls/arm.in
@@ -400,3 +400,4 @@ landlock_restrict_self (__NR_SYSCALL_BASE+446)
 memfd_secret (__NR_SYSCALL_BASE+447)
 futex_waitv (__NR_SYSCALL_BASE+449)
 cachestat (__NR_SYSCALL_BASE+451)
+fchmodat2 (__NR_SYSCALL_BASE+452)
diff --git a/include/lapi/syscalls/hppa.in b/include/lapi/syscalls/hppa.in
index 4919ee65d..d8429a4c1 100644
--- a/include/lapi/syscalls/hppa.in
+++ b/include/lapi/syscalls/hppa.in
@@ -48,3 +48,4 @@ landlock_add_rule 445
 landlock_restrict_self 446
 futex_waitv 449
 cachestat 451
+fchmodat2 452
diff --git a/include/lapi/syscalls/i386.in b/include/lapi/syscalls/i386.in
index cff40957a..facb3824a 100644
--- a/include/lapi/syscalls/i386.in
+++ b/include/lapi/syscalls/i386.in
@@ -435,3 +435,4 @@ landlock_add_rule 445
 landlock_restrict_self 446
 futex_waitv 449
 cachestat 451
+fchmodat2 452
diff --git a/include/lapi/syscalls/ia64.in b/include/lapi/syscalls/ia64.in
index 11d4b46f4..9aeb0f99d 100644
--- a/include/lapi/syscalls/ia64.in
+++ b/include/lapi/syscalls/ia64.in
@@ -348,3 +348,4 @@ landlock_add_rule 1469
 landlock_restrict_self 1470
 futex_waitv 1473
 cachestat 1475
+fchmodat2 1476
diff --git a/include/lapi/syscalls/loongarch.in b/include/lapi/syscalls/loongarch.in
index 9bf6a7deb..edda29f75 100644
--- a/include/lapi/syscalls/loongarch.in
+++ b/include/lapi/syscalls/loongarch.in
@@ -306,3 +306,4 @@ process_mrelease 448
 futex_waitv 449
 set_mempolicy_home_node 450
 cachestat 451
+fchmodat2 452
diff --git a/include/lapi/syscalls/mips_n32.in b/include/lapi/syscalls/mips_n32.in
index a76c82593..039e674b9 100644
--- a/include/lapi/syscalls/mips_n32.in
+++ b/include/lapi/syscalls/mips_n32.in
@@ -375,3 +375,4 @@ landlock_add_rule 6445
 landlock_restrict_self 6446
 futex_waitv 6449
 cachestat 6451
+fchmodat2 6452
diff --git a/include/lapi/syscalls/mips_n64.in b/include/lapi/syscalls/mips_n64.in
index df991efd5..778979e1c 100644
--- a/include/lapi/syscalls/mips_n64.in
+++ b/include/lapi/syscalls/mips_n64.in
@@ -351,3 +351,4 @@ landlock_add_rule 5445
 landlock_restrict_self 5446
 futex_waitv 5449
 cachestat 5451
+fchmodat2 5452
diff --git a/include/lapi/syscalls/mips_o32.in b/include/lapi/syscalls/mips_o32.in
index 826b7d66e..11a6415a2 100644
--- a/include/lapi/syscalls/mips_o32.in
+++ b/include/lapi/syscalls/mips_o32.in
@@ -421,3 +421,4 @@ landlock_add_rule 4445
 landlock_restrict_self 4446
 futex_waitv 4449
 cachestat 4451
+fchmodat2 4452
diff --git a/include/lapi/syscalls/powerpc.in b/include/lapi/syscalls/powerpc.in
index 798ed9050..ad05235c1 100644
--- a/include/lapi/syscalls/powerpc.in
+++ b/include/lapi/syscalls/powerpc.in
@@ -428,3 +428,4 @@ landlock_add_rule 445
 landlock_restrict_self 446
 futex_waitv 449
 cachestat 451
+fchmodat2 452
diff --git a/include/lapi/syscalls/powerpc64.in b/include/lapi/syscalls/powerpc64.in
index 798ed9050..ad05235c1 100644
--- a/include/lapi/syscalls/powerpc64.in
+++ b/include/lapi/syscalls/powerpc64.in
@@ -428,3 +428,4 @@ landlock_add_rule 445
 landlock_restrict_self 446
 futex_waitv 449
 cachestat 451
+fchmodat2 452
diff --git a/include/lapi/syscalls/s390.in b/include/lapi/syscalls/s390.in
index 126938095..cdfdb670e 100644
--- a/include/lapi/syscalls/s390.in
+++ b/include/lapi/syscalls/s390.in
@@ -415,3 +415,4 @@ landlock_add_rule 445
 landlock_restrict_self 446
 futex_waitv 449
 cachestat 451
+fchmodat2 452
diff --git a/include/lapi/syscalls/s390x.in b/include/lapi/syscalls/s390x.in
index 18f2496a0..a9e6fa707 100644
--- a/include/lapi/syscalls/s390x.in
+++ b/include/lapi/syscalls/s390x.in
@@ -363,3 +363,4 @@ landlock_add_rule 445
 landlock_restrict_self 446
 futex_waitv 449
 cachestat 451
+fchmodat2 452
diff --git a/include/lapi/syscalls/sh.in b/include/lapi/syscalls/sh.in
index ae6f26050..3c0a927fd 100644
--- a/include/lapi/syscalls/sh.in
+++ b/include/lapi/syscalls/sh.in
@@ -409,3 +409,4 @@ landlock_add_rule 445
 landlock_restrict_self 446
 futex_waitv 449
 cachestat 451
+fchmodat2 452
diff --git a/include/lapi/syscalls/sparc.in b/include/lapi/syscalls/sparc.in
index 409fa2729..df77f5688 100644
--- a/include/lapi/syscalls/sparc.in
+++ b/include/lapi/syscalls/sparc.in
@@ -414,3 +414,4 @@ landlock_add_rule 445
 landlock_restrict_self 446
 futex_waitv 449
 cachestat 451
+fchmodat2 452
diff --git a/include/lapi/syscalls/sparc64.in b/include/lapi/syscalls/sparc64.in
index e13cf163e..860613684 100644
--- a/include/lapi/syscalls/sparc64.in
+++ b/include/lapi/syscalls/sparc64.in
@@ -379,3 +379,4 @@ landlock_add_rule 445
 landlock_restrict_self 446
 futex_waitv 449
 cachestat 451
+fchmodat2 452
diff --git a/include/lapi/syscalls/x86_64.in b/include/lapi/syscalls/x86_64.in
index 05b1bee55..477b08510 100644
--- a/include/lapi/syscalls/x86_64.in
+++ b/include/lapi/syscalls/x86_64.in
@@ -356,6 +356,7 @@ landlock_add_rule 445
 landlock_restrict_self 446
 futex_waitv 449
 cachestat 451
+fchmodat2 452
 rt_sigaction 512
 rt_sigreturn 513
 ioctl 514

-- 
2.43.0


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

[LTP] [PATCH v5 3/5] Add fchmodat2 fallback definition

[Andrea Cervesato]

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

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
 include/lapi/stat.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/lapi/stat.h b/include/lapi/stat.h
index 3606c9eb0..8e38ecfef 100644
--- a/include/lapi/stat.h
+++ b/include/lapi/stat.h
@@ -229,4 +229,20 @@ static inline int statx(int dirfd, const char *pathname, unsigned int flags,
 # define STATX_ATTR_VERITY	0x00100000
 #endif
 
+#define SAFE_FCHMODAT2(dfd, filename, mode, flags) \
+	safe_fchmodat2(__FILE__, __LINE__, (dfd), (filename), (mode), (flags))
+
+static inline int safe_fchmodat2(const char *file, const int lineno,
+		int dfd, const char *filename, mode_t mode, int flags)
+{
+	int ret;
+
+	ret = tst_syscall(__NR_fchmodat2, dfd, filename, mode, flags);
+	if (ret == -1)
+		tst_brk_(file, lineno, TBROK | TERRNO, "%s(%d,%s,%d,%d) error",
+			__func__, dfd, filename, mode, flags);
+
+	return ret;
+}
+
 #endif /* LAPI_STAT_H__ */

-- 
2.43.0


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

[LTP] [PATCH v5 4/5] Add fchmodat2_01 test

[Andrea Cervesato]

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

This test verifies that fchmodat2() syscall is properly working with
AT_SYMLINK_NOFOLLOW on regular files. When AT_SYMLINK_NOFOLLOW is used
on symbolic links instead, we check for EOPNOTSUPP, since the feature is
not implemented for VFS.

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
 runtest/syscalls                                   |   2 +
 testcases/kernel/syscalls/fchmodat2/.gitignore     |   1 +
 testcases/kernel/syscalls/fchmodat2/Makefile       |   7 ++
 testcases/kernel/syscalls/fchmodat2/fchmodat2_01.c | 123 +++++++++++++++++++++
 4 files changed, 133 insertions(+)

diff --git a/runtest/syscalls b/runtest/syscalls
index 9b3cba667..50298d01e 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -270,6 +270,8 @@ fchmod06 fchmod06
 fchmodat01 fchmodat01
 fchmodat02 fchmodat02
 
+fchmodat2_01 fchmodat2_01
+
 fchown01 fchown01
 fchown01_16 fchown01_16
 fchown02 fchown02
diff --git a/testcases/kernel/syscalls/fchmodat2/.gitignore b/testcases/kernel/syscalls/fchmodat2/.gitignore
new file mode 100644
index 000000000..47d5e2427
--- /dev/null
+++ b/testcases/kernel/syscalls/fchmodat2/.gitignore
@@ -0,0 +1 @@
+fchmodat2_01
diff --git a/testcases/kernel/syscalls/fchmodat2/Makefile b/testcases/kernel/syscalls/fchmodat2/Makefile
new file mode 100644
index 000000000..8cf1b9024
--- /dev/null
+++ b/testcases/kernel/syscalls/fchmodat2/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+# Copyright (C) 2024 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
+
+top_srcdir		?= ../../../..
+
+include $(top_srcdir)/include/mk/testcases.mk
+include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/kernel/syscalls/fchmodat2/fchmodat2_01.c b/testcases/kernel/syscalls/fchmodat2/fchmodat2_01.c
new file mode 100644
index 000000000..d12c4a8fd
--- /dev/null
+++ b/testcases/kernel/syscalls/fchmodat2/fchmodat2_01.c
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2024 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * This test verifies that fchmodat2() syscall is properly working with
+ * regular files, symbolic links and directories. AT_SYMLINK_NOFOLLOW is a
+ * special feature that is not enabled in VFS for symbolic links, so we only
+ * verify that EOPNOTSUPP is correctly raised when used on those particular
+ * files.
+ */
+
+#include "tst_test.h"
+#include "tst_safe_file_at.h"
+#include "lapi/fcntl.h"
+#include "lapi/stat.h"
+
+#define MNTPOINT "mntpoint"
+#define FNAME "myfile"
+#define SNAME "symlink"
+#define DNAME "mydir"
+#define DNAME_PATH MNTPOINT"/"DNAME
+
+static int fd_dir = -1;
+
+static void verify_mode(int dirfd, const char *path, mode_t mode)
+{
+	struct stat st;
+
+	SAFE_FSTATAT(dirfd, path, &st, AT_SYMLINK_NOFOLLOW);
+	TST_EXP_EQ_LI(st.st_mode, mode);
+}
+
+static void test_regular_file(void)
+{
+	tst_res(TINFO, "Using regular files");
+
+	SAFE_CHMOD(MNTPOINT"/"FNAME, 0640);
+
+	SAFE_FCHMODAT2(fd_dir, FNAME, 0700, 0);
+	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
+
+	SAFE_FCHMODAT2(fd_dir, FNAME, 0700, AT_SYMLINK_NOFOLLOW);
+	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
+}
+
+static void test_symbolic_link(void)
+{
+	tst_res(TINFO, "Using symbolic link");
+
+	SAFE_FCHMODAT2(fd_dir, SNAME, 0700, 0);
+	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
+	verify_mode(fd_dir, SNAME, S_IFLNK | 0777);
+
+	if (tst_kvercmp(6, 6, 0)) {
+		TST_EXP_FAIL(tst_syscall(__NR_fchmodat2,
+			fd_dir, SNAME, 0640, AT_SYMLINK_NOFOLLOW),
+			EOPNOTSUPP);
+	}
+}
+
+static void test_empty_folder(void)
+{
+	tst_res(TINFO, "Using empty folder");
+
+	int fd;
+
+	SAFE_CHMOD(DNAME_PATH, 0640);
+	fd = SAFE_OPEN(DNAME_PATH, O_PATH | O_DIRECTORY, 0640);
+
+	SAFE_FCHMODAT2(fd, "", 0777, AT_EMPTY_PATH);
+	verify_mode(fd_dir, DNAME, S_IFDIR | 0777);
+
+	SAFE_CLOSE(fd);
+}
+
+static void run(void)
+{
+	test_regular_file();
+	test_empty_folder();
+	test_symbolic_link();
+}
+
+static void setup(void)
+{
+	fd_dir = SAFE_OPEN(MNTPOINT, O_PATH | O_DIRECTORY, 0640);
+
+	if (access(DNAME_PATH, F_OK) == -1)
+		SAFE_MKDIR(DNAME_PATH, 0640);
+
+	SAFE_TOUCH(MNTPOINT"/"FNAME, 0640, NULL);
+	SAFE_SYMLINKAT(FNAME, fd_dir, SNAME);
+}
+
+static void cleanup(void)
+{
+	SAFE_UNLINKAT(fd_dir, SNAME, 0);
+	SAFE_RMDIR(DNAME_PATH);
+
+	if (fd_dir != -1)
+		SAFE_CLOSE(fd_dir);
+}
+
+static struct tst_test test = {
+	.test_all = run,
+	.setup = setup,
+	.cleanup = cleanup,
+	.needs_root = 1,
+	.mntpoint = MNTPOINT,
+	.format_device = 1,
+	.all_filesystems = 1,
+	.skip_filesystems = (const char *const []) {
+		"fuse",
+		NULL
+	},
+	.tags = (const struct tst_tag[]) {
+		{"linux-git", "5d1f903f75a8"},
+		{}
+	}
+};

-- 
2.43.0


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

[LTP] [PATCH v5 5/5] Add fchmodat2_02 test

[Andrea Cervesato]

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

This test verifies that fchmodat2() syscall properly raises errors with
invalid arguments.

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

diff --git a/runtest/syscalls b/runtest/syscalls
index 50298d01e..7292e5ae7 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -271,6 +271,7 @@ fchmodat01 fchmodat01
 fchmodat02 fchmodat02
 
 fchmodat2_01 fchmodat2_01
+fchmodat2_02 fchmodat2_02
 
 fchown01 fchown01
 fchown01_16 fchown01_16
diff --git a/testcases/kernel/syscalls/fchmodat2/.gitignore b/testcases/kernel/syscalls/fchmodat2/.gitignore
index 47d5e2427..9f713198c 100644
--- a/testcases/kernel/syscalls/fchmodat2/.gitignore
+++ b/testcases/kernel/syscalls/fchmodat2/.gitignore
@@ -1 +1,2 @@
 fchmodat2_01
+fchmodat2_02
diff --git a/testcases/kernel/syscalls/fchmodat2/fchmodat2_02.c b/testcases/kernel/syscalls/fchmodat2/fchmodat2_02.c
new file mode 100644
index 000000000..f8497d8a8
--- /dev/null
+++ b/testcases/kernel/syscalls/fchmodat2/fchmodat2_02.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2024 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * This test verifies that fchmodat2() syscall properly raises errors with
+ * invalid values.
+ */
+
+#include "tst_test.h"
+#include "lapi/fcntl.h"
+#include "lapi/syscalls.h"
+#include "tst_tmpdir.h"
+
+#define FILENAME "file.bin"
+
+static char *tmpdir;
+static int fd;
+static int fd_invalid = -1;
+
+static struct tcase {
+	int *fd;
+	char *fname;
+	int mode;
+	int flag;
+	int exp_errno;
+	char *msg;
+} tcases[] = {
+	{&fd_invalid, FILENAME, 0777, 0, EBADF, "bad file descriptor"},
+	{&fd, "doesnt_exist.txt", 0777, 0, ENOENT, "unexisting file"},
+	{&fd, FILENAME, 0777, -1, EINVAL, "invalid flags"},
+};
+
+static void run(unsigned int i)
+{
+	struct tcase *tc = &tcases[i];
+
+	TST_EXP_FAIL(tst_syscall(__NR_fchmodat2,
+		*tc->fd, tc->fname, tc->mode, tc->flag),
+		tc->exp_errno,
+		"Test %s", tc->msg);
+}
+
+static void setup(void)
+{
+	tmpdir = tst_tmpdir_path();
+
+	SAFE_TOUCH(FILENAME, 0640, NULL);
+	fd = SAFE_OPEN(tmpdir, O_PATH | O_DIRECTORY, 0640);
+}
+
+static void cleanup(void)
+{
+	if (fd != -1)
+		SAFE_CLOSE(fd);
+}
+
+static struct tst_test test = {
+	.test = run,
+	.tcnt = ARRAY_SIZE(tcases),
+	.setup = setup,
+	.cleanup = cleanup,
+	.needs_tmpdir = 1,
+};
+

-- 
2.43.0


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

Re: [LTP] [PATCH v5 4/5] Add fchmodat2_01 test

[Petr Vorel]

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

> This test verifies that fchmodat2() syscall is properly working with
> AT_SYMLINK_NOFOLLOW on regular files. When AT_SYMLINK_NOFOLLOW is used
> on symbolic links instead, we check for EOPNOTSUPP, since the feature is
> not implemented for VFS.

> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
>  runtest/syscalls                                   |   2 +
>  testcases/kernel/syscalls/fchmodat2/.gitignore     |   1 +
>  testcases/kernel/syscalls/fchmodat2/Makefile       |   7 ++
>  testcases/kernel/syscalls/fchmodat2/fchmodat2_01.c | 123 +++++++++++++++++++++
>  4 files changed, 133 insertions(+)

> diff --git a/runtest/syscalls b/runtest/syscalls
> index 9b3cba667..50298d01e 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -270,6 +270,8 @@ fchmod06 fchmod06
>  fchmodat01 fchmodat01
>  fchmodat02 fchmodat02

> +fchmodat2_01 fchmodat2_01
> +
>  fchown01 fchown01
>  fchown01_16 fchown01_16
>  fchown02 fchown02
> diff --git a/testcases/kernel/syscalls/fchmodat2/.gitignore b/testcases/kernel/syscalls/fchmodat2/.gitignore
> new file mode 100644
> index 000000000..47d5e2427
> --- /dev/null
> +++ b/testcases/kernel/syscalls/fchmodat2/.gitignore
> @@ -0,0 +1 @@
> +fchmodat2_01
> diff --git a/testcases/kernel/syscalls/fchmodat2/Makefile b/testcases/kernel/syscalls/fchmodat2/Makefile
> new file mode 100644
> index 000000000..8cf1b9024
> --- /dev/null
> +++ b/testcases/kernel/syscalls/fchmodat2/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (C) 2024 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
> +
> +top_srcdir		?= ../../../..
> +
> +include $(top_srcdir)/include/mk/testcases.mk
> +include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/syscalls/fchmodat2/fchmodat2_01.c b/testcases/kernel/syscalls/fchmodat2/fchmodat2_01.c
> new file mode 100644
> index 000000000..d12c4a8fd
> --- /dev/null
> +++ b/testcases/kernel/syscalls/fchmodat2/fchmodat2_01.c
> @@ -0,0 +1,123 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2024 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * This test verifies that fchmodat2() syscall is properly working with
> + * regular files, symbolic links and directories. AT_SYMLINK_NOFOLLOW is a
> + * special feature that is not enabled in VFS for symbolic links, so we only
> + * verify that EOPNOTSUPP is correctly raised when used on those particular
> + * files.

If you don't mind, I'll add here explicit note that change done in 5d1f903f75a8.

> + */
> +
> +#include "tst_test.h"
> +#include "tst_safe_file_at.h"
> +#include "lapi/fcntl.h"
> +#include "lapi/stat.h"
> +
> +#define MNTPOINT "mntpoint"
> +#define FNAME "myfile"
> +#define SNAME "symlink"
> +#define DNAME "mydir"
> +#define DNAME_PATH MNTPOINT"/"DNAME
> +
> +static int fd_dir = -1;
> +
> +static void verify_mode(int dirfd, const char *path, mode_t mode)
> +{
> +	struct stat st;
> +
> +	SAFE_FSTATAT(dirfd, path, &st, AT_SYMLINK_NOFOLLOW);
> +	TST_EXP_EQ_LI(st.st_mode, mode);
> +}
> +
> +static void test_regular_file(void)
> +{
> +	tst_res(TINFO, "Using regular files");
> +
> +	SAFE_CHMOD(MNTPOINT"/"FNAME, 0640);
> +
> +	SAFE_FCHMODAT2(fd_dir, FNAME, 0700, 0);
> +	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
> +
> +	SAFE_FCHMODAT2(fd_dir, FNAME, 0700, AT_SYMLINK_NOFOLLOW);
> +	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
> +}
> +
> +static void test_symbolic_link(void)
> +{
> +	tst_res(TINFO, "Using symbolic link");
> +
> +	SAFE_FCHMODAT2(fd_dir, SNAME, 0700, 0);
> +	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
> +	verify_mode(fd_dir, SNAME, S_IFLNK | 0777);
> +
> +	if (tst_kvercmp(6, 6, 0)) {
> +		TST_EXP_FAIL(tst_syscall(__NR_fchmodat2,
> +			fd_dir, SNAME, 0640, AT_SYMLINK_NOFOLLOW),
> +			EOPNOTSUPP);
> +	}
I wrote later this is wrong, because the change was backported to all active
stable/LTS trees. We should expect this functionality on all kernels
(tested on 6.6.15, 6.9.9, 6.10.1).

Also, I hesitated, whether we should check AT_SYMLINK_NOFOLLOW also in
safe_fchmodat2(), but we can probably ignore that.

I can change it before merge.

> +}
> +
> +static void test_empty_folder(void)
> +{
> +	tst_res(TINFO, "Using empty folder");
> +
> +	int fd;
> +
> +	SAFE_CHMOD(DNAME_PATH, 0640);
> +	fd = SAFE_OPEN(DNAME_PATH, O_PATH | O_DIRECTORY, 0640);
> +
> +	SAFE_FCHMODAT2(fd, "", 0777, AT_EMPTY_PATH);
> +	verify_mode(fd_dir, DNAME, S_IFDIR | 0777);
> +
> +	SAFE_CLOSE(fd);
> +}
> +
> +static void run(void)
> +{
> +	test_regular_file();
> +	test_empty_folder();
> +	test_symbolic_link();
> +}
> +
> +static void setup(void)
> +{
> +	fd_dir = SAFE_OPEN(MNTPOINT, O_PATH | O_DIRECTORY, 0640);
> +
> +	if (access(DNAME_PATH, F_OK) == -1)
> +		SAFE_MKDIR(DNAME_PATH, 0640);
> +
> +	SAFE_TOUCH(MNTPOINT"/"FNAME, 0640, NULL);
> +	SAFE_SYMLINKAT(FNAME, fd_dir, SNAME);
> +}
> +
> +static void cleanup(void)
> +{
> +	SAFE_UNLINKAT(fd_dir, SNAME, 0);
> +	SAFE_RMDIR(DNAME_PATH);
> +
> +	if (fd_dir != -1)
> +		SAFE_CLOSE(fd_dir);
> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.needs_root = 1,
> +	.mntpoint = MNTPOINT,
> +	.format_device = 1,
> +	.all_filesystems = 1,
> +	.skip_filesystems = (const char *const []) {
> +		"fuse",
> +		NULL
> +	},
You were faster than I managed to write. IMHO skipping fuse is not needed.
IMHO it was broken due EOPNOTSUPP.

With these 2 changes:

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

Kind regards,
Petr

> +	.tags = (const struct tst_tag[]) {
> +		{"linux-git", "5d1f903f75a8"},
> +		{}
> +	}
> +};

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

Re: [LTP] [PATCH v5 1/5] Add SAFE_SYMLINKAT macro

[Petr Vorel]

Hi Andrea,

> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
>  include/safe_macros_fn.h  |  4 ++++
>  include/tst_safe_macros.h |  3 +++
>  lib/safe_macros.c         | 20 ++++++++++++++++++++
>  3 files changed, 27 insertions(+)

> diff --git a/include/safe_macros_fn.h b/include/safe_macros_fn.h
> index d256091b7..6d9e72e4f 100644
> --- a/include/safe_macros_fn.h
> +++ b/include/safe_macros_fn.h
> @@ -122,6 +122,10 @@ int safe_symlink(const char *file, const int lineno,
>                   void (cleanup_fn)(void), const char *oldpath,
>                   const char *newpath);

> +int safe_symlinkat(const char *file, const int lineno,
> +                 void (cleanup_fn)(void), const char *oldpath,
> +                 const int newdirfd, const char *newpath);
> +

IMHO we agreed that we don't touch legacy API unless really needed.
If I remember correctly (I'm sorry I don't have a link), Cyril was not against
modifying lib/safe_macros.c (I would be against there is no need to touch old
API at all), but he was against exposing safe_symlinkat() in old API headers.

@Cyril correct me please if I'm wrong.
@Andrea if not, would you mind to move things to include/tst_safe_macros.h
and lib/tst_safe_macros.c?

The code itself is correct.

Kind regards,
Petr

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

Re: [LTP] [PATCH v5 1/5] Add SAFE_SYMLINKAT macro

[Cyril Hrubis]

Hi!
> @Cyril correct me please if I'm wrong.

Yes, the rule is no new API functions for old API.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v5 2/5] Add fchmodat2 syscalls definitions

[Petr Vorel]

Hi Andrea,

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

Thanks!

Kind regards,
Petr

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

Re: [LTP] [PATCH v5 3/5] Add fchmodat2 fallback definition

[Petr Vorel]

Hi Andrea,


>  include/lapi/stat.h | 16 ++++++++++++++++
What is the reason this to be added to include/lapi/stat.h?

It's not a general function? (otherwise tst_safe_macros.c would be more
appropriate).

And because one day it will be defined in <sys/stat.h>?

Probably ok due both reasons, but I'm just curious, also because static inline
brought various problems in the past.


>  1 file changed, 16 insertions(+)

> diff --git a/include/lapi/stat.h b/include/lapi/stat.h
> index 3606c9eb0..8e38ecfef 100644
> --- a/include/lapi/stat.h
> +++ b/include/lapi/stat.h
> @@ -229,4 +229,20 @@ static inline int statx(int dirfd, const char *pathname, unsigned int flags,
>  # define STATX_ATTR_VERITY	0x00100000
>  #endif

> +#define SAFE_FCHMODAT2(dfd, filename, mode, flags) \
> +	safe_fchmodat2(__FILE__, __LINE__, (dfd), (filename), (mode), (flags))
> +
> +static inline int safe_fchmodat2(const char *file, const int lineno,
> +		int dfd, const char *filename, mode_t mode, int flags)
> +{
> +	int ret;
> +
> +	ret = tst_syscall(__NR_fchmodat2, dfd, filename, mode, flags);
> +	if (ret == -1)
> +		tst_brk_(file, lineno, TBROK | TERRNO, "%s(%d,%s,%d,%d) error",
> +			__func__, dfd, filename, mode, flags);
> +
> +	return ret;

The code itself is obviously correct.

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

Kind regards,
Petr

> +}
> +
>  #endif /* LAPI_STAT_H__ */

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

Re: [LTP] [PATCH v5 0/5] Add fchmodat2 testing suite

[Petr Vorel]

Hi,

> ***********
> * WARNING *
> ***********

> fchmodat2_02 fails with EOPNOTSUPP because of missing feature.
> According to documentation, the feature has been implemented in
> kernel 6.6, but __in reality__ AT_SYMLINK_NOFOLLOW is not working
> on symbolic files. Also kselftests, which are meant to test the
> functionality, are not working and they are treating fchmodat2()
> syscall failure as SKIP. Please take a look at the following code
> before reviewing:

For anybody who did not follow the previous version, this has been discussed and
solved (visible in changelog below)

Kind regards,
Petr

> https://github.com/torvalds/linux/blob/8f6a15f095a63a83b096d9b29aaff4f0fbe6f6e6/tools/testing/selftests/fchmodat2/fchmodat2_test.c#L123

> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
> Changes in v5:
> - fchmodat2_01: check for EOPNOTSUPP, added tag and removed kernel
>   version in order to let backported feature to be available for testing
> - Link to v4: https://lore.kernel.org/r/20240801-fchmodat2-v4-0-7f2f11a53a09@suse.com

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

Re: [LTP] [PATCH v5 1/5] Add SAFE_SYMLINKAT macro

[Petr Vorel]

> Hi!
> > @Cyril correct me please if I'm wrong.

> Yes, the rule is no new API functions for old API.

Thanks, I'll send a patch to document it.

Kind regards,
Petr

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

Re: [LTP] [PATCH v5 5/5] Add fchmodat2_02 test

[Petr Vorel]

Hi Andrea,

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

Kind regards,
Petr

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

Re: [LTP] [PATCH v5 3/5] Add fchmodat2 fallback definition

[Andrea Cervesato via ltp]

Hi,

On 8/2/24 11:55, Petr Vorel wrote:
> Hi Andrea,
>
>
>>   include/lapi/stat.h | 16 ++++++++++++++++
> What is the reason this to be added to include/lapi/stat.h?
>
> It's not a general function? (otherwise tst_safe_macros.c would be more
> appropriate).
>
> And because one day it will be defined in <sys/stat.h>?
>
> Probably ok due both reasons, but I'm just curious, also because static inline
> brought various problems in the past.
>
I was asked by Cyril to use proper headers instead of safe macros 
headers, which contain anything.
>>   1 file changed, 16 insertions(+)
>> diff --git a/include/lapi/stat.h b/include/lapi/stat.h
>> index 3606c9eb0..8e38ecfef 100644
>> --- a/include/lapi/stat.h
>> +++ b/include/lapi/stat.h
>> @@ -229,4 +229,20 @@ static inline int statx(int dirfd, const char *pathname, unsigned int flags,
>>   # define STATX_ATTR_VERITY	0x00100000
>>   #endif
>> +#define SAFE_FCHMODAT2(dfd, filename, mode, flags) \
>> +	safe_fchmodat2(__FILE__, __LINE__, (dfd), (filename), (mode), (flags))
>> +
>> +static inline int safe_fchmodat2(const char *file, const int lineno,
>> +		int dfd, const char *filename, mode_t mode, int flags)
>> +{
>> +	int ret;
>> +
>> +	ret = tst_syscall(__NR_fchmodat2, dfd, filename, mode, flags);
>> +	if (ret == -1)
>> +		tst_brk_(file, lineno, TBROK | TERRNO, "%s(%d,%s,%d,%d) error",
>> +			__func__, dfd, filename, mode, flags);
>> +
>> +	return ret;
> The code itself is obviously correct.
>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>
> Kind regards,
> Petr
>
>> +}
>> +
>>   #endif /* LAPI_STAT_H__ */

Andrea


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

Re: [LTP] [PATCH v5 1/5] Add SAFE_SYMLINKAT macro

[Andrea Cervesato via ltp]

Thanks for catching it. It's clearly an error and I already knew these 
headers should not be touched.

On 8/2/24 11:48, Cyril Hrubis wrote:
> Hi!
>> @Cyril correct me please if I'm wrong.
> Yes, the rule is no new API functions for old API.
>
Andrea


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

Re: [LTP] [PATCH v5 3/5] Add fchmodat2 fallback definition

[Petr Vorel]

Hi Andrea,

> Hi,

> On 8/2/24 11:55, Petr Vorel wrote:
> > Hi Andrea,


> > >   include/lapi/stat.h | 16 ++++++++++++++++
> > What is the reason this to be added to include/lapi/stat.h?

> > It's not a general function? (otherwise tst_safe_macros.c would be more
> > appropriate).

> > And because one day it will be defined in <sys/stat.h>?

> > Probably ok due both reasons, but I'm just curious, also because static inline
> > brought various problems in the past.

> I was asked by Cyril to use proper headers instead of safe macros headers,
> which contain anything.

Thanks for info, I'm sorry for the noise.

Kind regards,
Petr

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