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

[LTP] [PATCH v4 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 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 | 114 +++++++++++++++++++++
 testcases/kernel/syscalls/fchmodat2/fchmodat2_02.c |  68 ++++++++++++
 27 files changed, 255 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 v4 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 v4 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 v4 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 v4 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.

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 | 114 +++++++++++++++++++++
 4 files changed, 124 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..6f77788d2
--- /dev/null
+++ b/testcases/kernel/syscalls/fchmodat2/fchmodat2_01.c
@@ -0,0 +1,114 @@
+// 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
+ * AT_SYMLINK_NOFOLLOW regular files, symbolic links and directories.
+ */
+
+#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);
+
+	SAFE_FCHMODAT2(fd_dir, SNAME, 0640, AT_SYMLINK_NOFOLLOW);
+	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
+	verify_mode(fd_dir, SNAME, S_IFLNK | 0640);
+}
+
+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,
+	.min_kver = "6.6",
+	.mntpoint = MNTPOINT,
+	.format_device = 1,
+	.all_filesystems = 1,
+	.skip_filesystems = (const char *const []) {
+		"fuse",
+		NULL
+	},
+};

-- 
2.43.0


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

[LTP] [PATCH v4 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 v4 0/5] Add fchmodat2 testing suite

[Petr Vorel]

Hi all,

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

I would hope that it'd be at least Christian's fork [1], but it's not there.
I suppose nobody is working on the man page.

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

For a record, it's fchmodat2_01.c (from this patchset) which is failing (on
6.10.1-1.g4c78d6f-default Tumbleweed and 6.6.21-0-lts Alpine, both x86_64 VMs).

Andrea, I would personally just skip test on EOPNOTSUPP (that's what we do in
LTP on EOPNOTSUPP). The question why is not supported and whether is going to be
fixed.

Looking into glibc change 65341f7bbe ("linux: Use fchmodat2 on fchmod for flags
different than 0 (BZ 26401)") one year old change from glibc-2.39 [2] it looks
just accepted behavior (glibc returns EOPNOTSUPP on symlink):

+  /* Some Linux versions with some file systems can actually
+     change symbolic link permissions via /proc, but this is not
+     intentional, and it gives inconsistent results (e.g., error
+     return despite mode change).  The expected behavior is that
+     symbolic link modes cannot be changed at all, and this check
+     enforces that.  */
+  if (S_ISLNK (st.st_mode))
+    {
       __close_nocancel (pathfd);
-      return ret;
+      __set_errno (EOPNOTSUPP);
+      return -1;
+    }

Also musl also behaves the same on his fallback on old kernels [3]
(it started 10 years ago on 0dc48244 ("work around linux's lack of flags
argument to fchmodat syscall") when SYS_fchmodat was used and kept when this
year SYS_fchmodat2 started to be used in d0ed307e):

	int ret = __syscall(SYS_fchmodat2, fd, path, mode, flag);
	if (ret != -ENOSYS) return __syscall_ret(ret);

	if (flag != AT_SYMLINK_NOFOLLOW)
		return __syscall_ret(-EINVAL);

	struct stat st;
	int fd2;
	char proc[15+3*sizeof(int)];

	if (fstatat(fd, path, &st, flag))
		return -1;
	if (S_ISLNK(st.st_mode))
		return __syscall_ret(-EOPNOTSUPP);


> 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

I see there is a kselftest workaround in 4859c257d295 ("selftests: Add fchmodat2
selftest") [4], where fchmodat2 failure on symlink is simply skipped.

Aleksa, you're probably aware of this fchmodat2() failure on symlinks. Does
anybody work or plan to work on fixing it? LTP has policy to not cover kernel
bugs, if it's not expected to be working we might just skip the test as well.

I see a RFC UAPI related patchset [5] which touches include/uapi/linux/fcntl.h,
but AFAIK it's not related to this problem.

Kind regards,
Petr

[1] https://github.com/brauner/man-pages-md
[2] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=65341f7bbea824d2ff9d37db15d8be162df42bd3;hp=c52c2c32db15aba8bbe1a0b4d3235f97d9c1a525
[3] https://git.musl-libc.org/cgit/musl/tree/src/stat/fchmodat.c
[4] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/tools/testing/selftests/fchmodat2/fchmodat2_test.c?h=next-20240801&id=4859c257d295949c23f4074850a8c2ec31357abb
[5] https://lore.kernel.org/lkml/20240801-exportfs-u64-mount-id-v3-0-be5d6283144a@cyphar.com/

> Signed-off-by: Andrea Cervesato <andrea.cervesato@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 | 114 +++++++++++++++++++++
>  testcases/kernel/syscalls/fchmodat2/fchmodat2_02.c |  68 ++++++++++++
>  27 files changed, 255 insertions(+)
> ---
> base-commit: 8422d4680b21e6576da63c677b5d49f46b477df0
> change-id: 20240517-fchmodat2-5b82867d71fc

> Best regards,

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

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

[Petr Vorel]

Hi all,

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

I would hope that it'd be at least Christian's fork [1], but it's not there.
I suppose nobody is working on the man page.

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

For a record, it's fchmodat2_01.c (from this patchset) which is failing (on
6.10.1-1.g4c78d6f-default Tumbleweed and 6.6.21-0-lts Alpine, both x86_64 VMs).

Andrea, I would personally just skip test on EOPNOTSUPP (that's what we do in
LTP on EOPNOTSUPP). The question why is not supported and whether is going to be
fixed.

Looking into glibc change 65341f7bbe ("linux: Use fchmodat2 on fchmod for flags
different than 0 (BZ 26401)") one year old change from glibc-2.39 [2] it looks
just accepted behavior (glibc returns EOPNOTSUPP on symlink):

+  /* Some Linux versions with some file systems can actually
+     change symbolic link permissions via /proc, but this is not
+     intentional, and it gives inconsistent results (e.g., error
+     return despite mode change).  The expected behavior is that
+     symbolic link modes cannot be changed at all, and this check
+     enforces that.  */
+  if (S_ISLNK (st.st_mode))
+    {
       __close_nocancel (pathfd);
-      return ret;
+      __set_errno (EOPNOTSUPP);
+      return -1;
+    }

Also musl also behaves the same on his fallback on old kernels [3]
(it started 10 years ago on 0dc48244 ("work around linux's lack of flags
argument to fchmodat syscall") when SYS_fchmodat was used and kept when this
year SYS_fchmodat2 started to be used in d0ed307e):

	int ret = __syscall(SYS_fchmodat2, fd, path, mode, flag);
	if (ret != -ENOSYS) return __syscall_ret(ret);

	if (flag != AT_SYMLINK_NOFOLLOW)
		return __syscall_ret(-EINVAL);

	struct stat st;
	int fd2;
	char proc[15+3*sizeof(int)];

	if (fstatat(fd, path, &st, flag))
		return -1;
	if (S_ISLNK(st.st_mode))
		return __syscall_ret(-EOPNOTSUPP);


> 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

I see there is a kselftest workaround in 4859c257d295 ("selftests: Add fchmodat2
selftest") [4], where fchmodat2 failure on symlink is simply skipped.

Aleksa, you're probably aware of this fchmodat2() failure on symlinks. Does
anybody work or plan to work on fixing it? LTP has policy to not cover kernel
bugs, if it's not expected to be working we might just skip the test as well.

I see a RFC UAPI related patchset [5] which touches include/uapi/linux/fcntl.h,
but AFAIK it's not related to this problem.

Kind regards,
Petr

[1] https://github.com/brauner/man-pages-md
[2] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=65341f7bbea824d2ff9d37db15d8be162df42bd3;hp=c52c2c32db15aba8bbe1a0b4d3235f97d9c1a525
[3] https://git.musl-libc.org/cgit/musl/tree/src/stat/fchmodat.c
[4] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/tools/testing/selftests/fchmodat2/fchmodat2_test.c?h=next-20240801&id=4859c257d295949c23f4074850a8c2ec31357abb
[5] https://lore.kernel.org/lkml/20240801-exportfs-u64-mount-id-v3-0-be5d6283144a@cyphar.com/

> Signed-off-by: Andrea Cervesato <andrea.cervesato@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 | 114 +++++++++++++++++++++
>  testcases/kernel/syscalls/fchmodat2/fchmodat2_02.c |  68 ++++++++++++
>  27 files changed, 255 insertions(+)
> ---
> base-commit: 8422d4680b21e6576da63c677b5d49f46b477df0
> change-id: 20240517-fchmodat2-5b82867d71fc

> Best regards,

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

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 8610 bytes --]

On 2024-08-01, Petr Vorel <pvorel@suse.cz> wrote:
> Hi all,
> 
> > 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:
> 
> I would hope that it'd be at least Christian's fork [1], but it's not there.
> I suppose nobody is working on the man page.
> 
> > 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.
> 
> For a record, it's fchmodat2_01.c (from this patchset) which is failing (on
> 6.10.1-1.g4c78d6f-default Tumbleweed and 6.6.21-0-lts Alpine, both x86_64 VMs).
> 
> Andrea, I would personally just skip test on EOPNOTSUPP (that's what we do in
> LTP on EOPNOTSUPP). The question why is not supported and whether is going to be
> fixed.
> 
> Looking into glibc change 65341f7bbe ("linux: Use fchmodat2 on fchmod for flags
> different than 0 (BZ 26401)") one year old change from glibc-2.39 [2] it looks
> just accepted behavior (glibc returns EOPNOTSUPP on symlink):
> 
> +  /* Some Linux versions with some file systems can actually
> +     change symbolic link permissions via /proc, but this is not
> +     intentional, and it gives inconsistent results (e.g., error
> +     return despite mode change).  The expected behavior is that
> +     symbolic link modes cannot be changed at all, and this check
> +     enforces that.  */
> +  if (S_ISLNK (st.st_mode))
> +    {
>        __close_nocancel (pathfd);
> -      return ret;
> +      __set_errno (EOPNOTSUPP);
> +      return -1;
> +    }
> 
> Also musl also behaves the same on his fallback on old kernels [3]
> (it started 10 years ago on 0dc48244 ("work around linux's lack of flags
> argument to fchmodat syscall") when SYS_fchmodat was used and kept when this
> year SYS_fchmodat2 started to be used in d0ed307e):
> 
> 	int ret = __syscall(SYS_fchmodat2, fd, path, mode, flag);
> 	if (ret != -ENOSYS) return __syscall_ret(ret);
> 
> 	if (flag != AT_SYMLINK_NOFOLLOW)
> 		return __syscall_ret(-EINVAL);
> 
> 	struct stat st;
> 	int fd2;
> 	char proc[15+3*sizeof(int)];
> 
> 	if (fstatat(fd, path, &st, flag))
> 		return -1;
> 	if (S_ISLNK(st.st_mode))
> 		return __syscall_ret(-EOPNOTSUPP);
> 
> 
> > 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
> 
> I see there is a kselftest workaround in 4859c257d295 ("selftests: Add fchmodat2
> selftest") [4], where fchmodat2 failure on symlink is simply skipped.
> 
> Aleksa, you're probably aware of this fchmodat2() failure on symlinks. Does
> anybody work or plan to work on fixing it? LTP has policy to not cover kernel
> bugs, if it's not expected to be working we might just skip the test as well.

If I understand the bug report, the issue is that fchmodat2() doesn't
work on symlinks?

This is intentional -- Christian fixed a tree-wide bug a while ago[1]
where some filesystems would change the mode of symlinks despite
returning an error (usually EOPNOTSUPP) and IIRC a few others would
happily change the mode of symlinks.

The current intended behaviour is to always return EOPNOTSUPP, and AFAIK
there is no plan to re-enable the changing of symlink modes. EOPNOTSUPP
was chosen because that's what filesystems were already returning.
(While this is a little confusing, VFS syscalls return EINVAL for an
unsupported flag, not EOPNOTSUPP.)

The benefit of an AT_SYMLINK_NOFOLLOW flag is not just to to allow a
syscall to operate on symlinks, it also allows programs to safely
operate on path components without worrying about symlinks being
followed (this is relevant for container runtimes, where we are
operating on untrusted filesystem roots -- though in the case of
fchmodat2(2) you would probably just use AT_EMPTY_PATH in practice). So
an error here is actually what you want as a program that uses
AT_SYMLINK_NOFOLLOW (since the actual operation is intentionally not
supported by filesystems).

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5d1f903f75a80daa4dfb3d84e114ec8ecbf29956

> I see a RFC UAPI related patchset [5] which touches include/uapi/linux/fcntl.h,
> but AFAIK it's not related to this problem.

Yeah this is unrelated, that patch is about clarifying how AT_* flags
are allocated, not syscall behaviour.

> Kind regards,
> Petr
> 
> [1] https://github.com/brauner/man-pages-md
> [2] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=65341f7bbea824d2ff9d37db15d8be162df42bd3;hp=c52c2c32db15aba8bbe1a0b4d3235f97d9c1a525
> [3] https://git.musl-libc.org/cgit/musl/tree/src/stat/fchmodat.c
> [4] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/tools/testing/selftests/fchmodat2/fchmodat2_test.c?h=next-20240801&id=4859c257d295949c23f4074850a8c2ec31357abb
> [5] https://lore.kernel.org/lkml/20240801-exportfs-u64-mount-id-v3-0-be5d6283144a@cyphar.com/
> 
> > Signed-off-by: Andrea Cervesato <andrea.cervesato@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 | 114 +++++++++++++++++++++
> >  testcases/kernel/syscalls/fchmodat2/fchmodat2_02.c |  68 ++++++++++++
> >  27 files changed, 255 insertions(+)
> > ---
> > base-commit: 8422d4680b21e6576da63c677b5d49f46b477df0
> > change-id: 20240517-fchmodat2-5b82867d71fc
> 
> > Best regards,

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Petr Vorel]

> On 2024-08-01, Petr Vorel <pvorel@suse.cz> wrote:
> > Hi all,

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

> > I would hope that it'd be at least Christian's fork [1], but it's not there.
> > I suppose nobody is working on the man page.

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

> > For a record, it's fchmodat2_01.c (from this patchset) which is failing (on
> > 6.10.1-1.g4c78d6f-default Tumbleweed and 6.6.21-0-lts Alpine, both x86_64 VMs).

> > Andrea, I would personally just skip test on EOPNOTSUPP (that's what we do in
> > LTP on EOPNOTSUPP). The question why is not supported and whether is going to be
> > fixed.

> > Looking into glibc change 65341f7bbe ("linux: Use fchmodat2 on fchmod for flags
> > different than 0 (BZ 26401)") one year old change from glibc-2.39 [2] it looks
> > just accepted behavior (glibc returns EOPNOTSUPP on symlink):

> > +  /* Some Linux versions with some file systems can actually
> > +     change symbolic link permissions via /proc, but this is not
> > +     intentional, and it gives inconsistent results (e.g., error
> > +     return despite mode change).  The expected behavior is that
> > +     symbolic link modes cannot be changed at all, and this check
> > +     enforces that.  */
> > +  if (S_ISLNK (st.st_mode))
> > +    {
> >        __close_nocancel (pathfd);
> > -      return ret;
> > +      __set_errno (EOPNOTSUPP);
> > +      return -1;
> > +    }

> > Also musl also behaves the same on his fallback on old kernels [3]
> > (it started 10 years ago on 0dc48244 ("work around linux's lack of flags
> > argument to fchmodat syscall") when SYS_fchmodat was used and kept when this
> > year SYS_fchmodat2 started to be used in d0ed307e):

> > 	int ret = __syscall(SYS_fchmodat2, fd, path, mode, flag);
> > 	if (ret != -ENOSYS) return __syscall_ret(ret);

> > 	if (flag != AT_SYMLINK_NOFOLLOW)
> > 		return __syscall_ret(-EINVAL);

> > 	struct stat st;
> > 	int fd2;
> > 	char proc[15+3*sizeof(int)];

> > 	if (fstatat(fd, path, &st, flag))
> > 		return -1;
> > 	if (S_ISLNK(st.st_mode))
> > 		return __syscall_ret(-EOPNOTSUPP);


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

> > I see there is a kselftest workaround in 4859c257d295 ("selftests: Add fchmodat2
> > selftest") [4], where fchmodat2 failure on symlink is simply skipped.

> > Aleksa, you're probably aware of this fchmodat2() failure on symlinks. Does
> > anybody work or plan to work on fixing it? LTP has policy to not cover kernel
> > bugs, if it's not expected to be working we might just skip the test as well.

> If I understand the bug report, the issue is that fchmodat2() doesn't
> work on symlinks?

Yes.

> This is intentional -- Christian fixed a tree-wide bug a while ago[1]
> where some filesystems would change the mode of symlinks despite
> returning an error (usually EOPNOTSUPP) and IIRC a few others would
> happily change the mode of symlinks.

Ah, I've seen this in the past. Thanks a lot for reminding me.

> The current intended behaviour is to always return EOPNOTSUPP, and AFAIK
> there is no plan to re-enable the changing of symlink modes. EOPNOTSUPP
> was chosen because that's what filesystems were already returning.
> (While this is a little confusing, VFS syscalls return EINVAL for an
> unsupported flag, not EOPNOTSUPP.)

> The benefit of an AT_SYMLINK_NOFOLLOW flag is not just to to allow a
> syscall to operate on symlinks, it also allows programs to safely
> operate on path components without worrying about symlinks being
> followed (this is relevant for container runtimes, where we are
> operating on untrusted filesystem roots -- though in the case of
> fchmodat2(2) you would probably just use AT_EMPTY_PATH in practice). So
> an error here is actually what you want as a program that uses
> AT_SYMLINK_NOFOLLOW (since the actual operation is intentionally not
> supported by filesystems).

Thanks a lot for explaining the background!

> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5d1f903f75a80daa4dfb3d84e114ec8ecbf29956

> > I see a RFC UAPI related patchset [5] which touches include/uapi/linux/fcntl.h,
> > but AFAIK it's not related to this problem.

> Yeah this is unrelated, that patch is about clarifying how AT_* flags
> are allocated, not syscall behaviour.

Thanks!

> > Kind regards,
> > Petr

@Andrea, I guess we want something like this:

+++ testcases/kernel/syscalls/fchmodat2/fchmodat2_01.c
@@ -43,9 +43,10 @@ static void test_symbolic_link(void)
 	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
 	verify_mode(fd_dir, SNAME, S_IFLNK | 0777);
 
-	TST_EXP_PASS(fchmodat2(fd_dir, SNAME, 0640, AT_SYMLINK_NOFOLLOW));
-	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
-	verify_mode(fd_dir, SNAME, S_IFLNK | 0640);
+	if (tst_kvercmp(6, 6, 0) >= 0) {
+		TST_EXP_FAIL(tst_syscall(__NR_fchmodat2, fd_dir, SNAME, 0640,
+					 AT_SYMLINK_NOFOLLOW), EOPNOTSUPP);
+	}
 }
 
 static void test_empty_folder(void)

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

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

[Petr Vorel]

> On 2024-08-01, Petr Vorel <pvorel@suse.cz> wrote:
> > Hi all,

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

> > I would hope that it'd be at least Christian's fork [1], but it's not there.
> > I suppose nobody is working on the man page.

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

> > For a record, it's fchmodat2_01.c (from this patchset) which is failing (on
> > 6.10.1-1.g4c78d6f-default Tumbleweed and 6.6.21-0-lts Alpine, both x86_64 VMs).

> > Andrea, I would personally just skip test on EOPNOTSUPP (that's what we do in
> > LTP on EOPNOTSUPP). The question why is not supported and whether is going to be
> > fixed.

> > Looking into glibc change 65341f7bbe ("linux: Use fchmodat2 on fchmod for flags
> > different than 0 (BZ 26401)") one year old change from glibc-2.39 [2] it looks
> > just accepted behavior (glibc returns EOPNOTSUPP on symlink):

> > +  /* Some Linux versions with some file systems can actually
> > +     change symbolic link permissions via /proc, but this is not
> > +     intentional, and it gives inconsistent results (e.g., error
> > +     return despite mode change).  The expected behavior is that
> > +     symbolic link modes cannot be changed at all, and this check
> > +     enforces that.  */
> > +  if (S_ISLNK (st.st_mode))
> > +    {
> >        __close_nocancel (pathfd);
> > -      return ret;
> > +      __set_errno (EOPNOTSUPP);
> > +      return -1;
> > +    }

> > Also musl also behaves the same on his fallback on old kernels [3]
> > (it started 10 years ago on 0dc48244 ("work around linux's lack of flags
> > argument to fchmodat syscall") when SYS_fchmodat was used and kept when this
> > year SYS_fchmodat2 started to be used in d0ed307e):

> > 	int ret = __syscall(SYS_fchmodat2, fd, path, mode, flag);
> > 	if (ret != -ENOSYS) return __syscall_ret(ret);

> > 	if (flag != AT_SYMLINK_NOFOLLOW)
> > 		return __syscall_ret(-EINVAL);

> > 	struct stat st;
> > 	int fd2;
> > 	char proc[15+3*sizeof(int)];

> > 	if (fstatat(fd, path, &st, flag))
> > 		return -1;
> > 	if (S_ISLNK(st.st_mode))
> > 		return __syscall_ret(-EOPNOTSUPP);


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

> > I see there is a kselftest workaround in 4859c257d295 ("selftests: Add fchmodat2
> > selftest") [4], where fchmodat2 failure on symlink is simply skipped.

> > Aleksa, you're probably aware of this fchmodat2() failure on symlinks. Does
> > anybody work or plan to work on fixing it? LTP has policy to not cover kernel
> > bugs, if it's not expected to be working we might just skip the test as well.

> If I understand the bug report, the issue is that fchmodat2() doesn't
> work on symlinks?

Yes.

> This is intentional -- Christian fixed a tree-wide bug a while ago[1]
> where some filesystems would change the mode of symlinks despite
> returning an error (usually EOPNOTSUPP) and IIRC a few others would
> happily change the mode of symlinks.

Ah, I've seen this in the past. Thanks a lot for reminding me.

> The current intended behaviour is to always return EOPNOTSUPP, and AFAIK
> there is no plan to re-enable the changing of symlink modes. EOPNOTSUPP
> was chosen because that's what filesystems were already returning.
> (While this is a little confusing, VFS syscalls return EINVAL for an
> unsupported flag, not EOPNOTSUPP.)

> The benefit of an AT_SYMLINK_NOFOLLOW flag is not just to to allow a
> syscall to operate on symlinks, it also allows programs to safely
> operate on path components without worrying about symlinks being
> followed (this is relevant for container runtimes, where we are
> operating on untrusted filesystem roots -- though in the case of
> fchmodat2(2) you would probably just use AT_EMPTY_PATH in practice). So
> an error here is actually what you want as a program that uses
> AT_SYMLINK_NOFOLLOW (since the actual operation is intentionally not
> supported by filesystems).

Thanks a lot for explaining the background!

> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5d1f903f75a80daa4dfb3d84e114ec8ecbf29956

> > I see a RFC UAPI related patchset [5] which touches include/uapi/linux/fcntl.h,
> > but AFAIK it's not related to this problem.

> Yeah this is unrelated, that patch is about clarifying how AT_* flags
> are allocated, not syscall behaviour.

Thanks!

> > Kind regards,
> > Petr

@Andrea, I guess we want something like this:

+++ testcases/kernel/syscalls/fchmodat2/fchmodat2_01.c
@@ -43,9 +43,10 @@ static void test_symbolic_link(void)
 	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
 	verify_mode(fd_dir, SNAME, S_IFLNK | 0777);
 
-	TST_EXP_PASS(fchmodat2(fd_dir, SNAME, 0640, AT_SYMLINK_NOFOLLOW));
-	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
-	verify_mode(fd_dir, SNAME, S_IFLNK | 0640);
+	if (tst_kvercmp(6, 6, 0) >= 0) {
+		TST_EXP_FAIL(tst_syscall(__NR_fchmodat2, fd_dir, SNAME, 0640,
+					 AT_SYMLINK_NOFOLLOW), EOPNOTSUPP);
+	}
 }
 
 static void test_empty_folder(void)

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

[Andrea Cervesato via ltp]

Hi!

On 8/2/24 07:42, Petr Vorel wrote:
>> On 2024-08-01, Petr Vorel <pvorel@suse.cz> wrote:
>>> Hi all,
>>>> 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:
>>> I would hope that it'd be at least Christian's fork [1], but it's not there.
>>> I suppose nobody is working on the man page.
>>>> 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.
>>> For a record, it's fchmodat2_01.c (from this patchset) which is failing (on
>>> 6.10.1-1.g4c78d6f-default Tumbleweed and 6.6.21-0-lts Alpine, both x86_64 VMs).
>>> Andrea, I would personally just skip test on EOPNOTSUPP (that's what we do in
>>> LTP on EOPNOTSUPP). The question why is not supported and whether is going to be
>>> fixed.
>>> Looking into glibc change 65341f7bbe ("linux: Use fchmodat2 on fchmod for flags
>>> different than 0 (BZ 26401)") one year old change from glibc-2.39 [2] it looks
>>> just accepted behavior (glibc returns EOPNOTSUPP on symlink):
>>> +  /* Some Linux versions with some file systems can actually
>>> +     change symbolic link permissions via /proc, but this is not
>>> +     intentional, and it gives inconsistent results (e.g., error
>>> +     return despite mode change).  The expected behavior is that
>>> +     symbolic link modes cannot be changed at all, and this check
>>> +     enforces that.  */
>>> +  if (S_ISLNK (st.st_mode))
>>> +    {
>>>         __close_nocancel (pathfd);
>>> -      return ret;
>>> +      __set_errno (EOPNOTSUPP);
>>> +      return -1;
>>> +    }
>>> Also musl also behaves the same on his fallback on old kernels [3]
>>> (it started 10 years ago on 0dc48244 ("work around linux's lack of flags
>>> argument to fchmodat syscall") when SYS_fchmodat was used and kept when this
>>> year SYS_fchmodat2 started to be used in d0ed307e):
>>> 	int ret = __syscall(SYS_fchmodat2, fd, path, mode, flag);
>>> 	if (ret != -ENOSYS) return __syscall_ret(ret);
>>> 	if (flag != AT_SYMLINK_NOFOLLOW)
>>> 		return __syscall_ret(-EINVAL);
>>> 	struct stat st;
>>> 	int fd2;
>>> 	char proc[15+3*sizeof(int)];
>>> 	if (fstatat(fd, path, &st, flag))
>>> 		return -1;
>>> 	if (S_ISLNK(st.st_mode))
>>> 		return __syscall_ret(-EOPNOTSUPP);
>
>>>> 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
>>> I see there is a kselftest workaround in 4859c257d295 ("selftests: Add fchmodat2
>>> selftest") [4], where fchmodat2 failure on symlink is simply skipped.
>>> Aleksa, you're probably aware of this fchmodat2() failure on symlinks. Does
>>> anybody work or plan to work on fixing it? LTP has policy to not cover kernel
>>> bugs, if it's not expected to be working we might just skip the test as well.
>> If I understand the bug report, the issue is that fchmodat2() doesn't
>> work on symlinks?
> Yes.
>
>> This is intentional -- Christian fixed a tree-wide bug a while ago[1]
>> where some filesystems would change the mode of symlinks despite
>> returning an error (usually EOPNOTSUPP) and IIRC a few others would
>> happily change the mode of symlinks.
> Ah, I've seen this in the past. Thanks a lot for reminding me.
>
>> The current intended behaviour is to always return EOPNOTSUPP, and AFAIK
>> there is no plan to re-enable the changing of symlink modes. EOPNOTSUPP
>> was chosen because that's what filesystems were already returning.
>> (While this is a little confusing, VFS syscalls return EINVAL for an
>> unsupported flag, not EOPNOTSUPP.)
>> The benefit of an AT_SYMLINK_NOFOLLOW flag is not just to to allow a
>> syscall to operate on symlinks, it also allows programs to safely
>> operate on path components without worrying about symlinks being
>> followed (this is relevant for container runtimes, where we are
>> operating on untrusted filesystem roots -- though in the case of
>> fchmodat2(2) you would probably just use AT_EMPTY_PATH in practice). So
>> an error here is actually what you want as a program that uses
>> AT_SYMLINK_NOFOLLOW (since the actual operation is intentionally not
>> supported by filesystems).
Thanks for the explanation. I also have a question around this topic: 
AT_SYMLINK_NOFOLLOW has been added in order to avoid symlinks being 
followed. But if filesystems are not supporting it, why do we have this 
feature? Also, is it an unsupported feature only on certain filesystems?
> Thanks a lot for explaining the background!
>
>> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5d1f903f75a80daa4dfb3d84e114ec8ecbf29956
>>> I see a RFC UAPI related patchset [5] which touches include/uapi/linux/fcntl.h,
>>> but AFAIK it's not related to this problem.
>> Yeah this is unrelated, that patch is about clarifying how AT_* flags
>> are allocated, not syscall behaviour.
> Thanks!
>
>>> Kind regards,
>>> Petr
> @Andrea, I guess we want something like this:
>
> +++ testcases/kernel/syscalls/fchmodat2/fchmodat2_01.c
> @@ -43,9 +43,10 @@ static void test_symbolic_link(void)
>   	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
>   	verify_mode(fd_dir, SNAME, S_IFLNK | 0777);
>   
> -	TST_EXP_PASS(fchmodat2(fd_dir, SNAME, 0640, AT_SYMLINK_NOFOLLOW));
> -	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
> -	verify_mode(fd_dir, SNAME, S_IFLNK | 0640);
> +	if (tst_kvercmp(6, 6, 0) >= 0) {
> +		TST_EXP_FAIL(tst_syscall(__NR_fchmodat2, fd_dir, SNAME, 0640,
> +					 AT_SYMLINK_NOFOLLOW), EOPNOTSUPP);
> +	}
>   }
>   
>   static void test_empty_folder(void)

I think it makes more sense to filter out only filesystems which are not 
supporting this feature (see my comment above).

Best regards,
Andrea


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

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

[Andrea Cervesato]

Hi!

On 8/2/24 07:42, Petr Vorel wrote:
>> On 2024-08-01, Petr Vorel <pvorel@suse.cz> wrote:
>>> Hi all,
>>>> 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:
>>> I would hope that it'd be at least Christian's fork [1], but it's not there.
>>> I suppose nobody is working on the man page.
>>>> 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.
>>> For a record, it's fchmodat2_01.c (from this patchset) which is failing (on
>>> 6.10.1-1.g4c78d6f-default Tumbleweed and 6.6.21-0-lts Alpine, both x86_64 VMs).
>>> Andrea, I would personally just skip test on EOPNOTSUPP (that's what we do in
>>> LTP on EOPNOTSUPP). The question why is not supported and whether is going to be
>>> fixed.
>>> Looking into glibc change 65341f7bbe ("linux: Use fchmodat2 on fchmod for flags
>>> different than 0 (BZ 26401)") one year old change from glibc-2.39 [2] it looks
>>> just accepted behavior (glibc returns EOPNOTSUPP on symlink):
>>> +  /* Some Linux versions with some file systems can actually
>>> +     change symbolic link permissions via /proc, but this is not
>>> +     intentional, and it gives inconsistent results (e.g., error
>>> +     return despite mode change).  The expected behavior is that
>>> +     symbolic link modes cannot be changed at all, and this check
>>> +     enforces that.  */
>>> +  if (S_ISLNK (st.st_mode))
>>> +    {
>>>         __close_nocancel (pathfd);
>>> -      return ret;
>>> +      __set_errno (EOPNOTSUPP);
>>> +      return -1;
>>> +    }
>>> Also musl also behaves the same on his fallback on old kernels [3]
>>> (it started 10 years ago on 0dc48244 ("work around linux's lack of flags
>>> argument to fchmodat syscall") when SYS_fchmodat was used and kept when this
>>> year SYS_fchmodat2 started to be used in d0ed307e):
>>> 	int ret = __syscall(SYS_fchmodat2, fd, path, mode, flag);
>>> 	if (ret != -ENOSYS) return __syscall_ret(ret);
>>> 	if (flag != AT_SYMLINK_NOFOLLOW)
>>> 		return __syscall_ret(-EINVAL);
>>> 	struct stat st;
>>> 	int fd2;
>>> 	char proc[15+3*sizeof(int)];
>>> 	if (fstatat(fd, path, &st, flag))
>>> 		return -1;
>>> 	if (S_ISLNK(st.st_mode))
>>> 		return __syscall_ret(-EOPNOTSUPP);
>
>>>> 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
>>> I see there is a kselftest workaround in 4859c257d295 ("selftests: Add fchmodat2
>>> selftest") [4], where fchmodat2 failure on symlink is simply skipped.
>>> Aleksa, you're probably aware of this fchmodat2() failure on symlinks. Does
>>> anybody work or plan to work on fixing it? LTP has policy to not cover kernel
>>> bugs, if it's not expected to be working we might just skip the test as well.
>> If I understand the bug report, the issue is that fchmodat2() doesn't
>> work on symlinks?
> Yes.
>
>> This is intentional -- Christian fixed a tree-wide bug a while ago[1]
>> where some filesystems would change the mode of symlinks despite
>> returning an error (usually EOPNOTSUPP) and IIRC a few others would
>> happily change the mode of symlinks.
> Ah, I've seen this in the past. Thanks a lot for reminding me.
>
>> The current intended behaviour is to always return EOPNOTSUPP, and AFAIK
>> there is no plan to re-enable the changing of symlink modes. EOPNOTSUPP
>> was chosen because that's what filesystems were already returning.
>> (While this is a little confusing, VFS syscalls return EINVAL for an
>> unsupported flag, not EOPNOTSUPP.)
>> The benefit of an AT_SYMLINK_NOFOLLOW flag is not just to to allow a
>> syscall to operate on symlinks, it also allows programs to safely
>> operate on path components without worrying about symlinks being
>> followed (this is relevant for container runtimes, where we are
>> operating on untrusted filesystem roots -- though in the case of
>> fchmodat2(2) you would probably just use AT_EMPTY_PATH in practice). So
>> an error here is actually what you want as a program that uses
>> AT_SYMLINK_NOFOLLOW (since the actual operation is intentionally not
>> supported by filesystems).
Thanks for the explanation. I also have a question around this topic: 
AT_SYMLINK_NOFOLLOW has been added in order to avoid symlinks being 
followed. But if filesystems are not supporting it, why do we have this 
feature? Also, is it an unsupported feature only on certain filesystems?
> Thanks a lot for explaining the background!
>
>> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5d1f903f75a80daa4dfb3d84e114ec8ecbf29956
>>> I see a RFC UAPI related patchset [5] which touches include/uapi/linux/fcntl.h,
>>> but AFAIK it's not related to this problem.
>> Yeah this is unrelated, that patch is about clarifying how AT_* flags
>> are allocated, not syscall behaviour.
> Thanks!
>
>>> Kind regards,
>>> Petr
> @Andrea, I guess we want something like this:
>
> +++ testcases/kernel/syscalls/fchmodat2/fchmodat2_01.c
> @@ -43,9 +43,10 @@ static void test_symbolic_link(void)
>   	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
>   	verify_mode(fd_dir, SNAME, S_IFLNK | 0777);
>   
> -	TST_EXP_PASS(fchmodat2(fd_dir, SNAME, 0640, AT_SYMLINK_NOFOLLOW));
> -	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
> -	verify_mode(fd_dir, SNAME, S_IFLNK | 0640);
> +	if (tst_kvercmp(6, 6, 0) >= 0) {
> +		TST_EXP_FAIL(tst_syscall(__NR_fchmodat2, fd_dir, SNAME, 0640,
> +					 AT_SYMLINK_NOFOLLOW), EOPNOTSUPP);
> +	}
>   }
>   
>   static void test_empty_folder(void)

I think it makes more sense to filter out only filesystems which are not 
supporting this feature (see my comment above).

Best regards,
Andrea

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

[Petr Vorel]

> Hi!

> On 8/2/24 07:42, Petr Vorel wrote:
> > > On 2024-08-01, Petr Vorel <pvorel@suse.cz> wrote:
> > > > Hi all,
> > > > > 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:
> > > > I would hope that it'd be at least Christian's fork [1], but it's not there.
> > > > I suppose nobody is working on the man page.
> > > > > 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.
> > > > For a record, it's fchmodat2_01.c (from this patchset) which is failing (on
> > > > 6.10.1-1.g4c78d6f-default Tumbleweed and 6.6.21-0-lts Alpine, both x86_64 VMs).
> > > > Andrea, I would personally just skip test on EOPNOTSUPP (that's what we do in
> > > > LTP on EOPNOTSUPP). The question why is not supported and whether is going to be
> > > > fixed.
> > > > Looking into glibc change 65341f7bbe ("linux: Use fchmodat2 on fchmod for flags
> > > > different than 0 (BZ 26401)") one year old change from glibc-2.39 [2] it looks
> > > > just accepted behavior (glibc returns EOPNOTSUPP on symlink):
> > > > +  /* Some Linux versions with some file systems can actually
> > > > +     change symbolic link permissions via /proc, but this is not
> > > > +     intentional, and it gives inconsistent results (e.g., error
> > > > +     return despite mode change).  The expected behavior is that
> > > > +     symbolic link modes cannot be changed at all, and this check
> > > > +     enforces that.  */
> > > > +  if (S_ISLNK (st.st_mode))
> > > > +    {
> > > >         __close_nocancel (pathfd);
> > > > -      return ret;
> > > > +      __set_errno (EOPNOTSUPP);
> > > > +      return -1;
> > > > +    }
> > > > Also musl also behaves the same on his fallback on old kernels [3]
> > > > (it started 10 years ago on 0dc48244 ("work around linux's lack of flags
> > > > argument to fchmodat syscall") when SYS_fchmodat was used and kept when this
> > > > year SYS_fchmodat2 started to be used in d0ed307e):
> > > > 	int ret = __syscall(SYS_fchmodat2, fd, path, mode, flag);
> > > > 	if (ret != -ENOSYS) return __syscall_ret(ret);
> > > > 	if (flag != AT_SYMLINK_NOFOLLOW)
> > > > 		return __syscall_ret(-EINVAL);
> > > > 	struct stat st;
> > > > 	int fd2;
> > > > 	char proc[15+3*sizeof(int)];
> > > > 	if (fstatat(fd, path, &st, flag))
> > > > 		return -1;
> > > > 	if (S_ISLNK(st.st_mode))
> > > > 		return __syscall_ret(-EOPNOTSUPP);

> > > > > 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
> > > > I see there is a kselftest workaround in 4859c257d295 ("selftests: Add fchmodat2
> > > > selftest") [4], where fchmodat2 failure on symlink is simply skipped.
> > > > Aleksa, you're probably aware of this fchmodat2() failure on symlinks. Does
> > > > anybody work or plan to work on fixing it? LTP has policy to not cover kernel
> > > > bugs, if it's not expected to be working we might just skip the test as well.
> > > If I understand the bug report, the issue is that fchmodat2() doesn't
> > > work on symlinks?
> > Yes.

> > > This is intentional -- Christian fixed a tree-wide bug a while ago[1]
> > > where some filesystems would change the mode of symlinks despite
> > > returning an error (usually EOPNOTSUPP) and IIRC a few others would
> > > happily change the mode of symlinks.
> > Ah, I've seen this in the past. Thanks a lot for reminding me.

> > > The current intended behaviour is to always return EOPNOTSUPP, and AFAIK
> > > there is no plan to re-enable the changing of symlink modes. EOPNOTSUPP
> > > was chosen because that's what filesystems were already returning.
> > > (While this is a little confusing, VFS syscalls return EINVAL for an
> > > unsupported flag, not EOPNOTSUPP.)
> > > The benefit of an AT_SYMLINK_NOFOLLOW flag is not just to to allow a
> > > syscall to operate on symlinks, it also allows programs to safely
> > > operate on path components without worrying about symlinks being
> > > followed (this is relevant for container runtimes, where we are
> > > operating on untrusted filesystem roots -- though in the case of
> > > fchmodat2(2) you would probably just use AT_EMPTY_PATH in practice). So
> > > an error here is actually what you want as a program that uses
> > > AT_SYMLINK_NOFOLLOW (since the actual operation is intentionally not
> > > supported by filesystems).
> Thanks for the explanation. I also have a question around this topic:
> AT_SYMLINK_NOFOLLOW has been added in order to avoid symlinks being
> followed. But if filesystems are not supporting it, why do we have this
> feature?

@Aleksa please correct me if I'm wrong.

AFAIK (reading 5d1f903f75a8 commit message [1] and Aleksa's comments) previously
it was an idea which many of the filesystem implemented wrongly - a mess
regardless whether supported by the filesystem or not. I particularly like
changing the mode but fail EOPNOTSUPP. And because glibc and musl did EOPNOTSUPP
anyway (I found that as well), the best was just to follow this in kernel and
unify all filesystems behavior by disabling this in VFS.

> Also, is it an unsupported feature only on certain filesystems?

Disabled in VFS => unsupported on all filesystems.

> > Thanks a lot for explaining the background!

> > > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5d1f903f75a80daa4dfb3d84e114ec8ecbf29956
> > > > I see a RFC UAPI related patchset [5] which touches include/uapi/linux/fcntl.h,
> > > > but AFAIK it's not related to this problem.
> > > Yeah this is unrelated, that patch is about clarifying how AT_* flags
> > > are allocated, not syscall behaviour.
> > Thanks!

> > > > Kind regards,
> > > > Petr
> > @Andrea, I guess we want something like this:

> > +++ testcases/kernel/syscalls/fchmodat2/fchmodat2_01.c
> > @@ -43,9 +43,10 @@ static void test_symbolic_link(void)
> >   	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
> >   	verify_mode(fd_dir, SNAME, S_IFLNK | 0777);
> > -	TST_EXP_PASS(fchmodat2(fd_dir, SNAME, 0640, AT_SYMLINK_NOFOLLOW));
> > -	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
> > -	verify_mode(fd_dir, SNAME, S_IFLNK | 0640);
> > +	if (tst_kvercmp(6, 6, 0) >= 0) {
> > +		TST_EXP_FAIL(tst_syscall(__NR_fchmodat2, fd_dir, SNAME, 0640,
> > +					 AT_SYMLINK_NOFOLLOW), EOPNOTSUPP);
> > +	}
> >   }
> >   static void test_empty_folder(void)

> I think it makes more sense to filter out only filesystems which are not
> supporting this feature (see my comment above).

Due disabled in VFS since 5d1f903f75a8 all filesystems fail with EOPNOTSUPP,
thus failure in LTP (TBROK), which needs to be handled.  Before 5d1f903f75a8
some of them actually changed the mode (e.g. btrfs, ext4, xfs), but that's no
longer the case. And because it got backported to all stable/LTS, we can expect
this is the correct behavior.

Kind regards,
Petr

> Best regards,
> Andrea


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

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

[Petr Vorel]

> Hi!

> On 8/2/24 07:42, Petr Vorel wrote:
> > > On 2024-08-01, Petr Vorel <pvorel@suse.cz> wrote:
> > > > Hi all,
> > > > > 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:
> > > > I would hope that it'd be at least Christian's fork [1], but it's not there.
> > > > I suppose nobody is working on the man page.
> > > > > 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.
> > > > For a record, it's fchmodat2_01.c (from this patchset) which is failing (on
> > > > 6.10.1-1.g4c78d6f-default Tumbleweed and 6.6.21-0-lts Alpine, both x86_64 VMs).
> > > > Andrea, I would personally just skip test on EOPNOTSUPP (that's what we do in
> > > > LTP on EOPNOTSUPP). The question why is not supported and whether is going to be
> > > > fixed.
> > > > Looking into glibc change 65341f7bbe ("linux: Use fchmodat2 on fchmod for flags
> > > > different than 0 (BZ 26401)") one year old change from glibc-2.39 [2] it looks
> > > > just accepted behavior (glibc returns EOPNOTSUPP on symlink):
> > > > +  /* Some Linux versions with some file systems can actually
> > > > +     change symbolic link permissions via /proc, but this is not
> > > > +     intentional, and it gives inconsistent results (e.g., error
> > > > +     return despite mode change).  The expected behavior is that
> > > > +     symbolic link modes cannot be changed at all, and this check
> > > > +     enforces that.  */
> > > > +  if (S_ISLNK (st.st_mode))
> > > > +    {
> > > >         __close_nocancel (pathfd);
> > > > -      return ret;
> > > > +      __set_errno (EOPNOTSUPP);
> > > > +      return -1;
> > > > +    }
> > > > Also musl also behaves the same on his fallback on old kernels [3]
> > > > (it started 10 years ago on 0dc48244 ("work around linux's lack of flags
> > > > argument to fchmodat syscall") when SYS_fchmodat was used and kept when this
> > > > year SYS_fchmodat2 started to be used in d0ed307e):
> > > > 	int ret = __syscall(SYS_fchmodat2, fd, path, mode, flag);
> > > > 	if (ret != -ENOSYS) return __syscall_ret(ret);
> > > > 	if (flag != AT_SYMLINK_NOFOLLOW)
> > > > 		return __syscall_ret(-EINVAL);
> > > > 	struct stat st;
> > > > 	int fd2;
> > > > 	char proc[15+3*sizeof(int)];
> > > > 	if (fstatat(fd, path, &st, flag))
> > > > 		return -1;
> > > > 	if (S_ISLNK(st.st_mode))
> > > > 		return __syscall_ret(-EOPNOTSUPP);

> > > > > 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
> > > > I see there is a kselftest workaround in 4859c257d295 ("selftests: Add fchmodat2
> > > > selftest") [4], where fchmodat2 failure on symlink is simply skipped.
> > > > Aleksa, you're probably aware of this fchmodat2() failure on symlinks. Does
> > > > anybody work or plan to work on fixing it? LTP has policy to not cover kernel
> > > > bugs, if it's not expected to be working we might just skip the test as well.
> > > If I understand the bug report, the issue is that fchmodat2() doesn't
> > > work on symlinks?
> > Yes.

> > > This is intentional -- Christian fixed a tree-wide bug a while ago[1]
> > > where some filesystems would change the mode of symlinks despite
> > > returning an error (usually EOPNOTSUPP) and IIRC a few others would
> > > happily change the mode of symlinks.
> > Ah, I've seen this in the past. Thanks a lot for reminding me.

> > > The current intended behaviour is to always return EOPNOTSUPP, and AFAIK
> > > there is no plan to re-enable the changing of symlink modes. EOPNOTSUPP
> > > was chosen because that's what filesystems were already returning.
> > > (While this is a little confusing, VFS syscalls return EINVAL for an
> > > unsupported flag, not EOPNOTSUPP.)
> > > The benefit of an AT_SYMLINK_NOFOLLOW flag is not just to to allow a
> > > syscall to operate on symlinks, it also allows programs to safely
> > > operate on path components without worrying about symlinks being
> > > followed (this is relevant for container runtimes, where we are
> > > operating on untrusted filesystem roots -- though in the case of
> > > fchmodat2(2) you would probably just use AT_EMPTY_PATH in practice). So
> > > an error here is actually what you want as a program that uses
> > > AT_SYMLINK_NOFOLLOW (since the actual operation is intentionally not
> > > supported by filesystems).
> Thanks for the explanation. I also have a question around this topic:
> AT_SYMLINK_NOFOLLOW has been added in order to avoid symlinks being
> followed. But if filesystems are not supporting it, why do we have this
> feature?

@Aleksa please correct me if I'm wrong.

AFAIK (reading 5d1f903f75a8 commit message [1] and Aleksa's comments) previously
it was an idea which many of the filesystem implemented wrongly - a mess
regardless whether supported by the filesystem or not. I particularly like
changing the mode but fail EOPNOTSUPP. And because glibc and musl did EOPNOTSUPP
anyway (I found that as well), the best was just to follow this in kernel and
unify all filesystems behavior by disabling this in VFS.

> Also, is it an unsupported feature only on certain filesystems?

Disabled in VFS => unsupported on all filesystems.

> > Thanks a lot for explaining the background!

> > > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5d1f903f75a80daa4dfb3d84e114ec8ecbf29956
> > > > I see a RFC UAPI related patchset [5] which touches include/uapi/linux/fcntl.h,
> > > > but AFAIK it's not related to this problem.
> > > Yeah this is unrelated, that patch is about clarifying how AT_* flags
> > > are allocated, not syscall behaviour.
> > Thanks!

> > > > Kind regards,
> > > > Petr
> > @Andrea, I guess we want something like this:

> > +++ testcases/kernel/syscalls/fchmodat2/fchmodat2_01.c
> > @@ -43,9 +43,10 @@ static void test_symbolic_link(void)
> >   	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
> >   	verify_mode(fd_dir, SNAME, S_IFLNK | 0777);
> > -	TST_EXP_PASS(fchmodat2(fd_dir, SNAME, 0640, AT_SYMLINK_NOFOLLOW));
> > -	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
> > -	verify_mode(fd_dir, SNAME, S_IFLNK | 0640);
> > +	if (tst_kvercmp(6, 6, 0) >= 0) {
> > +		TST_EXP_FAIL(tst_syscall(__NR_fchmodat2, fd_dir, SNAME, 0640,
> > +					 AT_SYMLINK_NOFOLLOW), EOPNOTSUPP);
> > +	}
> >   }
> >   static void test_empty_folder(void)

> I think it makes more sense to filter out only filesystems which are not
> supporting this feature (see my comment above).

Due disabled in VFS since 5d1f903f75a8 all filesystems fail with EOPNOTSUPP,
thus failure in LTP (TBROK), which needs to be handled.  Before 5d1f903f75a8
some of them actually changed the mode (e.g. btrfs, ext4, xfs), but that's no
longer the case. And because it got backported to all stable/LTS, we can expect
this is the correct behavior.

Kind regards,
Petr

> Best regards,
> Andrea

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

[Andrea Cervesato via ltp]

On 8/2/24 09:49, Petr Vorel wrote:
>> Hi!
>> On 8/2/24 07:42, Petr Vorel wrote:
>>>> On 2024-08-01, Petr Vorel <pvorel@suse.cz> wrote:
>>>>> Hi all,
>>>>>> 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:
>>>>> I would hope that it'd be at least Christian's fork [1], but it's not there.
>>>>> I suppose nobody is working on the man page.
>>>>>> 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.
>>>>> For a record, it's fchmodat2_01.c (from this patchset) which is failing (on
>>>>> 6.10.1-1.g4c78d6f-default Tumbleweed and 6.6.21-0-lts Alpine, both x86_64 VMs).
>>>>> Andrea, I would personally just skip test on EOPNOTSUPP (that's what we do in
>>>>> LTP on EOPNOTSUPP). The question why is not supported and whether is going to be
>>>>> fixed.
>>>>> Looking into glibc change 65341f7bbe ("linux: Use fchmodat2 on fchmod for flags
>>>>> different than 0 (BZ 26401)") one year old change from glibc-2.39 [2] it looks
>>>>> just accepted behavior (glibc returns EOPNOTSUPP on symlink):
>>>>> +  /* Some Linux versions with some file systems can actually
>>>>> +     change symbolic link permissions via /proc, but this is not
>>>>> +     intentional, and it gives inconsistent results (e.g., error
>>>>> +     return despite mode change).  The expected behavior is that
>>>>> +     symbolic link modes cannot be changed at all, and this check
>>>>> +     enforces that.  */
>>>>> +  if (S_ISLNK (st.st_mode))
>>>>> +    {
>>>>>          __close_nocancel (pathfd);
>>>>> -      return ret;
>>>>> +      __set_errno (EOPNOTSUPP);
>>>>> +      return -1;
>>>>> +    }
>>>>> Also musl also behaves the same on his fallback on old kernels [3]
>>>>> (it started 10 years ago on 0dc48244 ("work around linux's lack of flags
>>>>> argument to fchmodat syscall") when SYS_fchmodat was used and kept when this
>>>>> year SYS_fchmodat2 started to be used in d0ed307e):
>>>>> 	int ret = __syscall(SYS_fchmodat2, fd, path, mode, flag);
>>>>> 	if (ret != -ENOSYS) return __syscall_ret(ret);
>>>>> 	if (flag != AT_SYMLINK_NOFOLLOW)
>>>>> 		return __syscall_ret(-EINVAL);
>>>>> 	struct stat st;
>>>>> 	int fd2;
>>>>> 	char proc[15+3*sizeof(int)];
>>>>> 	if (fstatat(fd, path, &st, flag))
>>>>> 		return -1;
>>>>> 	if (S_ISLNK(st.st_mode))
>>>>> 		return __syscall_ret(-EOPNOTSUPP);
>>>>>> 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
>>>>> I see there is a kselftest workaround in 4859c257d295 ("selftests: Add fchmodat2
>>>>> selftest") [4], where fchmodat2 failure on symlink is simply skipped.
>>>>> Aleksa, you're probably aware of this fchmodat2() failure on symlinks. Does
>>>>> anybody work or plan to work on fixing it? LTP has policy to not cover kernel
>>>>> bugs, if it's not expected to be working we might just skip the test as well.
>>>> If I understand the bug report, the issue is that fchmodat2() doesn't
>>>> work on symlinks?
>>> Yes.
>>>> This is intentional -- Christian fixed a tree-wide bug a while ago[1]
>>>> where some filesystems would change the mode of symlinks despite
>>>> returning an error (usually EOPNOTSUPP) and IIRC a few others would
>>>> happily change the mode of symlinks.
>>> Ah, I've seen this in the past. Thanks a lot for reminding me.
>>>> The current intended behaviour is to always return EOPNOTSUPP, and AFAIK
>>>> there is no plan to re-enable the changing of symlink modes. EOPNOTSUPP
>>>> was chosen because that's what filesystems were already returning.
>>>> (While this is a little confusing, VFS syscalls return EINVAL for an
>>>> unsupported flag, not EOPNOTSUPP.)
>>>> The benefit of an AT_SYMLINK_NOFOLLOW flag is not just to to allow a
>>>> syscall to operate on symlinks, it also allows programs to safely
>>>> operate on path components without worrying about symlinks being
>>>> followed (this is relevant for container runtimes, where we are
>>>> operating on untrusted filesystem roots -- though in the case of
>>>> fchmodat2(2) you would probably just use AT_EMPTY_PATH in practice). So
>>>> an error here is actually what you want as a program that uses
>>>> AT_SYMLINK_NOFOLLOW (since the actual operation is intentionally not
>>>> supported by filesystems).
>> Thanks for the explanation. I also have a question around this topic:
>> AT_SYMLINK_NOFOLLOW has been added in order to avoid symlinks being
>> followed. But if filesystems are not supporting it, why do we have this
>> feature?
> @Aleksa please correct me if I'm wrong.
>
> AFAIK (reading 5d1f903f75a8 commit message [1] and Aleksa's comments) previously
> it was an idea which many of the filesystem implemented wrongly - a mess
> regardless whether supported by the filesystem or not. I particularly like
> changing the mode but fail EOPNOTSUPP. And because glibc and musl did EOPNOTSUPP
> anyway (I found that as well), the best was just to follow this in kernel and
> unify all filesystems behavior by disabling this in VFS.
>
>> Also, is it an unsupported feature only on certain filesystems?
> Disabled in VFS => unsupported on all filesystems.
Ah right, that's quite obvious indeed.
>>> Thanks a lot for explaining the background!
>>>> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5d1f903f75a80daa4dfb3d84e114ec8ecbf29956
>>>>> I see a RFC UAPI related patchset [5] which touches include/uapi/linux/fcntl.h,
>>>>> but AFAIK it's not related to this problem.
>>>> Yeah this is unrelated, that patch is about clarifying how AT_* flags
>>>> are allocated, not syscall behaviour.
>>> Thanks!
>>>>> Kind regards,
>>>>> Petr
>>> @Andrea, I guess we want something like this:
>>> +++ testcases/kernel/syscalls/fchmodat2/fchmodat2_01.c
>>> @@ -43,9 +43,10 @@ static void test_symbolic_link(void)
>>>    	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
>>>    	verify_mode(fd_dir, SNAME, S_IFLNK | 0777);
>>> -	TST_EXP_PASS(fchmodat2(fd_dir, SNAME, 0640, AT_SYMLINK_NOFOLLOW));
>>> -	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
>>> -	verify_mode(fd_dir, SNAME, S_IFLNK | 0640);
>>> +	if (tst_kvercmp(6, 6, 0) >= 0) {
>>> +		TST_EXP_FAIL(tst_syscall(__NR_fchmodat2, fd_dir, SNAME, 0640,
>>> +					 AT_SYMLINK_NOFOLLOW), EOPNOTSUPP);
>>> +	}
>>>    }
>>>    static void test_empty_folder(void)
>> I think it makes more sense to filter out only filesystems which are not
>> supporting this feature (see my comment above).
> Due disabled in VFS since 5d1f903f75a8 all filesystems fail with EOPNOTSUPP,
> thus failure in LTP (TBROK), which needs to be handled.  Before 5d1f903f75a8
> some of them actually changed the mode (e.g. btrfs, ext4, xfs), but that's no
> longer the case. And because it got backported to all stable/LTS, we can expect
> this is the correct behavior.
>
> Kind regards,
> Petr
>
>> Best regards,
>> Andrea

I guess we can just assume EOPNOTSUPP for the test. I will add the .tag 
and errno check.

Thanks,
Andrea


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

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

[Andrea Cervesato]

On 8/2/24 09:49, Petr Vorel wrote:
>> Hi!
>> On 8/2/24 07:42, Petr Vorel wrote:
>>>> On 2024-08-01, Petr Vorel <pvorel@suse.cz> wrote:
>>>>> Hi all,
>>>>>> 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:
>>>>> I would hope that it'd be at least Christian's fork [1], but it's not there.
>>>>> I suppose nobody is working on the man page.
>>>>>> 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.
>>>>> For a record, it's fchmodat2_01.c (from this patchset) which is failing (on
>>>>> 6.10.1-1.g4c78d6f-default Tumbleweed and 6.6.21-0-lts Alpine, both x86_64 VMs).
>>>>> Andrea, I would personally just skip test on EOPNOTSUPP (that's what we do in
>>>>> LTP on EOPNOTSUPP). The question why is not supported and whether is going to be
>>>>> fixed.
>>>>> Looking into glibc change 65341f7bbe ("linux: Use fchmodat2 on fchmod for flags
>>>>> different than 0 (BZ 26401)") one year old change from glibc-2.39 [2] it looks
>>>>> just accepted behavior (glibc returns EOPNOTSUPP on symlink):
>>>>> +  /* Some Linux versions with some file systems can actually
>>>>> +     change symbolic link permissions via /proc, but this is not
>>>>> +     intentional, and it gives inconsistent results (e.g., error
>>>>> +     return despite mode change).  The expected behavior is that
>>>>> +     symbolic link modes cannot be changed at all, and this check
>>>>> +     enforces that.  */
>>>>> +  if (S_ISLNK (st.st_mode))
>>>>> +    {
>>>>>          __close_nocancel (pathfd);
>>>>> -      return ret;
>>>>> +      __set_errno (EOPNOTSUPP);
>>>>> +      return -1;
>>>>> +    }
>>>>> Also musl also behaves the same on his fallback on old kernels [3]
>>>>> (it started 10 years ago on 0dc48244 ("work around linux's lack of flags
>>>>> argument to fchmodat syscall") when SYS_fchmodat was used and kept when this
>>>>> year SYS_fchmodat2 started to be used in d0ed307e):
>>>>> 	int ret = __syscall(SYS_fchmodat2, fd, path, mode, flag);
>>>>> 	if (ret != -ENOSYS) return __syscall_ret(ret);
>>>>> 	if (flag != AT_SYMLINK_NOFOLLOW)
>>>>> 		return __syscall_ret(-EINVAL);
>>>>> 	struct stat st;
>>>>> 	int fd2;
>>>>> 	char proc[15+3*sizeof(int)];
>>>>> 	if (fstatat(fd, path, &st, flag))
>>>>> 		return -1;
>>>>> 	if (S_ISLNK(st.st_mode))
>>>>> 		return __syscall_ret(-EOPNOTSUPP);
>>>>>> 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
>>>>> I see there is a kselftest workaround in 4859c257d295 ("selftests: Add fchmodat2
>>>>> selftest") [4], where fchmodat2 failure on symlink is simply skipped.
>>>>> Aleksa, you're probably aware of this fchmodat2() failure on symlinks. Does
>>>>> anybody work or plan to work on fixing it? LTP has policy to not cover kernel
>>>>> bugs, if it's not expected to be working we might just skip the test as well.
>>>> If I understand the bug report, the issue is that fchmodat2() doesn't
>>>> work on symlinks?
>>> Yes.
>>>> This is intentional -- Christian fixed a tree-wide bug a while ago[1]
>>>> where some filesystems would change the mode of symlinks despite
>>>> returning an error (usually EOPNOTSUPP) and IIRC a few others would
>>>> happily change the mode of symlinks.
>>> Ah, I've seen this in the past. Thanks a lot for reminding me.
>>>> The current intended behaviour is to always return EOPNOTSUPP, and AFAIK
>>>> there is no plan to re-enable the changing of symlink modes. EOPNOTSUPP
>>>> was chosen because that's what filesystems were already returning.
>>>> (While this is a little confusing, VFS syscalls return EINVAL for an
>>>> unsupported flag, not EOPNOTSUPP.)
>>>> The benefit of an AT_SYMLINK_NOFOLLOW flag is not just to to allow a
>>>> syscall to operate on symlinks, it also allows programs to safely
>>>> operate on path components without worrying about symlinks being
>>>> followed (this is relevant for container runtimes, where we are
>>>> operating on untrusted filesystem roots -- though in the case of
>>>> fchmodat2(2) you would probably just use AT_EMPTY_PATH in practice). So
>>>> an error here is actually what you want as a program that uses
>>>> AT_SYMLINK_NOFOLLOW (since the actual operation is intentionally not
>>>> supported by filesystems).
>> Thanks for the explanation. I also have a question around this topic:
>> AT_SYMLINK_NOFOLLOW has been added in order to avoid symlinks being
>> followed. But if filesystems are not supporting it, why do we have this
>> feature?
> @Aleksa please correct me if I'm wrong.
>
> AFAIK (reading 5d1f903f75a8 commit message [1] and Aleksa's comments) previously
> it was an idea which many of the filesystem implemented wrongly - a mess
> regardless whether supported by the filesystem or not. I particularly like
> changing the mode but fail EOPNOTSUPP. And because glibc and musl did EOPNOTSUPP
> anyway (I found that as well), the best was just to follow this in kernel and
> unify all filesystems behavior by disabling this in VFS.
>
>> Also, is it an unsupported feature only on certain filesystems?
> Disabled in VFS => unsupported on all filesystems.
Ah right, that's quite obvious indeed.
>>> Thanks a lot for explaining the background!
>>>> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5d1f903f75a80daa4dfb3d84e114ec8ecbf29956
>>>>> I see a RFC UAPI related patchset [5] which touches include/uapi/linux/fcntl.h,
>>>>> but AFAIK it's not related to this problem.
>>>> Yeah this is unrelated, that patch is about clarifying how AT_* flags
>>>> are allocated, not syscall behaviour.
>>> Thanks!
>>>>> Kind regards,
>>>>> Petr
>>> @Andrea, I guess we want something like this:
>>> +++ testcases/kernel/syscalls/fchmodat2/fchmodat2_01.c
>>> @@ -43,9 +43,10 @@ static void test_symbolic_link(void)
>>>    	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
>>>    	verify_mode(fd_dir, SNAME, S_IFLNK | 0777);
>>> -	TST_EXP_PASS(fchmodat2(fd_dir, SNAME, 0640, AT_SYMLINK_NOFOLLOW));
>>> -	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
>>> -	verify_mode(fd_dir, SNAME, S_IFLNK | 0640);
>>> +	if (tst_kvercmp(6, 6, 0) >= 0) {
>>> +		TST_EXP_FAIL(tst_syscall(__NR_fchmodat2, fd_dir, SNAME, 0640,
>>> +					 AT_SYMLINK_NOFOLLOW), EOPNOTSUPP);
>>> +	}
>>>    }
>>>    static void test_empty_folder(void)
>> I think it makes more sense to filter out only filesystems which are not
>> supporting this feature (see my comment above).
> Due disabled in VFS since 5d1f903f75a8 all filesystems fail with EOPNOTSUPP,
> thus failure in LTP (TBROK), which needs to be handled.  Before 5d1f903f75a8
> some of them actually changed the mode (e.g. btrfs, ext4, xfs), but that's no
> longer the case. And because it got backported to all stable/LTS, we can expect
> this is the correct behavior.
>
> Kind regards,
> Petr
>
>> Best regards,
>> Andrea

I guess we can just assume EOPNOTSUPP for the test. I will add the .tag 
and errno check.

Thanks,
Andrea

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

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 9561 bytes --]

On 2024-08-02, Andrea Cervesato <andrea.cervesato@suse.com> wrote:
> Hi!
> 
> On 8/2/24 07:42, Petr Vorel wrote:
> > > On 2024-08-01, Petr Vorel <pvorel@suse.cz> wrote:
> > > > Hi all,
> > > > > 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:
> > > > I would hope that it'd be at least Christian's fork [1], but it's not there.
> > > > I suppose nobody is working on the man page.
> > > > > 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.
> > > > For a record, it's fchmodat2_01.c (from this patchset) which is failing (on
> > > > 6.10.1-1.g4c78d6f-default Tumbleweed and 6.6.21-0-lts Alpine, both x86_64 VMs).
> > > > Andrea, I would personally just skip test on EOPNOTSUPP (that's what we do in
> > > > LTP on EOPNOTSUPP). The question why is not supported and whether is going to be
> > > > fixed.
> > > > Looking into glibc change 65341f7bbe ("linux: Use fchmodat2 on fchmod for flags
> > > > different than 0 (BZ 26401)") one year old change from glibc-2.39 [2] it looks
> > > > just accepted behavior (glibc returns EOPNOTSUPP on symlink):
> > > > +  /* Some Linux versions with some file systems can actually
> > > > +     change symbolic link permissions via /proc, but this is not
> > > > +     intentional, and it gives inconsistent results (e.g., error
> > > > +     return despite mode change).  The expected behavior is that
> > > > +     symbolic link modes cannot be changed at all, and this check
> > > > +     enforces that.  */
> > > > +  if (S_ISLNK (st.st_mode))
> > > > +    {
> > > >         __close_nocancel (pathfd);
> > > > -      return ret;
> > > > +      __set_errno (EOPNOTSUPP);
> > > > +      return -1;
> > > > +    }
> > > > Also musl also behaves the same on his fallback on old kernels [3]
> > > > (it started 10 years ago on 0dc48244 ("work around linux's lack of flags
> > > > argument to fchmodat syscall") when SYS_fchmodat was used and kept when this
> > > > year SYS_fchmodat2 started to be used in d0ed307e):
> > > > 	int ret = __syscall(SYS_fchmodat2, fd, path, mode, flag);
> > > > 	if (ret != -ENOSYS) return __syscall_ret(ret);
> > > > 	if (flag != AT_SYMLINK_NOFOLLOW)
> > > > 		return __syscall_ret(-EINVAL);
> > > > 	struct stat st;
> > > > 	int fd2;
> > > > 	char proc[15+3*sizeof(int)];
> > > > 	if (fstatat(fd, path, &st, flag))
> > > > 		return -1;
> > > > 	if (S_ISLNK(st.st_mode))
> > > > 		return __syscall_ret(-EOPNOTSUPP);
> > 
> > > > > 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
> > > > I see there is a kselftest workaround in 4859c257d295 ("selftests: Add fchmodat2
> > > > selftest") [4], where fchmodat2 failure on symlink is simply skipped.
> > > > Aleksa, you're probably aware of this fchmodat2() failure on symlinks. Does
> > > > anybody work or plan to work on fixing it? LTP has policy to not cover kernel
> > > > bugs, if it's not expected to be working we might just skip the test as well.
> > > If I understand the bug report, the issue is that fchmodat2() doesn't
> > > work on symlinks?
> > Yes.
> > 
> > > This is intentional -- Christian fixed a tree-wide bug a while ago[1]
> > > where some filesystems would change the mode of symlinks despite
> > > returning an error (usually EOPNOTSUPP) and IIRC a few others would
> > > happily change the mode of symlinks.
> > Ah, I've seen this in the past. Thanks a lot for reminding me.
> > 
> > > The current intended behaviour is to always return EOPNOTSUPP, and AFAIK
> > > there is no plan to re-enable the changing of symlink modes. EOPNOTSUPP
> > > was chosen because that's what filesystems were already returning.
> > > (While this is a little confusing, VFS syscalls return EINVAL for an
> > > unsupported flag, not EOPNOTSUPP.)
> > > The benefit of an AT_SYMLINK_NOFOLLOW flag is not just to to allow a
> > > syscall to operate on symlinks, it also allows programs to safely
> > > operate on path components without worrying about symlinks being
> > > followed (this is relevant for container runtimes, where we are
> > > operating on untrusted filesystem roots -- though in the case of
> > > fchmodat2(2) you would probably just use AT_EMPTY_PATH in practice). So
> > > an error here is actually what you want as a program that uses
> > > AT_SYMLINK_NOFOLLOW (since the actual operation is intentionally not
> > > supported by filesystems).
>
> Thanks for the explanation. I also have a question around this topic:
> AT_SYMLINK_NOFOLLOW has been added in order to avoid symlinks being
> followed. [...] Also, is it an unsupported feature only on certain
> filesystems?

AFAIK it was never supposed to be supported by filesystems, there was a
series of bugs that lead to it working by accident and that has now been
fixed. It's blocked by the VFS now, so no filesystems support it.

EOPNOTSUPP was chosen as the error code to avoid breaking userspace
because that was the error code used by glibc as well as some
filesystems. I wouldn't interpret this EOPNOTSUPP as meaning "we plan to
add support for this in the future" nor "only some filesystems don't
support this" -- I would just treat it if it were ELOOP.

> But if filesystems are not supporting it, why do we have this feature?

The problem being solved is the same as O_NOFOLLOW. Before O_PATH,
O_NOFOLLOW would always return an error -- this was what you wanted
because you wanted to open a file without following (trailing) symlinks.
If the final component was a symlink you wanted to get an error rather
than following the symlink and opening some other file (this could be a
particular problem if you are dealing with an extracted rootfs tree --
symlinks could escape to the host and you could end up operating on host
files). These kinds of problems crop up a lot when dealing with
privileged tools that need to deal with untrusted directory trees. (If
you're interested, I'm working on a path resolution library that makes
use of these kinds of tricks[1].)

That being said, O_NOFOLLOW/AT_SYMLINK_NOFOLLOW doesn't help you with
symlink path components. That's what openat2(RESOLVE_IN_ROOT) or
openat2(RESOLVE_NO_SYMLINKS) is for. So in most cases, I suspect people
are going to want to use openat2+fchmodat2(AT_EMPTY_PATH) instead but
AT_SYMLINK_NOFOLLOW does still make sense for this kind of syscall.

Modern VFS syscalls are designed to make sure that it's possible for you
to either operate on a file descriptor (AT_EMPTY_PATH) or to ensure
trailing symlinks are not followed (AT_SYMLINK_NOFOLLOW). This lets you
make sure that you are never operating on a path that could change
underneath you. With AT_SYMLINK_NOFOLLOW as long as the path doesn't
contain "/" you can safely operate on any path (you just need to first
open the parent directory, either with openat2(RESOLVE_*) or by opening
each component of the path using openat[2,3]).

[1]: https://github.com/openSUSE/libpathrs
[2]: https://github.com/cyphar/filepath-securejoin/blob/v0.3.1/lookup_linux.go#L178
[3]: https://github.com/openSUSE/libpathrs/blob/de588611aa9a/src/resolvers/opath.rs

> > Thanks a lot for explaining the background!
> > 
> > > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5d1f903f75a80daa4dfb3d84e114ec8ecbf29956
> > > > I see a RFC UAPI related patchset [5] which touches include/uapi/linux/fcntl.h,
> > > > but AFAIK it's not related to this problem.
> > > Yeah this is unrelated, that patch is about clarifying how AT_* flags
> > > are allocated, not syscall behaviour.
> > Thanks!
> > 
> > > > Kind regards,
> > > > Petr
> > @Andrea, I guess we want something like this:
> > 
> > +++ testcases/kernel/syscalls/fchmodat2/fchmodat2_01.c
> > @@ -43,9 +43,10 @@ static void test_symbolic_link(void)
> >   	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
> >   	verify_mode(fd_dir, SNAME, S_IFLNK | 0777);
> > -	TST_EXP_PASS(fchmodat2(fd_dir, SNAME, 0640, AT_SYMLINK_NOFOLLOW));
> > -	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
> > -	verify_mode(fd_dir, SNAME, S_IFLNK | 0640);
> > +	if (tst_kvercmp(6, 6, 0) >= 0) {
> > +		TST_EXP_FAIL(tst_syscall(__NR_fchmodat2, fd_dir, SNAME, 0640,
> > +					 AT_SYMLINK_NOFOLLOW), EOPNOTSUPP);
> > +	}
> >   }
> >   static void test_empty_folder(void)
> 
> I think it makes more sense to filter out only filesystems which are not
> supporting this feature (see my comment above).
> 
> Best regards,
> Andrea
> 

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Petr Vorel]

Hi all,

...
> @Andrea, I guess we want something like this:

> +++ testcases/kernel/syscalls/fchmodat2/fchmodat2_01.c
> @@ -43,9 +43,10 @@ static void test_symbolic_link(void)
>  	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
>  	verify_mode(fd_dir, SNAME, S_IFLNK | 0777);

> -	TST_EXP_PASS(fchmodat2(fd_dir, SNAME, 0640, AT_SYMLINK_NOFOLLOW));
> -	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
> -	verify_mode(fd_dir, SNAME, S_IFLNK | 0640);
> +	if (tst_kvercmp(6, 6, 0) >= 0) {
> +		TST_EXP_FAIL(tst_syscall(__NR_fchmodat2, fd_dir, SNAME, 0640,
> +					 AT_SYMLINK_NOFOLLOW), EOPNOTSUPP);
> +	}
>  }

Actually, this has been backported to all stable/LTS trees (up to 4.19).
Thus there should not be a version check, but instead suggesting a missing
5d1f903f75a8 commit.

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

Kind regards,
Petr

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

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

[Petr Vorel]

Hi all,

...
> @Andrea, I guess we want something like this:

> +++ testcases/kernel/syscalls/fchmodat2/fchmodat2_01.c
> @@ -43,9 +43,10 @@ static void test_symbolic_link(void)
>  	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
>  	verify_mode(fd_dir, SNAME, S_IFLNK | 0777);

> -	TST_EXP_PASS(fchmodat2(fd_dir, SNAME, 0640, AT_SYMLINK_NOFOLLOW));
> -	verify_mode(fd_dir, FNAME, S_IFREG | 0700);
> -	verify_mode(fd_dir, SNAME, S_IFLNK | 0640);
> +	if (tst_kvercmp(6, 6, 0) >= 0) {
> +		TST_EXP_FAIL(tst_syscall(__NR_fchmodat2, fd_dir, SNAME, 0640,
> +					 AT_SYMLINK_NOFOLLOW), EOPNOTSUPP);
> +	}
>  }

Actually, this has been backported to all stable/LTS trees (up to 4.19).
Thus there should not be a version check, but instead suggesting a missing
5d1f903f75a8 commit.

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

Kind regards,
Petr