[PATCH RESEND v3 0/2] fhandle: expose u64 mount id to name_to_handle_at(2)

[PATCH RESEND v3 0/2] fhandle: expose u64 mount id to name_to_handle_at(2)

[Aleksa Sarai]

Now that we provide a unique 64-bit mount ID interface in statx(2), we
can now provide a race-free way for name_to_handle_at(2) to provide a
file handle and corresponding mount without needing to worry about
racing with /proc/mountinfo parsing or having to open a file just to do
statx(2).

While this is not necessary if you are using AT_EMPTY_PATH and don't
care about an extra statx(2) call, users that pass full paths into
name_to_handle_at(2) need to know which mount the file handle comes from
(to make sure they don't try to open_by_handle_at a file handle from a
different filesystem) and switching to AT_EMPTY_PATH would require
allocating a file for every name_to_handle_at(2) call, turning

  err = name_to_handle_at(-EBADF, "/foo/bar/baz", &handle, &mntid,
                          AT_HANDLE_MNT_ID_UNIQUE);

into

  int fd = openat(-EBADF, "/foo/bar/baz", O_PATH | O_CLOEXEC);
  err1 = name_to_handle_at(fd, "", &handle, &unused_mntid, AT_EMPTY_PATH);
  err2 = statx(fd, "", AT_EMPTY_PATH, STATX_MNT_ID_UNIQUE, &statxbuf);
  mntid = statxbuf.stx_mnt_id;
  close(fd);

Also, this series adds a patch to clarify how AT_* flag allocation
should work going forwards.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
Resend:
- v3: <https://lore.kernel.org/r/20240801-exportfs-u64-mount-id-v3-0-be5d6283144a@cyphar.com>
Changes in v3:
- Added a patch describing how AT_* flags should be allocated in the
  future, based on Amir's suggestions.
- Included AT_* aliases for RENAME_* flags to further indicate that
  renameat2(2) is an *at(2) syscall and to indicate that those flags
  have been allocated already in the per-syscall range.
- Switched AT_HANDLE_MNT_ID_UNIQUE to use 0x01 (to reuse
  (AT_)RENAME_NOREPLACE).
- v2: <https://lore.kernel.org/r/20240523-exportfs-u64-mount-id-v2-1-f9f959f17eb1@cyphar.com>
Changes in v2:
- Fixed a few minor compiler warnings and a buggy copy_to_user() check.
- Rename AT_HANDLE_UNIQUE_MOUNT_ID -> AT_HANDLE_MNT_ID_UNIQUE to match statx.
- Switched to using an AT_* bit from 0xFF and defining that range as
  being "per-syscall" for future usage.
- Sync tools/ copy of <linux/fcntl.h> to include changes.
- v1: <https://lore.kernel.org/r/20240520-exportfs-u64-mount-id-v1-1-f55fd9215b8e@cyphar.com>

---
Aleksa Sarai (2):
      uapi: explain how per-syscall AT_* flags should be allocated
      fhandle: expose u64 mount id to name_to_handle_at(2)

 fs/fhandle.c                                       | 29 ++++++--
 include/linux/syscalls.h                           |  2 +-
 include/uapi/linux/fcntl.h                         | 81 ++++++++++++++-------
 tools/perf/trace/beauty/include/uapi/linux/fcntl.h | 84 +++++++++++++++-------
 4 files changed, 140 insertions(+), 56 deletions(-)
---
base-commit: 766508e7e2c5075eb744cb29b8cef6fa835b0344
change-id: 20240515-exportfs-u64-mount-id-9ebb5c58b53c

Best regards,
-- 
Aleksa Sarai <cyphar@cyphar.com>

[PATCH RESEND v3 1/2] uapi: explain how per-syscall AT_* flags should be allocated

[Aleksa Sarai]

Unfortunately, the way we have gone about adding new AT_* flags has
been a little messy. In the beginning, all of the AT_* flags had generic
meanings and so it made sense to share the flag bits indiscriminately.
However, we inevitably ran into syscalls that needed their own
syscall-specific flags. Due to the lack of a planned out policy, we
ended up with the following situations:

 * Existing syscalls adding new features tended to use new AT_* bits,
   with some effort taken to try to re-use bits for flags that were so
   obviously syscall specific that they only make sense for a single
   syscall (such as the AT_EACCESS/AT_REMOVEDIR/AT_HANDLE_FID triplet).

   Given the constraints of bitflags, this works well in practice, but
   ideally (to avoid future confusion) we would plan ahead and define a
   set of "per-syscall bits" ahead of time so that when allocating new
   bits we don't end up with a complete mish-mash of which bits are
   supposed to be per-syscall and which aren't.

 * New syscalls dealt with this in several ways:

   - Some syscalls (like renameat2(2), move_mount(2), fsopen(2), and
     fspick(2)) created their separate own flag spaces that have no
     overlap with the AT_* flags. Most of these ended up allocating
     their bits sequentually.

     In the case of move_mount(2) and fspick(2), several flags have
     identical meanings to AT_* flags but were allocated in their own
     flag space.

     This makes sense for syscalls that will never share AT_* flags, but
     for some syscalls this leads to duplication with AT_* flags in a
     way that could cause confusion (if renameat2(2) grew a
     RENAME_EMPTY_PATH it seems likely that users could mistake it for
     AT_EMPTY_PATH since it is an *at(2) syscall).

   - Some syscalls unfortunately ended up both creating their own flag
     space while also using bits from other flag spaces. The most
     obvious example is open_tree(2), where the standard usage ends up
     using flags from *THREE* separate flag spaces:

       open_tree(AT_FDCWD, "/foo", OPEN_TREE_CLONE|O_CLOEXEC|AT_RECURSIVE);

     (Note that O_CLOEXEC is also platform-specific, so several future
     OPEN_TREE_* bits are also made unusable in one fell swoop.)

It's not entirely clear to me what the "right" choice is for new
syscalls. Just saying that all future VFS syscalls should use AT_* flags
doesn't seem practical. openat2(2) has RESOLVE_* flags (many of which
don't make much sense to burn generic AT_* flags for) and move_mount(2)
has separate AT_*-like flags for both the source and target so separate
flags are needed anyway (though it seems possible that renameat2(2)
could grow *_EMPTY_PATH flags at some point, and it's a bit of a shame
they can't be reused).

But at least for syscalls that _do_ choose to use AT_* flags, we should
explicitly state the policy that 0x2ff is currently intended for
per-syscall flags and that new flags should err on the side of
overlapping with existing flag bits (so we can extend the scope of
generic flags in the future if necessary).

And add AT_* aliases for the RENAME_* flags to further cement that
renameat2(2) is an *at(2) flag, just with its own per-syscall flags.

Suggested-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 include/uapi/linux/fcntl.h                         | 80 ++++++++++++++-------
 tools/perf/trace/beauty/include/uapi/linux/fcntl.h | 83 +++++++++++++++-------
 2 files changed, 115 insertions(+), 48 deletions(-)

diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index e55a3314bcb0..38a6d66d9e88 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -90,37 +90,69 @@
 #define DN_ATTRIB	0x00000020	/* File changed attibutes */
 #define DN_MULTISHOT	0x80000000	/* Don't remove notifier */
 
+#define AT_FDCWD		-100    /* Special value for dirfd used to
+					   indicate openat should use the
+					   current working directory. */
+
+
+/* Generic flags for the *at(2) family of syscalls. */
+
+/* Reserved for per-syscall flags	0xff. */
+#define AT_SYMLINK_NOFOLLOW		0x100   /* Do not follow symbolic
+						   links. */
+/* Reserved for per-syscall flags	0x200 */
+#define AT_SYMLINK_FOLLOW		0x400   /* Follow symbolic links. */
+#define AT_NO_AUTOMOUNT			0x800	/* Suppress terminal automount
+						   traversal. */
+#define AT_EMPTY_PATH			0x1000	/* Allow empty relative
+						   pathname to operate on dirfd
+						   directly. */
 /*
- * The constants AT_REMOVEDIR and AT_EACCESS have the same value.  AT_EACCESS is
- * meaningful only to faccessat, while AT_REMOVEDIR is meaningful only to
- * unlinkat.  The two functions do completely different things and therefore,
- * the flags can be allowed to overlap.  For example, passing AT_REMOVEDIR to
- * faccessat would be undefined behavior and thus treating it equivalent to
- * AT_EACCESS is valid undefined behavior.
+ * These flags are currently statx(2)-specific, but they could be made generic
+ * in the future and so they should not be used for other per-syscall flags.
  */
-#define AT_FDCWD		-100    /* Special value used to indicate
-                                           openat should use the current
-                                           working directory. */
-#define AT_SYMLINK_NOFOLLOW	0x100   /* Do not follow symbolic links.  */
+#define AT_STATX_SYNC_TYPE		0x6000	/* Type of synchronisation required from statx() */
+#define AT_STATX_SYNC_AS_STAT		0x0000	/* - Do whatever stat() does */
+#define AT_STATX_FORCE_SYNC		0x2000	/* - Force the attributes to be sync'd with the server */
+#define AT_STATX_DONT_SYNC		0x4000	/* - Don't sync attributes with the server */
+
+#define AT_RECURSIVE			0x8000	/* Apply to the entire subtree */
+
+/*
+ * Per-syscall flags for the *at(2) family of syscalls.
+ *
+ * These are flags that are so syscall-specific that a user passing these flags
+ * to the wrong syscall is so "clearly wrong" that we can safely call such
+ * usage "undefined behaviour".
+ *
+ * For example, the constants AT_REMOVEDIR and AT_EACCESS have the same value.
+ * AT_EACCESS is meaningful only to faccessat, while AT_REMOVEDIR is meaningful
+ * only to unlinkat. The two functions do completely different things and
+ * therefore, the flags can be allowed to overlap. For example, passing
+ * AT_REMOVEDIR to faccessat would be undefined behavior and thus treating it
+ * equivalent to AT_EACCESS is valid undefined behavior.
+ *
+ * Note for implementers: When picking a new per-syscall AT_* flag, try to
+ * reuse already existing flags first. This leaves us with as many unused bits
+ * as possible, so we can use them for generic bits in the future if necessary.
+ */
+
+/* Flags for renameat2(2) (must match legacy RENAME_* flags). */
+#define AT_RENAME_NOREPLACE	0x0001
+#define AT_RENAME_EXCHANGE	0x0002
+#define AT_RENAME_WHITEOUT	0x0004
+
+/* Flag for faccessat(2). */
 #define AT_EACCESS		0x200	/* Test access permitted for
                                            effective IDs, not real IDs.  */
+/* Flag for unlinkat(2). */
 #define AT_REMOVEDIR		0x200   /* Remove directory instead of
                                            unlinking file.  */
-#define AT_SYMLINK_FOLLOW	0x400   /* Follow symbolic links.  */
-#define AT_NO_AUTOMOUNT		0x800	/* Suppress terminal automount traversal */
-#define AT_EMPTY_PATH		0x1000	/* Allow empty relative pathname */
-
-#define AT_STATX_SYNC_TYPE	0x6000	/* Type of synchronisation required from statx() */
-#define AT_STATX_SYNC_AS_STAT	0x0000	/* - Do whatever stat() does */
-#define AT_STATX_FORCE_SYNC	0x2000	/* - Force the attributes to be sync'd with the server */
-#define AT_STATX_DONT_SYNC	0x4000	/* - Don't sync attributes with the server */
-
-#define AT_RECURSIVE		0x8000	/* Apply to the entire subtree */
+/* Flags for name_to_handle_at(2). */
+#define AT_HANDLE_FID		0x200	/* File handle is needed to compare
+					   object identity and may not be
+					   usable with open_by_handle_at(2). */
 
-/* Flags for name_to_handle_at(2). We reuse AT_ flag space to save bits... */
-#define AT_HANDLE_FID		AT_REMOVEDIR	/* file handle is needed to
-					compare object identity and may not
-					be usable to open_by_handle_at(2) */
 #if defined(__KERNEL__)
 #define AT_GETATTR_NOSEC	0x80000000
 #endif
diff --git a/tools/perf/trace/beauty/include/uapi/linux/fcntl.h b/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
index c0bcc185fa48..38a6d66d9e88 100644
--- a/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
+++ b/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
@@ -16,6 +16,9 @@
 
 #define F_DUPFD_QUERY	(F_LINUX_SPECIFIC_BASE + 3)
 
+/* Was the file just created? */
+#define F_CREATED_QUERY	(F_LINUX_SPECIFIC_BASE + 4)
+
 /*
  * Cancel a blocking posix lock; internal use only until we expose an
  * asynchronous lock api to userspace:
@@ -87,37 +90,69 @@
 #define DN_ATTRIB	0x00000020	/* File changed attibutes */
 #define DN_MULTISHOT	0x80000000	/* Don't remove notifier */
 
+#define AT_FDCWD		-100    /* Special value for dirfd used to
+					   indicate openat should use the
+					   current working directory. */
+
+
+/* Generic flags for the *at(2) family of syscalls. */
+
+/* Reserved for per-syscall flags	0xff. */
+#define AT_SYMLINK_NOFOLLOW		0x100   /* Do not follow symbolic
+						   links. */
+/* Reserved for per-syscall flags	0x200 */
+#define AT_SYMLINK_FOLLOW		0x400   /* Follow symbolic links. */
+#define AT_NO_AUTOMOUNT			0x800	/* Suppress terminal automount
+						   traversal. */
+#define AT_EMPTY_PATH			0x1000	/* Allow empty relative
+						   pathname to operate on dirfd
+						   directly. */
+/*
+ * These flags are currently statx(2)-specific, but they could be made generic
+ * in the future and so they should not be used for other per-syscall flags.
+ */
+#define AT_STATX_SYNC_TYPE		0x6000	/* Type of synchronisation required from statx() */
+#define AT_STATX_SYNC_AS_STAT		0x0000	/* - Do whatever stat() does */
+#define AT_STATX_FORCE_SYNC		0x2000	/* - Force the attributes to be sync'd with the server */
+#define AT_STATX_DONT_SYNC		0x4000	/* - Don't sync attributes with the server */
+
+#define AT_RECURSIVE			0x8000	/* Apply to the entire subtree */
+
 /*
- * The constants AT_REMOVEDIR and AT_EACCESS have the same value.  AT_EACCESS is
- * meaningful only to faccessat, while AT_REMOVEDIR is meaningful only to
- * unlinkat.  The two functions do completely different things and therefore,
- * the flags can be allowed to overlap.  For example, passing AT_REMOVEDIR to
- * faccessat would be undefined behavior and thus treating it equivalent to
- * AT_EACCESS is valid undefined behavior.
+ * Per-syscall flags for the *at(2) family of syscalls.
+ *
+ * These are flags that are so syscall-specific that a user passing these flags
+ * to the wrong syscall is so "clearly wrong" that we can safely call such
+ * usage "undefined behaviour".
+ *
+ * For example, the constants AT_REMOVEDIR and AT_EACCESS have the same value.
+ * AT_EACCESS is meaningful only to faccessat, while AT_REMOVEDIR is meaningful
+ * only to unlinkat. The two functions do completely different things and
+ * therefore, the flags can be allowed to overlap. For example, passing
+ * AT_REMOVEDIR to faccessat would be undefined behavior and thus treating it
+ * equivalent to AT_EACCESS is valid undefined behavior.
+ *
+ * Note for implementers: When picking a new per-syscall AT_* flag, try to
+ * reuse already existing flags first. This leaves us with as many unused bits
+ * as possible, so we can use them for generic bits in the future if necessary.
  */
-#define AT_FDCWD		-100    /* Special value used to indicate
-                                           openat should use the current
-                                           working directory. */
-#define AT_SYMLINK_NOFOLLOW	0x100   /* Do not follow symbolic links.  */
+
+/* Flags for renameat2(2) (must match legacy RENAME_* flags). */
+#define AT_RENAME_NOREPLACE	0x0001
+#define AT_RENAME_EXCHANGE	0x0002
+#define AT_RENAME_WHITEOUT	0x0004
+
+/* Flag for faccessat(2). */
 #define AT_EACCESS		0x200	/* Test access permitted for
                                            effective IDs, not real IDs.  */
+/* Flag for unlinkat(2). */
 #define AT_REMOVEDIR		0x200   /* Remove directory instead of
                                            unlinking file.  */
-#define AT_SYMLINK_FOLLOW	0x400   /* Follow symbolic links.  */
-#define AT_NO_AUTOMOUNT		0x800	/* Suppress terminal automount traversal */
-#define AT_EMPTY_PATH		0x1000	/* Allow empty relative pathname */
-
-#define AT_STATX_SYNC_TYPE	0x6000	/* Type of synchronisation required from statx() */
-#define AT_STATX_SYNC_AS_STAT	0x0000	/* - Do whatever stat() does */
-#define AT_STATX_FORCE_SYNC	0x2000	/* - Force the attributes to be sync'd with the server */
-#define AT_STATX_DONT_SYNC	0x4000	/* - Don't sync attributes with the server */
-
-#define AT_RECURSIVE		0x8000	/* Apply to the entire subtree */
+/* Flags for name_to_handle_at(2). */
+#define AT_HANDLE_FID		0x200	/* File handle is needed to compare
+					   object identity and may not be
+					   usable with open_by_handle_at(2). */
 
-/* Flags for name_to_handle_at(2). We reuse AT_ flag space to save bits... */
-#define AT_HANDLE_FID		AT_REMOVEDIR	/* file handle is needed to
-					compare object identity and may not
-					be usable to open_by_handle_at(2) */
 #if defined(__KERNEL__)
 #define AT_GETATTR_NOSEC	0x80000000
 #endif

-- 
2.46.0

[PATCH RESEND v3 2/2] fhandle: expose u64 mount id to name_to_handle_at(2)

[Aleksa Sarai]

Now that we provide a unique 64-bit mount ID interface in statx(2), we
can now provide a race-free way for name_to_handle_at(2) to provide a
file handle and corresponding mount without needing to worry about
racing with /proc/mountinfo parsing or having to open a file just to do
statx(2).

While this is not necessary if you are using AT_EMPTY_PATH and don't
care about an extra statx(2) call, users that pass full paths into
name_to_handle_at(2) need to know which mount the file handle comes from
(to make sure they don't try to open_by_handle_at a file handle from a
different filesystem) and switching to AT_EMPTY_PATH would require
allocating a file for every name_to_handle_at(2) call, turning

  err = name_to_handle_at(-EBADF, "/foo/bar/baz", &handle, &mntid,
                          AT_HANDLE_MNT_ID_UNIQUE);

into

  int fd = openat(-EBADF, "/foo/bar/baz", O_PATH | O_CLOEXEC);
  err1 = name_to_handle_at(fd, "", &handle, &unused_mntid, AT_EMPTY_PATH);
  err2 = statx(fd, "", AT_EMPTY_PATH, STATX_MNT_ID_UNIQUE, &statxbuf);
  mntid = statxbuf.stx_mnt_id;
  close(fd);

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 fs/fhandle.c                                       | 29 ++++++++++++++++------
 include/linux/syscalls.h                           |  2 +-
 include/uapi/linux/fcntl.h                         |  1 +
 tools/perf/trace/beauty/include/uapi/linux/fcntl.h |  1 +
 4 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/fs/fhandle.c b/fs/fhandle.c
index 6e8cea16790e..8cb665629f4a 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -16,7 +16,8 @@
 
 static long do_sys_name_to_handle(const struct path *path,
 				  struct file_handle __user *ufh,
-				  int __user *mnt_id, int fh_flags)
+				  void __user *mnt_id, bool unique_mntid,
+				  int fh_flags)
 {
 	long retval;
 	struct file_handle f_handle;
@@ -69,9 +70,19 @@ static long do_sys_name_to_handle(const struct path *path,
 	} else
 		retval = 0;
 	/* copy the mount id */
-	if (put_user(real_mount(path->mnt)->mnt_id, mnt_id) ||
-	    copy_to_user(ufh, handle,
-			 struct_size(handle, f_handle, handle_bytes)))
+	if (unique_mntid) {
+		if (put_user(real_mount(path->mnt)->mnt_id_unique,
+			     (u64 __user *) mnt_id))
+			retval = -EFAULT;
+	} else {
+		if (put_user(real_mount(path->mnt)->mnt_id,
+			     (int __user *) mnt_id))
+			retval = -EFAULT;
+	}
+	/* copy the handle */
+	if (retval != -EFAULT &&
+		copy_to_user(ufh, handle,
+			     struct_size(handle, f_handle, handle_bytes)))
 		retval = -EFAULT;
 	kfree(handle);
 	return retval;
@@ -83,6 +94,7 @@ static long do_sys_name_to_handle(const struct path *path,
  * @name: name that should be converted to handle.
  * @handle: resulting file handle
  * @mnt_id: mount id of the file system containing the file
+ *          (u64 if AT_HANDLE_MNT_ID_UNIQUE, otherwise int)
  * @flag: flag value to indicate whether to follow symlink or not
  *        and whether a decodable file handle is required.
  *
@@ -92,7 +104,7 @@ static long do_sys_name_to_handle(const struct path *path,
  * value required.
  */
 SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
-		struct file_handle __user *, handle, int __user *, mnt_id,
+		struct file_handle __user *, handle, void __user *, mnt_id,
 		int, flag)
 {
 	struct path path;
@@ -100,7 +112,8 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
 	int fh_flags;
 	int err;
 
-	if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID))
+	if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID |
+		     AT_HANDLE_MNT_ID_UNIQUE))
 		return -EINVAL;
 
 	lookup_flags = (flag & AT_SYMLINK_FOLLOW) ? LOOKUP_FOLLOW : 0;
@@ -109,7 +122,9 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
 		lookup_flags |= LOOKUP_EMPTY;
 	err = user_path_at(dfd, name, lookup_flags, &path);
 	if (!err) {
-		err = do_sys_name_to_handle(&path, handle, mnt_id, fh_flags);
+		err = do_sys_name_to_handle(&path, handle, mnt_id,
+					    flag & AT_HANDLE_MNT_ID_UNIQUE,
+					    fh_flags);
 		path_put(&path);
 	}
 	return err;
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 4bcf6754738d..5758104921e6 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -870,7 +870,7 @@ asmlinkage long sys_fanotify_mark(int fanotify_fd, unsigned int flags,
 #endif
 asmlinkage long sys_name_to_handle_at(int dfd, const char __user *name,
 				      struct file_handle __user *handle,
-				      int __user *mnt_id, int flag);
+				      void __user *mnt_id, int flag);
 asmlinkage long sys_open_by_handle_at(int mountdirfd,
 				      struct file_handle __user *handle,
 				      int flags);
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index 38a6d66d9e88..87e2dec79fea 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -152,6 +152,7 @@
 #define AT_HANDLE_FID		0x200	/* File handle is needed to compare
 					   object identity and may not be
 					   usable with open_by_handle_at(2). */
+#define AT_HANDLE_MNT_ID_UNIQUE	0x001	/* Return the u64 unique mount ID. */
 
 #if defined(__KERNEL__)
 #define AT_GETATTR_NOSEC	0x80000000
diff --git a/tools/perf/trace/beauty/include/uapi/linux/fcntl.h b/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
index 38a6d66d9e88..87e2dec79fea 100644
--- a/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
+++ b/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
@@ -152,6 +152,7 @@
 #define AT_HANDLE_FID		0x200	/* File handle is needed to compare
 					   object identity and may not be
 					   usable with open_by_handle_at(2). */
+#define AT_HANDLE_MNT_ID_UNIQUE	0x001	/* Return the u64 unique mount ID. */
 
 #if defined(__KERNEL__)
 #define AT_GETATTR_NOSEC	0x80000000

-- 
2.46.0

[PATCH xfstests v1 1/2] statx: update headers to include newer statx fields

[Aleksa Sarai]

These come from Linux v6.11-rc5.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 src/open_by_handle.c |  4 +++-
 src/statx.h          | 22 ++++++++++++++++++++--
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/src/open_by_handle.c b/src/open_by_handle.c
index 0f74ed08b1f0..d9c802ca9bd1 100644
--- a/src/open_by_handle.c
+++ b/src/open_by_handle.c
@@ -82,12 +82,14 @@ Examples:
 #include <string.h>
 #include <fcntl.h>
 #include <unistd.h>
-#include <sys/stat.h>
 #include <sys/types.h>
 #include <errno.h>
 #include <linux/limits.h>
 #include <libgen.h>
 
+#include <sys/stat.h>
+#include "statx.h"
+
 #define MAXFILES 1024
 
 struct handle {
diff --git a/src/statx.h b/src/statx.h
index 3f239d791dfe..935cb2ed415e 100644
--- a/src/statx.h
+++ b/src/statx.h
@@ -102,7 +102,7 @@ struct statx {
 	__u64	stx_ino;	/* Inode number */
 	__u64	stx_size;	/* File size */
 	__u64	stx_blocks;	/* Number of 512-byte blocks allocated */
-	__u64	__spare1[1];
+	__u64	stx_attributes_mask; /* Mask to show what's supported in stx_attributes */
 	/* 0x40 */
 	struct statx_timestamp	stx_atime;	/* Last access time */
 	struct statx_timestamp	stx_btime;	/* File creation time */
@@ -114,7 +114,18 @@ struct statx {
 	__u32	stx_dev_major;	/* ID of device containing file [uncond] */
 	__u32	stx_dev_minor;
 	/* 0x90 */
-	__u64	__spare2[14];	/* Spare space for future expansion */
+	__u64	stx_mnt_id;
+	__u32	stx_dio_mem_align;	/* Memory buffer alignment for direct I/O */
+	__u32	stx_dio_offset_align;	/* File offset alignment for direct I/O */
+	/* 0xa0 */
+	__u64	stx_subvol;	/* Subvolume identifier */
+	__u32	stx_atomic_write_unit_min;	/* Min atomic write unit in bytes */
+	__u32	stx_atomic_write_unit_max;	/* Max atomic write unit in bytes */
+	/* 0xb0 */
+	__u32   stx_atomic_write_segments_max;	/* Max atomic write segment count */
+	__u32   __spare1[1];
+	/* 0xb8 */
+	__u64	__spare3[9];	/* Spare space for future expansion */
 	/* 0x100 */
 };
 
@@ -139,6 +150,13 @@ struct statx {
 #define STATX_BLOCKS		0x00000400U	/* Want/got stx_blocks */
 #define STATX_BASIC_STATS	0x000007ffU	/* The stuff in the normal stat struct */
 #define STATX_BTIME		0x00000800U	/* Want/got stx_btime */
+#define STATX_MNT_ID		0x00001000U	/* Got stx_mnt_id */
+#define STATX_DIOALIGN		0x00002000U	/* Want/got direct I/O alignment info */
+#define STATX_MNT_ID_UNIQUE	0x00004000U	/* Want/got extended stx_mount_id */
+#define STATX_SUBVOL		0x00008000U	/* Want/got stx_subvol */
+#define STATX_WRITE_ATOMIC	0x00010000U	/* Want/got atomic_write_* fields */
+
+#define STATX__RESERVED		0x80000000U	/* Reserved for future struct statx expansion */
 #define STATX_ALL		0x00000fffU	/* All currently supported flags */
 
 /*
-- 
2.46.0

[PATCH xfstests v1 2/2] open_by_handle: add tests for u64 mount ID

[Aleksa Sarai]

Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
make sure they match properly as part of the regular open_by_handle
tests.

Link: https://lore.kernel.org/all/20240801-exportfs-u64-mount-id-v3-0-be5d6283144a@cyphar.com/
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 src/open_by_handle.c | 123 ++++++++++++++++++++++++++++++++-----------
 tests/generic/426    |   1 +
 2 files changed, 93 insertions(+), 31 deletions(-)

diff --git a/src/open_by_handle.c b/src/open_by_handle.c
index d9c802ca9bd1..cbd68aeadac1 100644
--- a/src/open_by_handle.c
+++ b/src/open_by_handle.c
@@ -86,10 +86,15 @@ Examples:
 #include <errno.h>
 #include <linux/limits.h>
 #include <libgen.h>
+#include <stdint.h>
 
 #include <sys/stat.h>
 #include "statx.h"
 
+#ifndef AT_HANDLE_MNT_ID_UNIQUE
+#	define AT_HANDLE_MNT_ID_UNIQUE 0x001
+#endif
+
 #define MAXFILES 1024
 
 struct handle {
@@ -99,7 +104,7 @@ struct handle {
 
 void usage(void)
 {
-	fprintf(stderr, "usage: open_by_handle [-cludmrwapknhs] [<-i|-o> <handles_file>] <test_dir> [num_files]\n");
+	fprintf(stderr, "usage: open_by_handle [-cludMmrwapknhs] [<-i|-o> <handles_file>] <test_dir> [num_files]\n");
 	fprintf(stderr, "\n");
 	fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, try to get file handles and exit\n");
 	fprintf(stderr, "open_by_handle    <test_dir> [N] - get file handles of test files, drop caches and try to open by handle\n");
@@ -111,6 +116,7 @@ void usage(void)
 	fprintf(stderr, "open_by_handle -l <test_dir> [N] - create hardlinks to test files, drop caches and try to open by handle\n");
 	fprintf(stderr, "open_by_handle -u <test_dir> [N] - unlink (hardlinked) test files, drop caches and try to open by handle\n");
 	fprintf(stderr, "open_by_handle -d <test_dir> [N] - unlink test files and hardlinks, drop caches and try to open by handle\n");
+	fprintf(stderr, "open_by_handle -M <test_dir> [N] - confirm that the mount id returned by name_to_handle_at matches the mount id in statx\n");
 	fprintf(stderr, "open_by_handle -m <test_dir> [N] - rename test files, drop caches and try to open by handle\n");
 	fprintf(stderr, "open_by_handle -p <test_dir>     - create/delete and try to open by handle also test_dir itself\n");
 	fprintf(stderr, "open_by_handle -i <handles_file> <test_dir> [N] - read test files handles from file and try to open by handle\n");
@@ -120,6 +126,81 @@ void usage(void)
 	exit(EXIT_FAILURE);
 }
 
+int do_name_to_handle_at(const char *fname, struct file_handle *fh, int bufsz,
+			 int checkmountid)
+{
+	int ret;
+	int mntid_short;
+
+	uint64_t mntid_unique;
+	uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
+	struct handle dummy_fh;
+
+	if (checkmountid) {
+		struct statx statxbuf;
+
+		/* Get both the short and unique mount id. */
+		if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) {
+			fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
+			return EXIT_FAILURE;
+		}
+		if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
+			fprintf(stderr, "%s: no STATX_MNT_ID in stx_mask\n", fname);
+			return EXIT_FAILURE;
+		}
+		statx_mntid_short = statxbuf.stx_mnt_id;
+
+		if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) {
+			fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname);
+			return EXIT_FAILURE;
+		}
+		if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)) {
+			fprintf(stderr, "%s: no STATX_MNT_ID_UNIQUE in stx_mask\n", fname);
+			return EXIT_FAILURE;
+		}
+		statx_mntid_unique = statxbuf.stx_mnt_id;
+	}
+
+	fh->handle_bytes = bufsz;
+	ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
+	if (bufsz < fh->handle_bytes) {
+		/* Query the filesystem required bufsz and the file handle */
+		if (ret != -1 || errno != EOVERFLOW) {
+			fprintf(stderr, "%s: unexpected result from name_to_handle_at: %d (%m)\n", fname, ret);
+			return EXIT_FAILURE;
+		}
+		ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
+	}
+	if (ret < 0) {
+		fprintf(stderr, "%s: name_to_handle: %m\n", fname);
+		return EXIT_FAILURE;
+	}
+
+	if (checkmountid) {
+		if (mntid_short != (int) statx_mntid_short) {
+			fprintf(stderr, "%s: name_to_handle_at returned a different mount ID to STATX_MNT_ID: %u != %lu\n", fname, mntid_short, statx_mntid_short);
+			return EXIT_FAILURE;
+		}
+
+		/*
+		 * Get the unique mount ID. We don't need to get another copy of the
+		 * handle so store it in a dummy struct.
+		 */
+		dummy_fh.fh.handle_bytes = fh->handle_bytes;
+		if (name_to_handle_at(AT_FDCWD, fname, &dummy_fh.fh, (int *) &mntid_unique, AT_HANDLE_MNT_ID_UNIQUE) < 0) {
+			fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE): %m\n", fname);
+			return EXIT_FAILURE;
+		}
+
+		if (mntid_unique != statx_mntid_unique) {
+			fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) returned a different mount ID to STATX_MNT_ID_UNIQUE: %lu != %lu\n", fname, mntid_unique, statx_mntid_unique);
+			return EXIT_FAILURE;
+		}
+	}
+
+	return 0;
+}
+
 int main(int argc, char **argv)
 {
 	int	i, c;
@@ -132,19 +213,20 @@ int main(int argc, char **argv)
 	char	fname2[PATH_MAX];
 	char	*test_dir;
 	char	*mount_dir;
-	int	mount_fd, mount_id;
+	int	mount_fd;
 	char	*infile = NULL, *outfile = NULL;
 	int	in_fd = 0, out_fd = 0;
 	int	numfiles = 1;
 	int	create = 0, delete = 0, nlink = 1, move = 0;
 	int	rd = 0, wr = 0, wrafter = 0, parent = 0;
 	int	keepopen = 0, drop_caches = 1, sleep_loop = 0;
+	int	checkmountid = 0;
 	int	bufsz = MAX_HANDLE_SZ;
 
 	if (argc < 2)
 		usage();
 
-	while ((c = getopt(argc, argv, "cludmrwapknhi:o:sz")) != -1) {
+	while ((c = getopt(argc, argv, "cludMmrwapknhi:o:sz")) != -1) {
 		switch (c) {
 		case 'c':
 			create = 1;
@@ -172,6 +254,9 @@ int main(int argc, char **argv)
 			delete = 1;
 			nlink = 0;
 			break;
+		case 'M':
+			checkmountid = 1;
+			break;
 		case 'm':
 			move = 1;
 			break;
@@ -307,21 +392,9 @@ int main(int argc, char **argv)
 				return EXIT_FAILURE;
 			}
 		} else {
-			handle[i].fh.handle_bytes = bufsz;
-			ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
-			if (bufsz < handle[i].fh.handle_bytes) {
-				/* Query the filesystem required bufsz and the file handle */
-				if (ret != -1 || errno != EOVERFLOW) {
-					fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", fname);
-					return EXIT_FAILURE;
-				}
-				ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
-			}
-			if (ret < 0) {
-				strcat(fname, ": name_to_handle");
-				perror(fname);
+			ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz, checkmountid);
+			if (ret < 0)
 				return EXIT_FAILURE;
-			}
 		}
 		if (keepopen) {
 			/* Open without close to keep unlinked files around */
@@ -349,21 +422,9 @@ int main(int argc, char **argv)
 				return EXIT_FAILURE;
 			}
 		} else {
-			dir_handle.fh.handle_bytes = bufsz;
-			ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
-			if (bufsz < dir_handle.fh.handle_bytes) {
-				/* Query the filesystem required bufsz and the file handle */
-				if (ret != -1 || errno != EOVERFLOW) {
-					fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", dname);
-					return EXIT_FAILURE;
-				}
-				ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
-			}
-			if (ret < 0) {
-				strcat(dname, ": name_to_handle");
-				perror(dname);
+			ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz, checkmountid);
+			if (ret < 0)
 				return EXIT_FAILURE;
-			}
 		}
 		if (out_fd) {
 			ret = write(out_fd, (char *)&dir_handle, sizeof(*handle));
diff --git a/tests/generic/426 b/tests/generic/426
index 25909f220e1e..df481c58562c 100755
--- a/tests/generic/426
+++ b/tests/generic/426
@@ -51,6 +51,7 @@ test_file_handles $testdir -d
 # Check non-stale handles to linked files
 create_test_files $testdir
 test_file_handles $testdir
+test_file_handles $testdir -M
 
 # Check non-stale handles to files that were hardlinked and original deleted
 create_test_files $testdir
-- 
2.46.0

Re: [PATCH xfstests v1 2/2] open_by_handle: add tests for u64 mount ID

[Amir Goldstein]

On Wed, Aug 28, 2024 at 12:37 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
> make sure they match properly as part of the regular open_by_handle
> tests.
>
> Link: https://lore.kernel.org/all/20240801-exportfs-u64-mount-id-v3-0-be5d6283144a@cyphar.com/
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
>  src/open_by_handle.c | 123 ++++++++++++++++++++++++++++++++-----------
>  tests/generic/426    |   1 +
>  2 files changed, 93 insertions(+), 31 deletions(-)
>
> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> index d9c802ca9bd1..cbd68aeadac1 100644
> --- a/src/open_by_handle.c
> +++ b/src/open_by_handle.c
> @@ -86,10 +86,15 @@ Examples:
>  #include <errno.h>
>  #include <linux/limits.h>
>  #include <libgen.h>
> +#include <stdint.h>
>
>  #include <sys/stat.h>
>  #include "statx.h"
>
> +#ifndef AT_HANDLE_MNT_ID_UNIQUE
> +#      define AT_HANDLE_MNT_ID_UNIQUE 0x001
> +#endif
> +
>  #define MAXFILES 1024
>
>  struct handle {
> @@ -99,7 +104,7 @@ struct handle {
>
>  void usage(void)
>  {
> -       fprintf(stderr, "usage: open_by_handle [-cludmrwapknhs] [<-i|-o> <handles_file>] <test_dir> [num_files]\n");
> +       fprintf(stderr, "usage: open_by_handle [-cludMmrwapknhs] [<-i|-o> <handles_file>] <test_dir> [num_files]\n");
>         fprintf(stderr, "\n");
>         fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, try to get file handles and exit\n");
>         fprintf(stderr, "open_by_handle    <test_dir> [N] - get file handles of test files, drop caches and try to open by handle\n");
> @@ -111,6 +116,7 @@ void usage(void)
>         fprintf(stderr, "open_by_handle -l <test_dir> [N] - create hardlinks to test files, drop caches and try to open by handle\n");
>         fprintf(stderr, "open_by_handle -u <test_dir> [N] - unlink (hardlinked) test files, drop caches and try to open by handle\n");
>         fprintf(stderr, "open_by_handle -d <test_dir> [N] - unlink test files and hardlinks, drop caches and try to open by handle\n");
> +       fprintf(stderr, "open_by_handle -M <test_dir> [N] - confirm that the mount id returned by name_to_handle_at matches the mount id in statx\n");
>         fprintf(stderr, "open_by_handle -m <test_dir> [N] - rename test files, drop caches and try to open by handle\n");
>         fprintf(stderr, "open_by_handle -p <test_dir>     - create/delete and try to open by handle also test_dir itself\n");
>         fprintf(stderr, "open_by_handle -i <handles_file> <test_dir> [N] - read test files handles from file and try to open by handle\n");
> @@ -120,6 +126,81 @@ void usage(void)
>         exit(EXIT_FAILURE);
>  }
>
> +int do_name_to_handle_at(const char *fname, struct file_handle *fh, int bufsz,
> +                        int checkmountid)
> +{
> +       int ret;
> +       int mntid_short;
> +
> +       uint64_t mntid_unique;
> +       uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
> +       struct handle dummy_fh;
> +
> +       if (checkmountid) {
> +               struct statx statxbuf;
> +
> +               /* Get both the short and unique mount id. */
> +               if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) {
> +                       fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
> +                       return EXIT_FAILURE;
> +               }
> +               if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
> +                       fprintf(stderr, "%s: no STATX_MNT_ID in stx_mask\n", fname);
> +                       return EXIT_FAILURE;
> +               }
> +               statx_mntid_short = statxbuf.stx_mnt_id;
> +
> +               if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) {
> +                       fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname);
> +                       return EXIT_FAILURE;

This failure will break the test on LTS kernels  - we don't want that.
Instead I think you should:
- drop the -M option
- get statx_mntid_unique here IF kernel supports STATX_MNT_ID_UNIQUE
and then...

> +               }
> +               if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)) {
> +                       fprintf(stderr, "%s: no STATX_MNT_ID_UNIQUE in stx_mask\n", fname);
> +                       return EXIT_FAILURE;
> +               }
> +               statx_mntid_unique = statxbuf.stx_mnt_id;
> +       }
> +
> +       fh->handle_bytes = bufsz;
> +       ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
> +       if (bufsz < fh->handle_bytes) {
> +               /* Query the filesystem required bufsz and the file handle */
> +               if (ret != -1 || errno != EOVERFLOW) {
> +                       fprintf(stderr, "%s: unexpected result from name_to_handle_at: %d (%m)\n", fname, ret);
> +                       return EXIT_FAILURE;
> +               }
> +               ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
> +       }
> +       if (ret < 0) {
> +               fprintf(stderr, "%s: name_to_handle: %m\n", fname);
> +               return EXIT_FAILURE;
> +       }
> +
> +       if (checkmountid) {
> +               if (mntid_short != (int) statx_mntid_short) {
> +                       fprintf(stderr, "%s: name_to_handle_at returned a different mount ID to STATX_MNT_ID: %u != %lu\n", fname, mntid_short, statx_mntid_short);
> +                       return EXIT_FAILURE;
> +               }
> +
> +               /*
> +                * Get the unique mount ID. We don't need to get another copy of the
> +                * handle so store it in a dummy struct.
> +                */
> +               dummy_fh.fh.handle_bytes = fh->handle_bytes;
> +               if (name_to_handle_at(AT_FDCWD, fname, &dummy_fh.fh, (int *) &mntid_unique, AT_HANDLE_MNT_ID_UNIQUE) < 0) {
> +                       fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE): %m\n", fname);
> +                       return EXIT_FAILURE;
> +               }
> +
> +               if (mntid_unique != statx_mntid_unique) {
> +                       fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) returned a different mount ID to STATX_MNT_ID_UNIQUE: %lu != %lu\n", fname, mntid_unique, statx_mntid_unique);
> +                       return EXIT_FAILURE;
> +               }

- check statx_mntid_unique here IFF statx_mntid_unique is set
- always check statx_mntid_short (what could be a reason to not check it?)

> +       }
> +
> +       return 0;
> +}
> +
>  int main(int argc, char **argv)
>  {
>         int     i, c;
> @@ -132,19 +213,20 @@ int main(int argc, char **argv)
>         char    fname2[PATH_MAX];
>         char    *test_dir;
>         char    *mount_dir;
> -       int     mount_fd, mount_id;
> +       int     mount_fd;
>         char    *infile = NULL, *outfile = NULL;
>         int     in_fd = 0, out_fd = 0;
>         int     numfiles = 1;
>         int     create = 0, delete = 0, nlink = 1, move = 0;
>         int     rd = 0, wr = 0, wrafter = 0, parent = 0;
>         int     keepopen = 0, drop_caches = 1, sleep_loop = 0;
> +       int     checkmountid = 0;
>         int     bufsz = MAX_HANDLE_SZ;
>
>         if (argc < 2)
>                 usage();
>
> -       while ((c = getopt(argc, argv, "cludmrwapknhi:o:sz")) != -1) {
> +       while ((c = getopt(argc, argv, "cludMmrwapknhi:o:sz")) != -1) {
>                 switch (c) {
>                 case 'c':
>                         create = 1;
> @@ -172,6 +254,9 @@ int main(int argc, char **argv)
>                         delete = 1;
>                         nlink = 0;
>                         break;
> +               case 'M':
> +                       checkmountid = 1;
> +                       break;
>                 case 'm':
>                         move = 1;
>                         break;
> @@ -307,21 +392,9 @@ int main(int argc, char **argv)
>                                 return EXIT_FAILURE;
>                         }
>                 } else {
> -                       handle[i].fh.handle_bytes = bufsz;
> -                       ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
> -                       if (bufsz < handle[i].fh.handle_bytes) {
> -                               /* Query the filesystem required bufsz and the file handle */
> -                               if (ret != -1 || errno != EOVERFLOW) {
> -                                       fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", fname);
> -                                       return EXIT_FAILURE;
> -                               }
> -                               ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
> -                       }
> -                       if (ret < 0) {
> -                               strcat(fname, ": name_to_handle");
> -                               perror(fname);
> +                       ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz, checkmountid);
> +                       if (ret < 0)
>                                 return EXIT_FAILURE;
> -                       }
>                 }
>                 if (keepopen) {
>                         /* Open without close to keep unlinked files around */
> @@ -349,21 +422,9 @@ int main(int argc, char **argv)
>                                 return EXIT_FAILURE;
>                         }
>                 } else {
> -                       dir_handle.fh.handle_bytes = bufsz;
> -                       ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
> -                       if (bufsz < dir_handle.fh.handle_bytes) {
> -                               /* Query the filesystem required bufsz and the file handle */
> -                               if (ret != -1 || errno != EOVERFLOW) {
> -                                       fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", dname);
> -                                       return EXIT_FAILURE;
> -                               }
> -                               ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
> -                       }
> -                       if (ret < 0) {
> -                               strcat(dname, ": name_to_handle");
> -                               perror(dname);
> +                       ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz, checkmountid);
> +                       if (ret < 0)
>                                 return EXIT_FAILURE;
> -                       }
>                 }
>                 if (out_fd) {
>                         ret = write(out_fd, (char *)&dir_handle, sizeof(*handle));
> diff --git a/tests/generic/426 b/tests/generic/426
> index 25909f220e1e..df481c58562c 100755
> --- a/tests/generic/426
> +++ b/tests/generic/426
> @@ -51,6 +51,7 @@ test_file_handles $testdir -d
>  # Check non-stale handles to linked files
>  create_test_files $testdir
>  test_file_handles $testdir
> +test_file_handles $testdir -M

I see no reason to add option -M and add a second invocation.

Something I am missing?

Thanks,
Amir.

Re: [PATCH xfstests v1 2/2] open_by_handle: add tests for u64 mount ID

[Aleksa Sarai]

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

On 2024-08-30, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, Aug 28, 2024 at 12:37 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> >
> > Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
> > make sure they match properly as part of the regular open_by_handle
> > tests.
> >
> > Link: https://lore.kernel.org/all/20240801-exportfs-u64-mount-id-v3-0-be5d6283144a@cyphar.com/
> > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > ---
> >  src/open_by_handle.c | 123 ++++++++++++++++++++++++++++++++-----------
> >  tests/generic/426    |   1 +
> >  2 files changed, 93 insertions(+), 31 deletions(-)
> >
> > diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> > index d9c802ca9bd1..cbd68aeadac1 100644
> > --- a/src/open_by_handle.c
> > +++ b/src/open_by_handle.c
> > @@ -86,10 +86,15 @@ Examples:
> >  #include <errno.h>
> >  #include <linux/limits.h>
> >  #include <libgen.h>
> > +#include <stdint.h>
> >
> >  #include <sys/stat.h>
> >  #include "statx.h"
> >
> > +#ifndef AT_HANDLE_MNT_ID_UNIQUE
> > +#      define AT_HANDLE_MNT_ID_UNIQUE 0x001
> > +#endif
> > +
> >  #define MAXFILES 1024
> >
> >  struct handle {
> > @@ -99,7 +104,7 @@ struct handle {
> >
> >  void usage(void)
> >  {
> > -       fprintf(stderr, "usage: open_by_handle [-cludmrwapknhs] [<-i|-o> <handles_file>] <test_dir> [num_files]\n");
> > +       fprintf(stderr, "usage: open_by_handle [-cludMmrwapknhs] [<-i|-o> <handles_file>] <test_dir> [num_files]\n");
> >         fprintf(stderr, "\n");
> >         fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, try to get file handles and exit\n");
> >         fprintf(stderr, "open_by_handle    <test_dir> [N] - get file handles of test files, drop caches and try to open by handle\n");
> > @@ -111,6 +116,7 @@ void usage(void)
> >         fprintf(stderr, "open_by_handle -l <test_dir> [N] - create hardlinks to test files, drop caches and try to open by handle\n");
> >         fprintf(stderr, "open_by_handle -u <test_dir> [N] - unlink (hardlinked) test files, drop caches and try to open by handle\n");
> >         fprintf(stderr, "open_by_handle -d <test_dir> [N] - unlink test files and hardlinks, drop caches and try to open by handle\n");
> > +       fprintf(stderr, "open_by_handle -M <test_dir> [N] - confirm that the mount id returned by name_to_handle_at matches the mount id in statx\n");
> >         fprintf(stderr, "open_by_handle -m <test_dir> [N] - rename test files, drop caches and try to open by handle\n");
> >         fprintf(stderr, "open_by_handle -p <test_dir>     - create/delete and try to open by handle also test_dir itself\n");
> >         fprintf(stderr, "open_by_handle -i <handles_file> <test_dir> [N] - read test files handles from file and try to open by handle\n");
> > @@ -120,6 +126,81 @@ void usage(void)
> >         exit(EXIT_FAILURE);
> >  }
> >
> > +int do_name_to_handle_at(const char *fname, struct file_handle *fh, int bufsz,
> > +                        int checkmountid)
> > +{
> > +       int ret;
> > +       int mntid_short;
> > +
> > +       uint64_t mntid_unique;
> > +       uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
> > +       struct handle dummy_fh;
> > +
> > +       if (checkmountid) {
> > +               struct statx statxbuf;
> > +
> > +               /* Get both the short and unique mount id. */
> > +               if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) {
> > +                       fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
> > +                       return EXIT_FAILURE;
> > +               }
> > +               if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
> > +                       fprintf(stderr, "%s: no STATX_MNT_ID in stx_mask\n", fname);
> > +                       return EXIT_FAILURE;
> > +               }
> > +               statx_mntid_short = statxbuf.stx_mnt_id;
> > +
> > +               if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) {
> > +                       fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname);
> > +                       return EXIT_FAILURE;
> 
> This failure will break the test on LTS kernels  - we don't want that.
> Instead I think you should:
> - drop the -M option
> - get statx_mntid_unique here IF kernel supports STATX_MNT_ID_UNIQUE
> and then...

Ah okay, I wasn't sure if the xfstests policy was like selftests where
only the latest kernel matters. I'll send a v2 with the suggestions you
mentioned.

However, presumably this means we would also not do the
STATX_MNT_ID_UNIQUE check if name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE)
returns -EINVAL, right?

> > +               }
> > +               if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)) {
> > +                       fprintf(stderr, "%s: no STATX_MNT_ID_UNIQUE in stx_mask\n", fname);
> > +                       return EXIT_FAILURE;
> > +               }
> > +               statx_mntid_unique = statxbuf.stx_mnt_id;
> > +       }
> > +
> > +       fh->handle_bytes = bufsz;
> > +       ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
> > +       if (bufsz < fh->handle_bytes) {
> > +               /* Query the filesystem required bufsz and the file handle */
> > +               if (ret != -1 || errno != EOVERFLOW) {
> > +                       fprintf(stderr, "%s: unexpected result from name_to_handle_at: %d (%m)\n", fname, ret);
> > +                       return EXIT_FAILURE;
> > +               }
> > +               ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
> > +       }
> > +       if (ret < 0) {
> > +               fprintf(stderr, "%s: name_to_handle: %m\n", fname);
> > +               return EXIT_FAILURE;
> > +       }
> > +
> > +       if (checkmountid) {
> > +               if (mntid_short != (int) statx_mntid_short) {
> > +                       fprintf(stderr, "%s: name_to_handle_at returned a different mount ID to STATX_MNT_ID: %u != %lu\n", fname, mntid_short, statx_mntid_short);
> > +                       return EXIT_FAILURE;
> > +               }
> > +
> > +               /*
> > +                * Get the unique mount ID. We don't need to get another copy of the
> > +                * handle so store it in a dummy struct.
> > +                */
> > +               dummy_fh.fh.handle_bytes = fh->handle_bytes;
> > +               if (name_to_handle_at(AT_FDCWD, fname, &dummy_fh.fh, (int *) &mntid_unique, AT_HANDLE_MNT_ID_UNIQUE) < 0) {
> > +                       fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE): %m\n", fname);
> > +                       return EXIT_FAILURE;
> > +               }
> > +
> > +               if (mntid_unique != statx_mntid_unique) {
> > +                       fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) returned a different mount ID to STATX_MNT_ID_UNIQUE: %lu != %lu\n", fname, mntid_unique, statx_mntid_unique);
> > +                       return EXIT_FAILURE;
> > +               }
> 
> - check statx_mntid_unique here IFF statx_mntid_unique is set
> - always check statx_mntid_short (what could be a reason to not check it?)
> 
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  int main(int argc, char **argv)
> >  {
> >         int     i, c;
> > @@ -132,19 +213,20 @@ int main(int argc, char **argv)
> >         char    fname2[PATH_MAX];
> >         char    *test_dir;
> >         char    *mount_dir;
> > -       int     mount_fd, mount_id;
> > +       int     mount_fd;
> >         char    *infile = NULL, *outfile = NULL;
> >         int     in_fd = 0, out_fd = 0;
> >         int     numfiles = 1;
> >         int     create = 0, delete = 0, nlink = 1, move = 0;
> >         int     rd = 0, wr = 0, wrafter = 0, parent = 0;
> >         int     keepopen = 0, drop_caches = 1, sleep_loop = 0;
> > +       int     checkmountid = 0;
> >         int     bufsz = MAX_HANDLE_SZ;
> >
> >         if (argc < 2)
> >                 usage();
> >
> > -       while ((c = getopt(argc, argv, "cludmrwapknhi:o:sz")) != -1) {
> > +       while ((c = getopt(argc, argv, "cludMmrwapknhi:o:sz")) != -1) {
> >                 switch (c) {
> >                 case 'c':
> >                         create = 1;
> > @@ -172,6 +254,9 @@ int main(int argc, char **argv)
> >                         delete = 1;
> >                         nlink = 0;
> >                         break;
> > +               case 'M':
> > +                       checkmountid = 1;
> > +                       break;
> >                 case 'm':
> >                         move = 1;
> >                         break;
> > @@ -307,21 +392,9 @@ int main(int argc, char **argv)
> >                                 return EXIT_FAILURE;
> >                         }
> >                 } else {
> > -                       handle[i].fh.handle_bytes = bufsz;
> > -                       ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
> > -                       if (bufsz < handle[i].fh.handle_bytes) {
> > -                               /* Query the filesystem required bufsz and the file handle */
> > -                               if (ret != -1 || errno != EOVERFLOW) {
> > -                                       fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", fname);
> > -                                       return EXIT_FAILURE;
> > -                               }
> > -                               ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
> > -                       }
> > -                       if (ret < 0) {
> > -                               strcat(fname, ": name_to_handle");
> > -                               perror(fname);
> > +                       ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz, checkmountid);
> > +                       if (ret < 0)
> >                                 return EXIT_FAILURE;
> > -                       }
> >                 }
> >                 if (keepopen) {
> >                         /* Open without close to keep unlinked files around */
> > @@ -349,21 +422,9 @@ int main(int argc, char **argv)
> >                                 return EXIT_FAILURE;
> >                         }
> >                 } else {
> > -                       dir_handle.fh.handle_bytes = bufsz;
> > -                       ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
> > -                       if (bufsz < dir_handle.fh.handle_bytes) {
> > -                               /* Query the filesystem required bufsz and the file handle */
> > -                               if (ret != -1 || errno != EOVERFLOW) {
> > -                                       fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", dname);
> > -                                       return EXIT_FAILURE;
> > -                               }
> > -                               ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
> > -                       }
> > -                       if (ret < 0) {
> > -                               strcat(dname, ": name_to_handle");
> > -                               perror(dname);
> > +                       ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz, checkmountid);
> > +                       if (ret < 0)
> >                                 return EXIT_FAILURE;
> > -                       }
> >                 }
> >                 if (out_fd) {
> >                         ret = write(out_fd, (char *)&dir_handle, sizeof(*handle));
> > diff --git a/tests/generic/426 b/tests/generic/426
> > index 25909f220e1e..df481c58562c 100755
> > --- a/tests/generic/426
> > +++ b/tests/generic/426
> > @@ -51,6 +51,7 @@ test_file_handles $testdir -d
> >  # Check non-stale handles to linked files
> >  create_test_files $testdir
> >  test_file_handles $testdir
> > +test_file_handles $testdir -M
> 
> I see no reason to add option -M and add a second invocation.
> 
> Something I am missing?

Given how many other custom modes there were, I assumed that providing
it as an additional flag would've been preferred. I'll just make it
automatic.

Thanks!

> 
> Thanks,
> Amir.

-- 
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: [PATCH RESEND v3 1/2] uapi: explain how per-syscall AT_* flags should be allocated

[Jan Kara]

On Wed 28-08-24 20:19:42, Aleksa Sarai wrote:
> Unfortunately, the way we have gone about adding new AT_* flags has
> been a little messy. In the beginning, all of the AT_* flags had generic
> meanings and so it made sense to share the flag bits indiscriminately.
> However, we inevitably ran into syscalls that needed their own
> syscall-specific flags. Due to the lack of a planned out policy, we
> ended up with the following situations:
> 
>  * Existing syscalls adding new features tended to use new AT_* bits,
>    with some effort taken to try to re-use bits for flags that were so
>    obviously syscall specific that they only make sense for a single
>    syscall (such as the AT_EACCESS/AT_REMOVEDIR/AT_HANDLE_FID triplet).
> 
>    Given the constraints of bitflags, this works well in practice, but
>    ideally (to avoid future confusion) we would plan ahead and define a
>    set of "per-syscall bits" ahead of time so that when allocating new
>    bits we don't end up with a complete mish-mash of which bits are
>    supposed to be per-syscall and which aren't.
> 
>  * New syscalls dealt with this in several ways:
> 
>    - Some syscalls (like renameat2(2), move_mount(2), fsopen(2), and
>      fspick(2)) created their separate own flag spaces that have no
>      overlap with the AT_* flags. Most of these ended up allocating
>      their bits sequentually.
> 
>      In the case of move_mount(2) and fspick(2), several flags have
>      identical meanings to AT_* flags but were allocated in their own
>      flag space.
> 
>      This makes sense for syscalls that will never share AT_* flags, but
>      for some syscalls this leads to duplication with AT_* flags in a
>      way that could cause confusion (if renameat2(2) grew a
>      RENAME_EMPTY_PATH it seems likely that users could mistake it for
>      AT_EMPTY_PATH since it is an *at(2) syscall).
> 
>    - Some syscalls unfortunately ended up both creating their own flag
>      space while also using bits from other flag spaces. The most
>      obvious example is open_tree(2), where the standard usage ends up
>      using flags from *THREE* separate flag spaces:
> 
>        open_tree(AT_FDCWD, "/foo", OPEN_TREE_CLONE|O_CLOEXEC|AT_RECURSIVE);
> 
>      (Note that O_CLOEXEC is also platform-specific, so several future
>      OPEN_TREE_* bits are also made unusable in one fell swoop.)
> 
> It's not entirely clear to me what the "right" choice is for new
> syscalls. Just saying that all future VFS syscalls should use AT_* flags
> doesn't seem practical. openat2(2) has RESOLVE_* flags (many of which
> don't make much sense to burn generic AT_* flags for) and move_mount(2)
> has separate AT_*-like flags for both the source and target so separate
> flags are needed anyway (though it seems possible that renameat2(2)
> could grow *_EMPTY_PATH flags at some point, and it's a bit of a shame
> they can't be reused).
> 
> But at least for syscalls that _do_ choose to use AT_* flags, we should
> explicitly state the policy that 0x2ff is currently intended for
> per-syscall flags and that new flags should err on the side of
> overlapping with existing flag bits (so we can extend the scope of
> generic flags in the future if necessary).
> 
> And add AT_* aliases for the RENAME_* flags to further cement that
> renameat2(2) is an *at(2) flag, just with its own per-syscall flags.
> 
> Suggested-by: Amir Goldstein <amir73il@gmail.com>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  include/uapi/linux/fcntl.h                         | 80 ++++++++++++++-------
>  tools/perf/trace/beauty/include/uapi/linux/fcntl.h | 83 +++++++++++++++-------
>  2 files changed, 115 insertions(+), 48 deletions(-)
> 
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index e55a3314bcb0..38a6d66d9e88 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -90,37 +90,69 @@
>  #define DN_ATTRIB	0x00000020	/* File changed attibutes */
>  #define DN_MULTISHOT	0x80000000	/* Don't remove notifier */
>  
> +#define AT_FDCWD		-100    /* Special value for dirfd used to
> +					   indicate openat should use the
> +					   current working directory. */
> +
> +
> +/* Generic flags for the *at(2) family of syscalls. */
> +
> +/* Reserved for per-syscall flags	0xff. */
> +#define AT_SYMLINK_NOFOLLOW		0x100   /* Do not follow symbolic
> +						   links. */
> +/* Reserved for per-syscall flags	0x200 */
> +#define AT_SYMLINK_FOLLOW		0x400   /* Follow symbolic links. */
> +#define AT_NO_AUTOMOUNT			0x800	/* Suppress terminal automount
> +						   traversal. */
> +#define AT_EMPTY_PATH			0x1000	/* Allow empty relative
> +						   pathname to operate on dirfd
> +						   directly. */
>  /*
> - * The constants AT_REMOVEDIR and AT_EACCESS have the same value.  AT_EACCESS is
> - * meaningful only to faccessat, while AT_REMOVEDIR is meaningful only to
> - * unlinkat.  The two functions do completely different things and therefore,
> - * the flags can be allowed to overlap.  For example, passing AT_REMOVEDIR to
> - * faccessat would be undefined behavior and thus treating it equivalent to
> - * AT_EACCESS is valid undefined behavior.
> + * These flags are currently statx(2)-specific, but they could be made generic
> + * in the future and so they should not be used for other per-syscall flags.
>   */
> -#define AT_FDCWD		-100    /* Special value used to indicate
> -                                           openat should use the current
> -                                           working directory. */
> -#define AT_SYMLINK_NOFOLLOW	0x100   /* Do not follow symbolic links.  */
> +#define AT_STATX_SYNC_TYPE		0x6000	/* Type of synchronisation required from statx() */
> +#define AT_STATX_SYNC_AS_STAT		0x0000	/* - Do whatever stat() does */
> +#define AT_STATX_FORCE_SYNC		0x2000	/* - Force the attributes to be sync'd with the server */
> +#define AT_STATX_DONT_SYNC		0x4000	/* - Don't sync attributes with the server */
> +
> +#define AT_RECURSIVE			0x8000	/* Apply to the entire subtree */
> +
> +/*
> + * Per-syscall flags for the *at(2) family of syscalls.
> + *
> + * These are flags that are so syscall-specific that a user passing these flags
> + * to the wrong syscall is so "clearly wrong" that we can safely call such
> + * usage "undefined behaviour".
> + *
> + * For example, the constants AT_REMOVEDIR and AT_EACCESS have the same value.
> + * AT_EACCESS is meaningful only to faccessat, while AT_REMOVEDIR is meaningful
> + * only to unlinkat. The two functions do completely different things and
> + * therefore, the flags can be allowed to overlap. For example, passing
> + * AT_REMOVEDIR to faccessat would be undefined behavior and thus treating it
> + * equivalent to AT_EACCESS is valid undefined behavior.
> + *
> + * Note for implementers: When picking a new per-syscall AT_* flag, try to
> + * reuse already existing flags first. This leaves us with as many unused bits
> + * as possible, so we can use them for generic bits in the future if necessary.
> + */
> +
> +/* Flags for renameat2(2) (must match legacy RENAME_* flags). */
> +#define AT_RENAME_NOREPLACE	0x0001
> +#define AT_RENAME_EXCHANGE	0x0002
> +#define AT_RENAME_WHITEOUT	0x0004
> +
> +/* Flag for faccessat(2). */
>  #define AT_EACCESS		0x200	/* Test access permitted for
>                                             effective IDs, not real IDs.  */
> +/* Flag for unlinkat(2). */
>  #define AT_REMOVEDIR		0x200   /* Remove directory instead of
>                                             unlinking file.  */
> -#define AT_SYMLINK_FOLLOW	0x400   /* Follow symbolic links.  */
> -#define AT_NO_AUTOMOUNT		0x800	/* Suppress terminal automount traversal */
> -#define AT_EMPTY_PATH		0x1000	/* Allow empty relative pathname */
> -
> -#define AT_STATX_SYNC_TYPE	0x6000	/* Type of synchronisation required from statx() */
> -#define AT_STATX_SYNC_AS_STAT	0x0000	/* - Do whatever stat() does */
> -#define AT_STATX_FORCE_SYNC	0x2000	/* - Force the attributes to be sync'd with the server */
> -#define AT_STATX_DONT_SYNC	0x4000	/* - Don't sync attributes with the server */
> -
> -#define AT_RECURSIVE		0x8000	/* Apply to the entire subtree */
> +/* Flags for name_to_handle_at(2). */
> +#define AT_HANDLE_FID		0x200	/* File handle is needed to compare
> +					   object identity and may not be
> +					   usable with open_by_handle_at(2). */
>  
> -/* Flags for name_to_handle_at(2). We reuse AT_ flag space to save bits... */
> -#define AT_HANDLE_FID		AT_REMOVEDIR	/* file handle is needed to
> -					compare object identity and may not
> -					be usable to open_by_handle_at(2) */
>  #if defined(__KERNEL__)
>  #define AT_GETATTR_NOSEC	0x80000000
>  #endif
> diff --git a/tools/perf/trace/beauty/include/uapi/linux/fcntl.h b/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
> index c0bcc185fa48..38a6d66d9e88 100644
> --- a/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
> +++ b/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
> @@ -16,6 +16,9 @@
>  
>  #define F_DUPFD_QUERY	(F_LINUX_SPECIFIC_BASE + 3)
>  
> +/* Was the file just created? */
> +#define F_CREATED_QUERY	(F_LINUX_SPECIFIC_BASE + 4)
> +
>  /*
>   * Cancel a blocking posix lock; internal use only until we expose an
>   * asynchronous lock api to userspace:
> @@ -87,37 +90,69 @@
>  #define DN_ATTRIB	0x00000020	/* File changed attibutes */
>  #define DN_MULTISHOT	0x80000000	/* Don't remove notifier */
>  
> +#define AT_FDCWD		-100    /* Special value for dirfd used to
> +					   indicate openat should use the
> +					   current working directory. */
> +
> +
> +/* Generic flags for the *at(2) family of syscalls. */
> +
> +/* Reserved for per-syscall flags	0xff. */
> +#define AT_SYMLINK_NOFOLLOW		0x100   /* Do not follow symbolic
> +						   links. */
> +/* Reserved for per-syscall flags	0x200 */
> +#define AT_SYMLINK_FOLLOW		0x400   /* Follow symbolic links. */
> +#define AT_NO_AUTOMOUNT			0x800	/* Suppress terminal automount
> +						   traversal. */
> +#define AT_EMPTY_PATH			0x1000	/* Allow empty relative
> +						   pathname to operate on dirfd
> +						   directly. */
> +/*
> + * These flags are currently statx(2)-specific, but they could be made generic
> + * in the future and so they should not be used for other per-syscall flags.
> + */
> +#define AT_STATX_SYNC_TYPE		0x6000	/* Type of synchronisation required from statx() */
> +#define AT_STATX_SYNC_AS_STAT		0x0000	/* - Do whatever stat() does */
> +#define AT_STATX_FORCE_SYNC		0x2000	/* - Force the attributes to be sync'd with the server */
> +#define AT_STATX_DONT_SYNC		0x4000	/* - Don't sync attributes with the server */
> +
> +#define AT_RECURSIVE			0x8000	/* Apply to the entire subtree */
> +
>  /*
> - * The constants AT_REMOVEDIR and AT_EACCESS have the same value.  AT_EACCESS is
> - * meaningful only to faccessat, while AT_REMOVEDIR is meaningful only to
> - * unlinkat.  The two functions do completely different things and therefore,
> - * the flags can be allowed to overlap.  For example, passing AT_REMOVEDIR to
> - * faccessat would be undefined behavior and thus treating it equivalent to
> - * AT_EACCESS is valid undefined behavior.
> + * Per-syscall flags for the *at(2) family of syscalls.
> + *
> + * These are flags that are so syscall-specific that a user passing these flags
> + * to the wrong syscall is so "clearly wrong" that we can safely call such
> + * usage "undefined behaviour".
> + *
> + * For example, the constants AT_REMOVEDIR and AT_EACCESS have the same value.
> + * AT_EACCESS is meaningful only to faccessat, while AT_REMOVEDIR is meaningful
> + * only to unlinkat. The two functions do completely different things and
> + * therefore, the flags can be allowed to overlap. For example, passing
> + * AT_REMOVEDIR to faccessat would be undefined behavior and thus treating it
> + * equivalent to AT_EACCESS is valid undefined behavior.
> + *
> + * Note for implementers: When picking a new per-syscall AT_* flag, try to
> + * reuse already existing flags first. This leaves us with as many unused bits
> + * as possible, so we can use them for generic bits in the future if necessary.
>   */
> -#define AT_FDCWD		-100    /* Special value used to indicate
> -                                           openat should use the current
> -                                           working directory. */
> -#define AT_SYMLINK_NOFOLLOW	0x100   /* Do not follow symbolic links.  */
> +
> +/* Flags for renameat2(2) (must match legacy RENAME_* flags). */
> +#define AT_RENAME_NOREPLACE	0x0001
> +#define AT_RENAME_EXCHANGE	0x0002
> +#define AT_RENAME_WHITEOUT	0x0004
> +
> +/* Flag for faccessat(2). */
>  #define AT_EACCESS		0x200	/* Test access permitted for
>                                             effective IDs, not real IDs.  */
> +/* Flag for unlinkat(2). */
>  #define AT_REMOVEDIR		0x200   /* Remove directory instead of
>                                             unlinking file.  */
> -#define AT_SYMLINK_FOLLOW	0x400   /* Follow symbolic links.  */
> -#define AT_NO_AUTOMOUNT		0x800	/* Suppress terminal automount traversal */
> -#define AT_EMPTY_PATH		0x1000	/* Allow empty relative pathname */
> -
> -#define AT_STATX_SYNC_TYPE	0x6000	/* Type of synchronisation required from statx() */
> -#define AT_STATX_SYNC_AS_STAT	0x0000	/* - Do whatever stat() does */
> -#define AT_STATX_FORCE_SYNC	0x2000	/* - Force the attributes to be sync'd with the server */
> -#define AT_STATX_DONT_SYNC	0x4000	/* - Don't sync attributes with the server */
> -
> -#define AT_RECURSIVE		0x8000	/* Apply to the entire subtree */
> +/* Flags for name_to_handle_at(2). */
> +#define AT_HANDLE_FID		0x200	/* File handle is needed to compare
> +					   object identity and may not be
> +					   usable with open_by_handle_at(2). */
>  
> -/* Flags for name_to_handle_at(2). We reuse AT_ flag space to save bits... */
> -#define AT_HANDLE_FID		AT_REMOVEDIR	/* file handle is needed to
> -					compare object identity and may not
> -					be usable to open_by_handle_at(2) */
>  #if defined(__KERNEL__)
>  #define AT_GETATTR_NOSEC	0x80000000
>  #endif
> 
> -- 
> 2.46.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH RESEND v3 2/2] fhandle: expose u64 mount id to name_to_handle_at(2)

[Jan Kara]

On Wed 28-08-24 20:19:43, Aleksa Sarai wrote:
> Now that we provide a unique 64-bit mount ID interface in statx(2), we
> can now provide a race-free way for name_to_handle_at(2) to provide a
> file handle and corresponding mount without needing to worry about
> racing with /proc/mountinfo parsing or having to open a file just to do
> statx(2).
> 
> While this is not necessary if you are using AT_EMPTY_PATH and don't
> care about an extra statx(2) call, users that pass full paths into
> name_to_handle_at(2) need to know which mount the file handle comes from
> (to make sure they don't try to open_by_handle_at a file handle from a
> different filesystem) and switching to AT_EMPTY_PATH would require
> allocating a file for every name_to_handle_at(2) call, turning
> 
>   err = name_to_handle_at(-EBADF, "/foo/bar/baz", &handle, &mntid,
>                           AT_HANDLE_MNT_ID_UNIQUE);
> 
> into
> 
>   int fd = openat(-EBADF, "/foo/bar/baz", O_PATH | O_CLOEXEC);
>   err1 = name_to_handle_at(fd, "", &handle, &unused_mntid, AT_EMPTY_PATH);
>   err2 = statx(fd, "", AT_EMPTY_PATH, STATX_MNT_ID_UNIQUE, &statxbuf);
>   mntid = statxbuf.stx_mnt_id;
>   close(fd);
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/fhandle.c                                       | 29 ++++++++++++++++------
>  include/linux/syscalls.h                           |  2 +-
>  include/uapi/linux/fcntl.h                         |  1 +
>  tools/perf/trace/beauty/include/uapi/linux/fcntl.h |  1 +
>  4 files changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 6e8cea16790e..8cb665629f4a 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -16,7 +16,8 @@
>  
>  static long do_sys_name_to_handle(const struct path *path,
>  				  struct file_handle __user *ufh,
> -				  int __user *mnt_id, int fh_flags)
> +				  void __user *mnt_id, bool unique_mntid,
> +				  int fh_flags)
>  {
>  	long retval;
>  	struct file_handle f_handle;
> @@ -69,9 +70,19 @@ static long do_sys_name_to_handle(const struct path *path,
>  	} else
>  		retval = 0;
>  	/* copy the mount id */
> -	if (put_user(real_mount(path->mnt)->mnt_id, mnt_id) ||
> -	    copy_to_user(ufh, handle,
> -			 struct_size(handle, f_handle, handle_bytes)))
> +	if (unique_mntid) {
> +		if (put_user(real_mount(path->mnt)->mnt_id_unique,
> +			     (u64 __user *) mnt_id))
> +			retval = -EFAULT;
> +	} else {
> +		if (put_user(real_mount(path->mnt)->mnt_id,
> +			     (int __user *) mnt_id))
> +			retval = -EFAULT;
> +	}
> +	/* copy the handle */
> +	if (retval != -EFAULT &&
> +		copy_to_user(ufh, handle,
> +			     struct_size(handle, f_handle, handle_bytes)))
>  		retval = -EFAULT;
>  	kfree(handle);
>  	return retval;
> @@ -83,6 +94,7 @@ static long do_sys_name_to_handle(const struct path *path,
>   * @name: name that should be converted to handle.
>   * @handle: resulting file handle
>   * @mnt_id: mount id of the file system containing the file
> + *          (u64 if AT_HANDLE_MNT_ID_UNIQUE, otherwise int)
>   * @flag: flag value to indicate whether to follow symlink or not
>   *        and whether a decodable file handle is required.
>   *
> @@ -92,7 +104,7 @@ static long do_sys_name_to_handle(const struct path *path,
>   * value required.
>   */
>  SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
> -		struct file_handle __user *, handle, int __user *, mnt_id,
> +		struct file_handle __user *, handle, void __user *, mnt_id,
>  		int, flag)
>  {
>  	struct path path;
> @@ -100,7 +112,8 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
>  	int fh_flags;
>  	int err;
>  
> -	if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID))
> +	if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID |
> +		     AT_HANDLE_MNT_ID_UNIQUE))
>  		return -EINVAL;
>  
>  	lookup_flags = (flag & AT_SYMLINK_FOLLOW) ? LOOKUP_FOLLOW : 0;
> @@ -109,7 +122,9 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
>  		lookup_flags |= LOOKUP_EMPTY;
>  	err = user_path_at(dfd, name, lookup_flags, &path);
>  	if (!err) {
> -		err = do_sys_name_to_handle(&path, handle, mnt_id, fh_flags);
> +		err = do_sys_name_to_handle(&path, handle, mnt_id,
> +					    flag & AT_HANDLE_MNT_ID_UNIQUE,
> +					    fh_flags);
>  		path_put(&path);
>  	}
>  	return err;
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 4bcf6754738d..5758104921e6 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -870,7 +870,7 @@ asmlinkage long sys_fanotify_mark(int fanotify_fd, unsigned int flags,
>  #endif
>  asmlinkage long sys_name_to_handle_at(int dfd, const char __user *name,
>  				      struct file_handle __user *handle,
> -				      int __user *mnt_id, int flag);
> +				      void __user *mnt_id, int flag);
>  asmlinkage long sys_open_by_handle_at(int mountdirfd,
>  				      struct file_handle __user *handle,
>  				      int flags);
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index 38a6d66d9e88..87e2dec79fea 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -152,6 +152,7 @@
>  #define AT_HANDLE_FID		0x200	/* File handle is needed to compare
>  					   object identity and may not be
>  					   usable with open_by_handle_at(2). */
> +#define AT_HANDLE_MNT_ID_UNIQUE	0x001	/* Return the u64 unique mount ID. */
>  
>  #if defined(__KERNEL__)
>  #define AT_GETATTR_NOSEC	0x80000000
> diff --git a/tools/perf/trace/beauty/include/uapi/linux/fcntl.h b/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
> index 38a6d66d9e88..87e2dec79fea 100644
> --- a/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
> +++ b/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
> @@ -152,6 +152,7 @@
>  #define AT_HANDLE_FID		0x200	/* File handle is needed to compare
>  					   object identity and may not be
>  					   usable with open_by_handle_at(2). */
> +#define AT_HANDLE_MNT_ID_UNIQUE	0x001	/* Return the u64 unique mount ID. */
>  
>  #if defined(__KERNEL__)
>  #define AT_GETATTR_NOSEC	0x80000000
> 
> -- 
> 2.46.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH RESEND v3 0/2] fhandle: expose u64 mount id to name_to_handle_at(2)

[Christian Brauner]

On Wed, 28 Aug 2024 20:19:41 +1000, Aleksa Sarai wrote:
> Now that we provide a unique 64-bit mount ID interface in statx(2), we
> can now provide a race-free way for name_to_handle_at(2) to provide a
> file handle and corresponding mount without needing to worry about
> racing with /proc/mountinfo parsing or having to open a file just to do
> statx(2).
> 
> While this is not necessary if you are using AT_EMPTY_PATH and don't
> care about an extra statx(2) call, users that pass full paths into
> name_to_handle_at(2) need to know which mount the file handle comes from
> (to make sure they don't try to open_by_handle_at a file handle from a
> different filesystem) and switching to AT_EMPTY_PATH would require
> allocating a file for every name_to_handle_at(2) call, turning
> 
> [...]

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc

[1/2] uapi: explain how per-syscall AT_* flags should be allocated
      https://git.kernel.org/vfs/vfs/c/34cf40849654
[2/2] fhandle: expose u64 mount id to name_to_handle_at(2)
      https://git.kernel.org/vfs/vfs/c/9cde4ebc6f4f

[PATCH xfstests v2 1/2] statx: update headers to include newer statx fields

[Aleksa Sarai]

These come from Linux v6.11-rc5.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 src/open_by_handle.c |  4 +++-
 src/statx.h          | 22 ++++++++++++++++++++--
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/src/open_by_handle.c b/src/open_by_handle.c
index 0f74ed08b1f0..d9c802ca9bd1 100644
--- a/src/open_by_handle.c
+++ b/src/open_by_handle.c
@@ -82,12 +82,14 @@ Examples:
 #include <string.h>
 #include <fcntl.h>
 #include <unistd.h>
-#include <sys/stat.h>
 #include <sys/types.h>
 #include <errno.h>
 #include <linux/limits.h>
 #include <libgen.h>
 
+#include <sys/stat.h>
+#include "statx.h"
+
 #define MAXFILES 1024
 
 struct handle {
diff --git a/src/statx.h b/src/statx.h
index 3f239d791dfe..935cb2ed415e 100644
--- a/src/statx.h
+++ b/src/statx.h
@@ -102,7 +102,7 @@ struct statx {
 	__u64	stx_ino;	/* Inode number */
 	__u64	stx_size;	/* File size */
 	__u64	stx_blocks;	/* Number of 512-byte blocks allocated */
-	__u64	__spare1[1];
+	__u64	stx_attributes_mask; /* Mask to show what's supported in stx_attributes */
 	/* 0x40 */
 	struct statx_timestamp	stx_atime;	/* Last access time */
 	struct statx_timestamp	stx_btime;	/* File creation time */
@@ -114,7 +114,18 @@ struct statx {
 	__u32	stx_dev_major;	/* ID of device containing file [uncond] */
 	__u32	stx_dev_minor;
 	/* 0x90 */
-	__u64	__spare2[14];	/* Spare space for future expansion */
+	__u64	stx_mnt_id;
+	__u32	stx_dio_mem_align;	/* Memory buffer alignment for direct I/O */
+	__u32	stx_dio_offset_align;	/* File offset alignment for direct I/O */
+	/* 0xa0 */
+	__u64	stx_subvol;	/* Subvolume identifier */
+	__u32	stx_atomic_write_unit_min;	/* Min atomic write unit in bytes */
+	__u32	stx_atomic_write_unit_max;	/* Max atomic write unit in bytes */
+	/* 0xb0 */
+	__u32   stx_atomic_write_segments_max;	/* Max atomic write segment count */
+	__u32   __spare1[1];
+	/* 0xb8 */
+	__u64	__spare3[9];	/* Spare space for future expansion */
 	/* 0x100 */
 };
 
@@ -139,6 +150,13 @@ struct statx {
 #define STATX_BLOCKS		0x00000400U	/* Want/got stx_blocks */
 #define STATX_BASIC_STATS	0x000007ffU	/* The stuff in the normal stat struct */
 #define STATX_BTIME		0x00000800U	/* Want/got stx_btime */
+#define STATX_MNT_ID		0x00001000U	/* Got stx_mnt_id */
+#define STATX_DIOALIGN		0x00002000U	/* Want/got direct I/O alignment info */
+#define STATX_MNT_ID_UNIQUE	0x00004000U	/* Want/got extended stx_mount_id */
+#define STATX_SUBVOL		0x00008000U	/* Want/got stx_subvol */
+#define STATX_WRITE_ATOMIC	0x00010000U	/* Want/got atomic_write_* fields */
+
+#define STATX__RESERVED		0x80000000U	/* Reserved for future struct statx expansion */
 #define STATX_ALL		0x00000fffU	/* All currently supported flags */
 
 /*
-- 
2.46.0

[PATCH xfstests v2 2/2] open_by_handle: add tests for u64 mount ID

[Aleksa Sarai]

Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
make sure they match properly as part of the regular open_by_handle
tests.

Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
v2:
- Remove -M argument and always do the mount ID tests. [Amir Goldstein]
- Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
  or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
- v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>

 src/open_by_handle.c | 128 +++++++++++++++++++++++++++++++++----------
 1 file changed, 99 insertions(+), 29 deletions(-)

diff --git a/src/open_by_handle.c b/src/open_by_handle.c
index d9c802ca9bd1..0ad591da632e 100644
--- a/src/open_by_handle.c
+++ b/src/open_by_handle.c
@@ -86,10 +86,16 @@ Examples:
 #include <errno.h>
 #include <linux/limits.h>
 #include <libgen.h>
+#include <stdint.h>
+#include <stdbool.h>
 
 #include <sys/stat.h>
 #include "statx.h"
 
+#ifndef AT_HANDLE_MNT_ID_UNIQUE
+#	define AT_HANDLE_MNT_ID_UNIQUE 0x001
+#endif
+
 #define MAXFILES 1024
 
 struct handle {
@@ -120,6 +126,94 @@ void usage(void)
 	exit(EXIT_FAILURE);
 }
 
+int do_name_to_handle_at(const char *fname, struct file_handle *fh, int bufsz)
+{
+	int ret;
+	int mntid_short;
+
+	static bool skip_mntid_unique;
+
+	uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
+	struct statx statxbuf;
+
+	/* Get both the short and unique mount id. */
+	if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) {
+		fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
+		return EXIT_FAILURE;
+	}
+	if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
+		fprintf(stderr, "%s: no STATX_MNT_ID in stx_mask\n", fname);
+		return EXIT_FAILURE;
+	}
+	statx_mntid_short = statxbuf.stx_mnt_id;
+
+	if (!skip_mntid_unique) {
+		if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) {
+			fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname);
+			return EXIT_FAILURE;
+		}
+		/*
+		 * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
+		 * kernel doesn't give us a unique mount ID just skip it.
+		 */
+		if ((skip_mntid_unique |= !(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)))
+			printf("statx(STATX_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n");
+		else
+			statx_mntid_unique = statxbuf.stx_mnt_id;
+	}
+
+	fh->handle_bytes = bufsz;
+	ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
+	if (bufsz < fh->handle_bytes) {
+		/* Query the filesystem required bufsz and the file handle */
+		if (ret != -1 || errno != EOVERFLOW) {
+			fprintf(stderr, "%s: unexpected result from name_to_handle_at: %d (%m)\n", fname, ret);
+			return EXIT_FAILURE;
+		}
+		ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
+	}
+	if (ret < 0) {
+		fprintf(stderr, "%s: name_to_handle: %m\n", fname);
+		return EXIT_FAILURE;
+	}
+
+	if (mntid_short != (int) statx_mntid_short) {
+		fprintf(stderr, "%s: name_to_handle_at returned a different mount ID to STATX_MNT_ID: %u != %lu\n", fname, mntid_short, statx_mntid_short);
+		return EXIT_FAILURE;
+	}
+
+	if (!skip_mntid_unique && statx_mntid_unique != 0) {
+		struct handle dummy_fh;
+		uint64_t mntid_unique = 0;
+
+		/*
+		 * Get the unique mount ID. We don't need to get another copy of the
+		 * handle so store it in a dummy struct.
+		 */
+		dummy_fh.fh.handle_bytes = fh->handle_bytes;
+		ret = name_to_handle_at(AT_FDCWD, fname, &dummy_fh.fh, (int *) &mntid_unique, AT_HANDLE_MNT_ID_UNIQUE);
+		if (ret < 0) {
+			if (errno != EINVAL) {
+				fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE): %m\n", fname);
+				return EXIT_FAILURE;
+			}
+			/*
+			 * EINVAL means AT_HANDLE_MNT_ID_UNIQUE is not supported, so skip
+			 * the check in that case.
+			 */
+			printf("name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n");
+			skip_mntid_unique = true;
+		} else {
+			if (mntid_unique != statx_mntid_unique) {
+				fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) returned a different mount ID to STATX_MNT_ID_UNIQUE: %lu != %lu\n", fname, mntid_unique, statx_mntid_unique);
+				return EXIT_FAILURE;
+			}
+		}
+	}
+
+	return 0;
+}
+
 int main(int argc, char **argv)
 {
 	int	i, c;
@@ -132,7 +226,7 @@ int main(int argc, char **argv)
 	char	fname2[PATH_MAX];
 	char	*test_dir;
 	char	*mount_dir;
-	int	mount_fd, mount_id;
+	int	mount_fd;
 	char	*infile = NULL, *outfile = NULL;
 	int	in_fd = 0, out_fd = 0;
 	int	numfiles = 1;
@@ -307,21 +401,9 @@ int main(int argc, char **argv)
 				return EXIT_FAILURE;
 			}
 		} else {
-			handle[i].fh.handle_bytes = bufsz;
-			ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
-			if (bufsz < handle[i].fh.handle_bytes) {
-				/* Query the filesystem required bufsz and the file handle */
-				if (ret != -1 || errno != EOVERFLOW) {
-					fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", fname);
-					return EXIT_FAILURE;
-				}
-				ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
-			}
-			if (ret < 0) {
-				strcat(fname, ": name_to_handle");
-				perror(fname);
+			ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz);
+			if (ret < 0)
 				return EXIT_FAILURE;
-			}
 		}
 		if (keepopen) {
 			/* Open without close to keep unlinked files around */
@@ -349,21 +431,9 @@ int main(int argc, char **argv)
 				return EXIT_FAILURE;
 			}
 		} else {
-			dir_handle.fh.handle_bytes = bufsz;
-			ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
-			if (bufsz < dir_handle.fh.handle_bytes) {
-				/* Query the filesystem required bufsz and the file handle */
-				if (ret != -1 || errno != EOVERFLOW) {
-					fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", dname);
-					return EXIT_FAILURE;
-				}
-				ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
-			}
-			if (ret < 0) {
-				strcat(dname, ": name_to_handle");
-				perror(dname);
+			ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz);
+			if (ret < 0)
 				return EXIT_FAILURE;
-			}
 		}
 		if (out_fd) {
 			ret = write(out_fd, (char *)&dir_handle, sizeof(*handle));
-- 
2.46.0

Re: [PATCH xfstests v2 2/2] open_by_handle: add tests for u64 mount ID

[Amir Goldstein]

On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
> make sure they match properly as part of the regular open_by_handle
> tests.
>
> Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
> v2:
> - Remove -M argument and always do the mount ID tests. [Amir Goldstein]
> - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
>   or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
> - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>

Looks good.

You may add:

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

It'd be nice to get a verification that this is indeed tested on the latest
upstream and does not regress the tests that run the open_by_handle program.

Thanks,
Amir.

>
>  src/open_by_handle.c | 128 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 99 insertions(+), 29 deletions(-)
>
> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> index d9c802ca9bd1..0ad591da632e 100644
> --- a/src/open_by_handle.c
> +++ b/src/open_by_handle.c
> @@ -86,10 +86,16 @@ Examples:
>  #include <errno.h>
>  #include <linux/limits.h>
>  #include <libgen.h>
> +#include <stdint.h>
> +#include <stdbool.h>
>
>  #include <sys/stat.h>
>  #include "statx.h"
>
> +#ifndef AT_HANDLE_MNT_ID_UNIQUE
> +#      define AT_HANDLE_MNT_ID_UNIQUE 0x001
> +#endif
> +
>  #define MAXFILES 1024
>
>  struct handle {
> @@ -120,6 +126,94 @@ void usage(void)
>         exit(EXIT_FAILURE);
>  }
>
> +int do_name_to_handle_at(const char *fname, struct file_handle *fh, int bufsz)
> +{
> +       int ret;
> +       int mntid_short;
> +
> +       static bool skip_mntid_unique;
> +
> +       uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
> +       struct statx statxbuf;
> +
> +       /* Get both the short and unique mount id. */
> +       if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) {
> +               fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
> +               return EXIT_FAILURE;
> +       }
> +       if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
> +               fprintf(stderr, "%s: no STATX_MNT_ID in stx_mask\n", fname);
> +               return EXIT_FAILURE;
> +       }
> +       statx_mntid_short = statxbuf.stx_mnt_id;
> +
> +       if (!skip_mntid_unique) {
> +               if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) {
> +                       fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname);
> +                       return EXIT_FAILURE;
> +               }
> +               /*
> +                * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
> +                * kernel doesn't give us a unique mount ID just skip it.
> +                */
> +               if ((skip_mntid_unique |= !(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)))
> +                       printf("statx(STATX_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n");
> +               else
> +                       statx_mntid_unique = statxbuf.stx_mnt_id;
> +       }
> +
> +       fh->handle_bytes = bufsz;
> +       ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
> +       if (bufsz < fh->handle_bytes) {
> +               /* Query the filesystem required bufsz and the file handle */
> +               if (ret != -1 || errno != EOVERFLOW) {
> +                       fprintf(stderr, "%s: unexpected result from name_to_handle_at: %d (%m)\n", fname, ret);
> +                       return EXIT_FAILURE;
> +               }
> +               ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
> +       }
> +       if (ret < 0) {
> +               fprintf(stderr, "%s: name_to_handle: %m\n", fname);
> +               return EXIT_FAILURE;
> +       }
> +
> +       if (mntid_short != (int) statx_mntid_short) {
> +               fprintf(stderr, "%s: name_to_handle_at returned a different mount ID to STATX_MNT_ID: %u != %lu\n", fname, mntid_short, statx_mntid_short);
> +               return EXIT_FAILURE;
> +       }
> +
> +       if (!skip_mntid_unique && statx_mntid_unique != 0) {
> +               struct handle dummy_fh;
> +               uint64_t mntid_unique = 0;
> +
> +               /*
> +                * Get the unique mount ID. We don't need to get another copy of the
> +                * handle so store it in a dummy struct.
> +                */
> +               dummy_fh.fh.handle_bytes = fh->handle_bytes;
> +               ret = name_to_handle_at(AT_FDCWD, fname, &dummy_fh.fh, (int *) &mntid_unique, AT_HANDLE_MNT_ID_UNIQUE);
> +               if (ret < 0) {
> +                       if (errno != EINVAL) {
> +                               fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE): %m\n", fname);
> +                               return EXIT_FAILURE;
> +                       }
> +                       /*
> +                        * EINVAL means AT_HANDLE_MNT_ID_UNIQUE is not supported, so skip
> +                        * the check in that case.
> +                        */
> +                       printf("name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n");
> +                       skip_mntid_unique = true;
> +               } else {
> +                       if (mntid_unique != statx_mntid_unique) {
> +                               fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) returned a different mount ID to STATX_MNT_ID_UNIQUE: %lu != %lu\n", fname, mntid_unique, statx_mntid_unique);
> +                               return EXIT_FAILURE;
> +                       }
> +               }
> +       }
> +
> +       return 0;
> +}
> +
>  int main(int argc, char **argv)
>  {
>         int     i, c;
> @@ -132,7 +226,7 @@ int main(int argc, char **argv)
>         char    fname2[PATH_MAX];
>         char    *test_dir;
>         char    *mount_dir;
> -       int     mount_fd, mount_id;
> +       int     mount_fd;
>         char    *infile = NULL, *outfile = NULL;
>         int     in_fd = 0, out_fd = 0;
>         int     numfiles = 1;
> @@ -307,21 +401,9 @@ int main(int argc, char **argv)
>                                 return EXIT_FAILURE;
>                         }
>                 } else {
> -                       handle[i].fh.handle_bytes = bufsz;
> -                       ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
> -                       if (bufsz < handle[i].fh.handle_bytes) {
> -                               /* Query the filesystem required bufsz and the file handle */
> -                               if (ret != -1 || errno != EOVERFLOW) {
> -                                       fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", fname);
> -                                       return EXIT_FAILURE;
> -                               }
> -                               ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
> -                       }
> -                       if (ret < 0) {
> -                               strcat(fname, ": name_to_handle");
> -                               perror(fname);
> +                       ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz);
> +                       if (ret < 0)
>                                 return EXIT_FAILURE;
> -                       }
>                 }
>                 if (keepopen) {
>                         /* Open without close to keep unlinked files around */
> @@ -349,21 +431,9 @@ int main(int argc, char **argv)
>                                 return EXIT_FAILURE;
>                         }
>                 } else {
> -                       dir_handle.fh.handle_bytes = bufsz;
> -                       ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
> -                       if (bufsz < dir_handle.fh.handle_bytes) {
> -                               /* Query the filesystem required bufsz and the file handle */
> -                               if (ret != -1 || errno != EOVERFLOW) {
> -                                       fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", dname);
> -                                       return EXIT_FAILURE;
> -                               }
> -                               ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
> -                       }
> -                       if (ret < 0) {
> -                               strcat(dname, ": name_to_handle");
> -                               perror(dname);
> +                       ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz);
> +                       if (ret < 0)
>                                 return EXIT_FAILURE;
> -                       }
>                 }
>                 if (out_fd) {
>                         ret = write(out_fd, (char *)&dir_handle, sizeof(*handle));
> --
> 2.46.0
>

Re: [PATCH xfstests v2 2/2] open_by_handle: add tests for u64 mount ID

[Aleksa Sarai]

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

On 2024-09-02, Amir Goldstein <amir73il@gmail.com> wrote:
> On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> >
> > Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
> > make sure they match properly as part of the regular open_by_handle
> > tests.
> >
> > Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
> > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > ---
> > v2:
> > - Remove -M argument and always do the mount ID tests. [Amir Goldstein]
> > - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
> >   or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
> > - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
> 
> Looks good.
> 
> You may add:
> 
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> 
> It'd be nice to get a verification that this is indeed tested on the latest
> upstream and does not regress the tests that run the open_by_handle program.

I've tested that the fallback works on mainline and correctly does the
test on patched kernels (by running open_by_handle directly) but I
haven't run the suite yet (still getting my mkosi testing setup working
to run fstests...).

> Thanks,
> Amir.
> 
> >
> >  src/open_by_handle.c | 128 +++++++++++++++++++++++++++++++++----------
> >  1 file changed, 99 insertions(+), 29 deletions(-)
> >
> > diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> > index d9c802ca9bd1..0ad591da632e 100644
> > --- a/src/open_by_handle.c
> > +++ b/src/open_by_handle.c
> > @@ -86,10 +86,16 @@ Examples:
> >  #include <errno.h>
> >  #include <linux/limits.h>
> >  #include <libgen.h>
> > +#include <stdint.h>
> > +#include <stdbool.h>
> >
> >  #include <sys/stat.h>
> >  #include "statx.h"
> >
> > +#ifndef AT_HANDLE_MNT_ID_UNIQUE
> > +#      define AT_HANDLE_MNT_ID_UNIQUE 0x001
> > +#endif
> > +
> >  #define MAXFILES 1024
> >
> >  struct handle {
> > @@ -120,6 +126,94 @@ void usage(void)
> >         exit(EXIT_FAILURE);
> >  }
> >
> > +int do_name_to_handle_at(const char *fname, struct file_handle *fh, int bufsz)
> > +{
> > +       int ret;
> > +       int mntid_short;
> > +
> > +       static bool skip_mntid_unique;
> > +
> > +       uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
> > +       struct statx statxbuf;
> > +
> > +       /* Get both the short and unique mount id. */
> > +       if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) {
> > +               fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
> > +               return EXIT_FAILURE;
> > +       }
> > +       if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
> > +               fprintf(stderr, "%s: no STATX_MNT_ID in stx_mask\n", fname);
> > +               return EXIT_FAILURE;
> > +       }
> > +       statx_mntid_short = statxbuf.stx_mnt_id;
> > +
> > +       if (!skip_mntid_unique) {
> > +               if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) {
> > +                       fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname);
> > +                       return EXIT_FAILURE;
> > +               }
> > +               /*
> > +                * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
> > +                * kernel doesn't give us a unique mount ID just skip it.
> > +                */
> > +               if ((skip_mntid_unique |= !(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)))
> > +                       printf("statx(STATX_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n");
> > +               else
> > +                       statx_mntid_unique = statxbuf.stx_mnt_id;
> > +       }
> > +
> > +       fh->handle_bytes = bufsz;
> > +       ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
> > +       if (bufsz < fh->handle_bytes) {
> > +               /* Query the filesystem required bufsz and the file handle */
> > +               if (ret != -1 || errno != EOVERFLOW) {
> > +                       fprintf(stderr, "%s: unexpected result from name_to_handle_at: %d (%m)\n", fname, ret);
> > +                       return EXIT_FAILURE;
> > +               }
> > +               ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
> > +       }
> > +       if (ret < 0) {
> > +               fprintf(stderr, "%s: name_to_handle: %m\n", fname);
> > +               return EXIT_FAILURE;
> > +       }
> > +
> > +       if (mntid_short != (int) statx_mntid_short) {
> > +               fprintf(stderr, "%s: name_to_handle_at returned a different mount ID to STATX_MNT_ID: %u != %lu\n", fname, mntid_short, statx_mntid_short);
> > +               return EXIT_FAILURE;
> > +       }
> > +
> > +       if (!skip_mntid_unique && statx_mntid_unique != 0) {
> > +               struct handle dummy_fh;
> > +               uint64_t mntid_unique = 0;
> > +
> > +               /*
> > +                * Get the unique mount ID. We don't need to get another copy of the
> > +                * handle so store it in a dummy struct.
> > +                */
> > +               dummy_fh.fh.handle_bytes = fh->handle_bytes;
> > +               ret = name_to_handle_at(AT_FDCWD, fname, &dummy_fh.fh, (int *) &mntid_unique, AT_HANDLE_MNT_ID_UNIQUE);
> > +               if (ret < 0) {
> > +                       if (errno != EINVAL) {
> > +                               fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE): %m\n", fname);
> > +                               return EXIT_FAILURE;
> > +                       }
> > +                       /*
> > +                        * EINVAL means AT_HANDLE_MNT_ID_UNIQUE is not supported, so skip
> > +                        * the check in that case.
> > +                        */
> > +                       printf("name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n");
> > +                       skip_mntid_unique = true;
> > +               } else {
> > +                       if (mntid_unique != statx_mntid_unique) {
> > +                               fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) returned a different mount ID to STATX_MNT_ID_UNIQUE: %lu != %lu\n", fname, mntid_unique, statx_mntid_unique);
> > +                               return EXIT_FAILURE;
> > +                       }
> > +               }
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  int main(int argc, char **argv)
> >  {
> >         int     i, c;
> > @@ -132,7 +226,7 @@ int main(int argc, char **argv)
> >         char    fname2[PATH_MAX];
> >         char    *test_dir;
> >         char    *mount_dir;
> > -       int     mount_fd, mount_id;
> > +       int     mount_fd;
> >         char    *infile = NULL, *outfile = NULL;
> >         int     in_fd = 0, out_fd = 0;
> >         int     numfiles = 1;
> > @@ -307,21 +401,9 @@ int main(int argc, char **argv)
> >                                 return EXIT_FAILURE;
> >                         }
> >                 } else {
> > -                       handle[i].fh.handle_bytes = bufsz;
> > -                       ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
> > -                       if (bufsz < handle[i].fh.handle_bytes) {
> > -                               /* Query the filesystem required bufsz and the file handle */
> > -                               if (ret != -1 || errno != EOVERFLOW) {
> > -                                       fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", fname);
> > -                                       return EXIT_FAILURE;
> > -                               }
> > -                               ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
> > -                       }
> > -                       if (ret < 0) {
> > -                               strcat(fname, ": name_to_handle");
> > -                               perror(fname);
> > +                       ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz);
> > +                       if (ret < 0)
> >                                 return EXIT_FAILURE;
> > -                       }
> >                 }
> >                 if (keepopen) {
> >                         /* Open without close to keep unlinked files around */
> > @@ -349,21 +431,9 @@ int main(int argc, char **argv)
> >                                 return EXIT_FAILURE;
> >                         }
> >                 } else {
> > -                       dir_handle.fh.handle_bytes = bufsz;
> > -                       ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
> > -                       if (bufsz < dir_handle.fh.handle_bytes) {
> > -                               /* Query the filesystem required bufsz and the file handle */
> > -                               if (ret != -1 || errno != EOVERFLOW) {
> > -                                       fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", dname);
> > -                                       return EXIT_FAILURE;
> > -                               }
> > -                               ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
> > -                       }
> > -                       if (ret < 0) {
> > -                               strcat(dname, ": name_to_handle");
> > -                               perror(dname);
> > +                       ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz);
> > +                       if (ret < 0)
> >                                 return EXIT_FAILURE;
> > -                       }
> >                 }
> >                 if (out_fd) {
> >                         ret = write(out_fd, (char *)&dir_handle, sizeof(*handle));
> > --
> > 2.46.0
> >

-- 
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: [PATCH xfstests v2 1/2] statx: update headers to include newer statx fields

[Amir Goldstein]

On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> These come from Linux v6.11-rc5.
>
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
>  src/open_by_handle.c |  4 +++-
>  src/statx.h          | 22 ++++++++++++++++++++--
>  2 files changed, 23 insertions(+), 3 deletions(-)
>

This patch conflicts with commit
873e36c9 - statx.h: update to latest kernel UAPI
already in for-next branch (this is the branch to base patches on)

> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> index 0f74ed08b1f0..d9c802ca9bd1 100644
> --- a/src/open_by_handle.c
> +++ b/src/open_by_handle.c
> @@ -82,12 +82,14 @@ Examples:
>  #include <string.h>
>  #include <fcntl.h>
>  #include <unistd.h>
> -#include <sys/stat.h>
>  #include <sys/types.h>
>  #include <errno.h>
>  #include <linux/limits.h>
>  #include <libgen.h>
>
> +#include <sys/stat.h>
> +#include "statx.h"
> +

So probably best to squash this one liner into the 2nd patch.
I guess Zorro can do it on commit if needed.

Thanks,
Amir.

Re: [PATCH xfstests v2 2/2] open_by_handle: add tests for u64 mount ID

[Amir Goldstein]

On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
> make sure they match properly as part of the regular open_by_handle
> tests.
>
> Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
> v2:
> - Remove -M argument and always do the mount ID tests. [Amir Goldstein]
> - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
>   or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
> - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
>
>  src/open_by_handle.c | 128 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 99 insertions(+), 29 deletions(-)
>
> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> index d9c802ca9bd1..0ad591da632e 100644
> --- a/src/open_by_handle.c
> +++ b/src/open_by_handle.c
> @@ -86,10 +86,16 @@ Examples:
>  #include <errno.h>
>  #include <linux/limits.h>
>  #include <libgen.h>
> +#include <stdint.h>
> +#include <stdbool.h>
>
>  #include <sys/stat.h>
>  #include "statx.h"
>
> +#ifndef AT_HANDLE_MNT_ID_UNIQUE
> +#      define AT_HANDLE_MNT_ID_UNIQUE 0x001
> +#endif
> +
>  #define MAXFILES 1024
>
>  struct handle {
> @@ -120,6 +126,94 @@ void usage(void)
>         exit(EXIT_FAILURE);
>  }
>
> +int do_name_to_handle_at(const char *fname, struct file_handle *fh, int bufsz)
> +{
> +       int ret;
> +       int mntid_short;
> +
> +       static bool skip_mntid_unique;
> +
> +       uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
> +       struct statx statxbuf;
> +
> +       /* Get both the short and unique mount id. */
> +       if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) {

This fails build on top of latest for-next branch with commit
873e36c9 - statx.h: update to latest kernel UAPI

It can be fixed by changing to use the private xfstests_statx()
implementation, same as in stat_test.c.

I am not sure how elegant this is, but that's the easy fix.

> +               fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
> +               return EXIT_FAILURE;
> +       }
> +       if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
> +               fprintf(stderr, "%s: no STATX_MNT_ID in stx_mask\n", fname);
> +               return EXIT_FAILURE;
> +       }
> +       statx_mntid_short = statxbuf.stx_mnt_id;
> +
> +       if (!skip_mntid_unique) {
> +               if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) {
> +                       fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname);
> +                       return EXIT_FAILURE;
> +               }
> +               /*
> +                * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
> +                * kernel doesn't give us a unique mount ID just skip it.
> +                */
> +               if ((skip_mntid_unique |= !(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)))
> +                       printf("statx(STATX_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n");

This verbose print breaks all existing "exportfs" tests which do not
expect it in the golden output.

I understand that silently ignoring the failure is not good, but I also
would like to add this test coverage to all the existing tests.

One solution is to resurrect the command line option -M from v1,
but instead of meaning "test unique mount id" let it mean
"do not allow to skip unique mount id" test.

Then you can add a new test that runs open_by_handle -M, but also
implement a helper _require_unique_mntid similar to _require_btime
which is needed for the new test to run only on new kernels.

I'm sorry for this complication, but fstest is a testsuite that runs on
disto and stable kernels as well and we need to allow test coverage
of new features along with stability of the test env.

Thanks,
Amir.

Re: [PATCH xfstests v2 2/2] open_by_handle: add tests for u64 mount ID

[Amir Goldstein]

On Tue, Sep 3, 2024 at 8:41 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> On 2024-09-02, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> > >
> > > Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
> > > make sure they match properly as part of the regular open_by_handle
> > > tests.
> > >
> > > Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
> > > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > > ---
> > > v2:
> > > - Remove -M argument and always do the mount ID tests. [Amir Goldstein]
> > > - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
> > >   or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
> > > - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
> >
> > Looks good.
> >
> > You may add:
> >
> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> >
> > It'd be nice to get a verification that this is indeed tested on the latest
> > upstream and does not regress the tests that run the open_by_handle program.
>
> I've tested that the fallback works on mainline and correctly does the
> test on patched kernels (by running open_by_handle directly) but I
> haven't run the suite yet (still getting my mkosi testing setup working
> to run fstests...).

I am afraid this has to be tested.
I started testing myself and found that it breaks existing tests.
Even if you make the test completely opt-in as in v1 it need to be
tested and _notrun on old kernels.

If you have a new version, I can test it until you get your fstests setup
ready, because anyway I would want to check that your test also
works with overlayfs which has some specialized exportfs tests.
Test by running ./check -overlay -g exportfs, but I can also do that for you.

Thanks,
Amir.

Re: [PATCH xfstests v2 2/2] open_by_handle: add tests for u64 mount ID

[Aleksa Sarai]

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

On 2024-09-03, Amir Goldstein <amir73il@gmail.com> wrote:
> On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> >
> > Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
> > make sure they match properly as part of the regular open_by_handle
> > tests.
> >
> > Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
> > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > ---
> > v2:
> > - Remove -M argument and always do the mount ID tests. [Amir Goldstein]
> > - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
> >   or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
> > - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
> >
> >  src/open_by_handle.c | 128 +++++++++++++++++++++++++++++++++----------
> >  1 file changed, 99 insertions(+), 29 deletions(-)
> >
> > diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> > index d9c802ca9bd1..0ad591da632e 100644
> > --- a/src/open_by_handle.c
> > +++ b/src/open_by_handle.c
> > @@ -86,10 +86,16 @@ Examples:
> >  #include <errno.h>
> >  #include <linux/limits.h>
> >  #include <libgen.h>
> > +#include <stdint.h>
> > +#include <stdbool.h>
> >
> >  #include <sys/stat.h>
> >  #include "statx.h"
> >
> > +#ifndef AT_HANDLE_MNT_ID_UNIQUE
> > +#      define AT_HANDLE_MNT_ID_UNIQUE 0x001
> > +#endif
> > +
> >  #define MAXFILES 1024
> >
> >  struct handle {
> > @@ -120,6 +126,94 @@ void usage(void)
> >         exit(EXIT_FAILURE);
> >  }
> >
> > +int do_name_to_handle_at(const char *fname, struct file_handle *fh, int bufsz)
> > +{
> > +       int ret;
> > +       int mntid_short;
> > +
> > +       static bool skip_mntid_unique;
> > +
> > +       uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
> > +       struct statx statxbuf;
> > +
> > +       /* Get both the short and unique mount id. */
> > +       if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) {
> 
> This fails build on top of latest for-next branch with commit
> 873e36c9 - statx.h: update to latest kernel UAPI
> 
> It can be fixed by changing to use the private xfstests_statx()
> implementation, same as in stat_test.c.
> 
> I am not sure how elegant this is, but that's the easy fix.

Ah, I was using master as the base. Sorry about that...

> > +               fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
> > +               return EXIT_FAILURE;
> > +       }
> > +       if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
> > +               fprintf(stderr, "%s: no STATX_MNT_ID in stx_mask\n", fname);
> > +               return EXIT_FAILURE;
> > +       }
> > +       statx_mntid_short = statxbuf.stx_mnt_id;
> > +
> > +       if (!skip_mntid_unique) {
> > +               if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) {
> > +                       fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname);
> > +                       return EXIT_FAILURE;
> > +               }
> > +               /*
> > +                * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
> > +                * kernel doesn't give us a unique mount ID just skip it.
> > +                */
> > +               if ((skip_mntid_unique |= !(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)))
> > +                       printf("statx(STATX_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n");
> 
> This verbose print breaks all existing "exportfs" tests which do not
> expect it in the golden output.
> 
> I understand that silently ignoring the failure is not good, but I also
> would like to add this test coverage to all the existing tests.
> 
> One solution is to resurrect the command line option -M from v1,
> but instead of meaning "test unique mount id" let it mean
> "do not allow to skip unique mount id" test.
> 
> Then you can add a new test that runs open_by_handle -M, but also
> implement a helper _require_unique_mntid similar to _require_btime
> which is needed for the new test to run only on new kernels.
> 
> I'm sorry for this complication, but fstest is a testsuite that runs on
> disto and stable kernels as well and we need to allow test coverage
> of new features along with stability of the test env.

No worries, I'll write it up. I'm not familiar with the exact
requirements of xfstests, sorry for the noise! (^_^")

> 
> Thanks,
> Amir.

-- 
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: [PATCH xfstests v2 2/2] open_by_handle: add tests for u64 mount ID

[Aleksa Sarai]

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

On 2024-09-03, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, Sep 3, 2024 at 8:41 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
> >
> > On 2024-09-02, Amir Goldstein <amir73il@gmail.com> wrote:
> > > On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> > > >
> > > > Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
> > > > make sure they match properly as part of the regular open_by_handle
> > > > tests.
> > > >
> > > > Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
> > > > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > > > ---
> > > > v2:
> > > > - Remove -M argument and always do the mount ID tests. [Amir Goldstein]
> > > > - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
> > > >   or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
> > > > - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
> > >
> > > Looks good.
> > >
> > > You may add:
> > >
> > > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > >
> > > It'd be nice to get a verification that this is indeed tested on the latest
> > > upstream and does not regress the tests that run the open_by_handle program.
> >
> > I've tested that the fallback works on mainline and correctly does the
> > test on patched kernels (by running open_by_handle directly) but I
> > haven't run the suite yet (still getting my mkosi testing setup working
> > to run fstests...).
> 
> I am afraid this has to be tested.
> I started testing myself and found that it breaks existing tests.
> Even if you make the test completely opt-in as in v1 it need to be
> tested and _notrun on old kernels.
> 
> If you have a new version, I can test it until you get your fstests setup
> ready, because anyway I would want to check that your test also
> works with overlayfs which has some specialized exportfs tests.
> Test by running ./check -overlay -g exportfs, but I can also do that for you.

I managed to get fstests running, sorry about that...

For the v3 I have ready (which includes a new test using -M), the
following runs work in my VM:

 - ./check -g exportfs
 - ./check -overlay -g exportfs

Should I check anything else before sending it?

Also, when running the tests I think I may have found a bug? Using
overlayfs+xfs leads to the following error when doing ./check -overlay
if the scratch device is XFS:

  ./common/rc: line 299: _xfs_has_feature: command not found
    not run: upper fs needs to support d_type

The fix I applied was simply:

diff --git a/common/rc b/common/rc
index 0beaf2ff1126..e6af1b16918f 100644
--- a/common/rc
+++ b/common/rc
@@ -296,6 +296,7 @@ _supports_filetype()
 	local fstyp=`$DF_PROG $dir | tail -1 | $AWK_PROG '{print $2}'`
 	case "$fstyp" in
 	xfs)
+		. common/xfs
 		_xfs_has_feature $dir ftype
 		;;
 	ext2|ext3|ext4)

Should I include this patch as well, or did I make a mistake somewhere?
(I could add the import to the top instead if you'd prefer that.)

Thanks.

> 
> Thanks,
> Amir.

-- 
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: [PATCH xfstests v2 2/2] open_by_handle: add tests for u64 mount ID

[Amir Goldstein]

On Wed, Sep 4, 2024 at 6:31 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> On 2024-09-03, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Tue, Sep 3, 2024 at 8:41 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
> > >
> > > On 2024-09-02, Amir Goldstein <amir73il@gmail.com> wrote:
> > > > On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> > > > >
> > > > > Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
> > > > > make sure they match properly as part of the regular open_by_handle
> > > > > tests.
> > > > >
> > > > > Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
> > > > > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > > > > ---
> > > > > v2:
> > > > > - Remove -M argument and always do the mount ID tests. [Amir Goldstein]
> > > > > - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
> > > > >   or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
> > > > > - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
> > > >
> > > > Looks good.
> > > >
> > > > You may add:
> > > >
> > > > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > > >
> > > > It'd be nice to get a verification that this is indeed tested on the latest
> > > > upstream and does not regress the tests that run the open_by_handle program.
> > >
> > > I've tested that the fallback works on mainline and correctly does the
> > > test on patched kernels (by running open_by_handle directly) but I
> > > haven't run the suite yet (still getting my mkosi testing setup working
> > > to run fstests...).
> >
> > I am afraid this has to be tested.
> > I started testing myself and found that it breaks existing tests.
> > Even if you make the test completely opt-in as in v1 it need to be
> > tested and _notrun on old kernels.
> >
> > If you have a new version, I can test it until you get your fstests setup
> > ready, because anyway I would want to check that your test also
> > works with overlayfs which has some specialized exportfs tests.
> > Test by running ./check -overlay -g exportfs, but I can also do that for you.
>
> I managed to get fstests running, sorry about that...
>
> For the v3 I have ready (which includes a new test using -M), the
> following runs work in my VM:
>
>  - ./check -g exportfs
>  - ./check -overlay -g exportfs
>
> Should I check anything else before sending it?
>

That should be enough.
So you have one new test that does not run on upstream kernel
and runs and passes on patched kernel?

> Also, when running the tests I think I may have found a bug? Using
> overlayfs+xfs leads to the following error when doing ./check -overlay
> if the scratch device is XFS:
>
>   ./common/rc: line 299: _xfs_has_feature: command not found
>     not run: upper fs needs to support d_type
>
> The fix I applied was simply:
>
> diff --git a/common/rc b/common/rc
> index 0beaf2ff1126..e6af1b16918f 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -296,6 +296,7 @@ _supports_filetype()
>         local fstyp=`$DF_PROG $dir | tail -1 | $AWK_PROG '{print $2}'`
>         case "$fstyp" in
>         xfs)
> +               . common/xfs
>                 _xfs_has_feature $dir ftype
>                 ;;
>         ext2|ext3|ext4)
>
> Should I include this patch as well, or did I make a mistake somewhere?
> (I could add the import to the top instead if you'd prefer that.)

This should already be handled by
if [ -n "$OVL_BASE_FSTYP" ];then
        _source_specific_fs $OVL_BASE_FSTYP
fi

in common/overlay

I think what you are missing is to
export FSTYP=xfs
as README.overlay suggests.

It's true that ./check does not *require* defining FSTYP
and can auto detect the test filesystem, but for running -overlay
is it a requirement to define the base FSTYP.

Thanks,
Amir.

Re: [PATCH xfstests v2 2/2] open_by_handle: add tests for u64 mount ID

[Aleksa Sarai]

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

On 2024-09-04, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, Sep 4, 2024 at 6:31 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> >
> > On 2024-09-03, Amir Goldstein <amir73il@gmail.com> wrote:
> > > On Tue, Sep 3, 2024 at 8:41 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
> > > >
> > > > On 2024-09-02, Amir Goldstein <amir73il@gmail.com> wrote:
> > > > > On Mon, Sep 2, 2024 at 6:46 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> > > > > >
> > > > > > Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
> > > > > > make sure they match properly as part of the regular open_by_handle
> > > > > > tests.
> > > > > >
> > > > > > Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
> > > > > > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > > > > > ---
> > > > > > v2:
> > > > > > - Remove -M argument and always do the mount ID tests. [Amir Goldstein]
> > > > > > - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
> > > > > >   or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
> > > > > > - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
> > > > >
> > > > > Looks good.
> > > > >
> > > > > You may add:
> > > > >
> > > > > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > > > >
> > > > > It'd be nice to get a verification that this is indeed tested on the latest
> > > > > upstream and does not regress the tests that run the open_by_handle program.
> > > >
> > > > I've tested that the fallback works on mainline and correctly does the
> > > > test on patched kernels (by running open_by_handle directly) but I
> > > > haven't run the suite yet (still getting my mkosi testing setup working
> > > > to run fstests...).
> > >
> > > I am afraid this has to be tested.
> > > I started testing myself and found that it breaks existing tests.
> > > Even if you make the test completely opt-in as in v1 it need to be
> > > tested and _notrun on old kernels.
> > >
> > > If you have a new version, I can test it until you get your fstests setup
> > > ready, because anyway I would want to check that your test also
> > > works with overlayfs which has some specialized exportfs tests.
> > > Test by running ./check -overlay -g exportfs, but I can also do that for you.
> >
> > I managed to get fstests running, sorry about that...
> >
> > For the v3 I have ready (which includes a new test using -M), the
> > following runs work in my VM:
> >
> >  - ./check -g exportfs
> >  - ./check -overlay -g exportfs
> >
> > Should I check anything else before sending it?
> >
> 
> That should be enough.
> So you have one new test that does not run on upstream kernel
> and runs and passes on patched kernel?

Yes. Both of those suite runs succeed without issues on v6.6.49,
v6.11-rc6 and with the AT_HANDLE_MNT_ID_UNIQUE patches.

I also added skipping logic such that it _should_ work on pre-5.8
kernels (pre-STATX_MNT_ID) as well, but I can't test that at the moment
(mkosi-kernel fails to boot kernels that old it seems...).

I'll send it now.

> > Also, when running the tests I think I may have found a bug? Using
> > overlayfs+xfs leads to the following error when doing ./check -overlay
> > if the scratch device is XFS:
> >
> >   ./common/rc: line 299: _xfs_has_feature: command not found
> >     not run: upper fs needs to support d_type
> >
> > The fix I applied was simply:
> >
> > diff --git a/common/rc b/common/rc
> > index 0beaf2ff1126..e6af1b16918f 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -296,6 +296,7 @@ _supports_filetype()
> >         local fstyp=`$DF_PROG $dir | tail -1 | $AWK_PROG '{print $2}'`
> >         case "$fstyp" in
> >         xfs)
> > +               . common/xfs
> >                 _xfs_has_feature $dir ftype
> >                 ;;
> >         ext2|ext3|ext4)
> >
> > Should I include this patch as well, or did I make a mistake somewhere?
> > (I could add the import to the top instead if you'd prefer that.)
> 
> This should already be handled by
> if [ -n "$OVL_BASE_FSTYP" ];then
>         _source_specific_fs $OVL_BASE_FSTYP
> fi
> 
> in common/overlay
> 
> I think what you are missing is to
> export FSTYP=xfs
> as README.overlay suggests.
> 
> It's true that ./check does not *require* defining FSTYP
> and can auto detect the test filesystem, but for running -overlay
> is it a requirement to define the base FSTYP.

Ah okay, I missed that. That fixed the issue, thanks!

> Thanks,
> Amir.

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

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

[PATCH xfstests v3 1/2] open_by_handle: verify u32 and u64 mount IDs

[Aleksa Sarai]

Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
make sure they match properly as part of the regular open_by_handle
tests. Also, add automatic tests for the old u32 mount IDs as well.

By default, we do mount ID checks but silently skip the tests if the
syscalls are not supported by the running kernel (to ensure the tests
continue to work for old kernels). We will add some tests explicitly
checking the new features (with no silent skipping) in a future patch.

The u32 mount ID tests require STATX_MNT_ID (Linux 5.8), while the u64
mount ID tests require STATX_MNT_ID_UNIQUE (Linux 6.9) and
AT_HANDLE_MNT_ID_UNIQUE (linux-next).

Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
Changed in v3:
- Make skipping completely silent in regular open_by_handle mode. [Amir Goldstein]
- Re-add -M to turn skipping into errors and add a new test that uses
  -M, but is skipped on older kernels. [Amir Goldstein]
- v2: <https://lore.kernel.org/all/20240902164554.928371-1-cyphar@cyphar.com/>
Changed in v2:
- Remove -M argument and always do the mount ID tests. [Amir Goldstein]
- Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
  or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
- v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
---
 src/open_by_handle.c | 132 +++++++++++++++++++++++++++++++++----------
 1 file changed, 103 insertions(+), 29 deletions(-)

diff --git a/src/open_by_handle.c b/src/open_by_handle.c
index 0f74ed08b1f0..920ec7d9170b 100644
--- a/src/open_by_handle.c
+++ b/src/open_by_handle.c
@@ -87,6 +87,15 @@ Examples:
 #include <errno.h>
 #include <linux/limits.h>
 #include <libgen.h>
+#include <stdint.h>
+#include <stdbool.h>
+
+#include <sys/stat.h>
+#include "statx.h"
+
+#ifndef AT_HANDLE_MNT_ID_UNIQUE
+#	define AT_HANDLE_MNT_ID_UNIQUE 0x001
+#endif
 
 #define MAXFILES 1024
 
@@ -108,6 +117,7 @@ void usage(void)
 	fprintf(stderr, "open_by_handle -a <test_dir> [N] - write data to test files after open by handle\n");
 	fprintf(stderr, "open_by_handle -l <test_dir> [N] - create hardlinks to test files, drop caches and try to open by handle\n");
 	fprintf(stderr, "open_by_handle -u <test_dir> [N] - unlink (hardlinked) test files, drop caches and try to open by handle\n");
+	fprintf(stderr, "open_by_handle -U <test_dir> [N] - verify the mount ID returned with AT_HANDLE_MNT_ID_UNIQUE is correct\n");
 	fprintf(stderr, "open_by_handle -d <test_dir> [N] - unlink test files and hardlinks, drop caches and try to open by handle\n");
 	fprintf(stderr, "open_by_handle -m <test_dir> [N] - rename test files, drop caches and try to open by handle\n");
 	fprintf(stderr, "open_by_handle -p <test_dir>     - create/delete and try to open by handle also test_dir itself\n");
@@ -118,6 +128,94 @@ void usage(void)
 	exit(EXIT_FAILURE);
 }
 
+static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
+				int bufsz)
+{
+	int ret;
+	int mntid_short;
+
+	static bool skip_mntid, skip_mntid_unique;
+
+	uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
+	struct statx statxbuf;
+
+	/* Get both the short and unique mount id. */
+	if (!skip_mntid) {
+		if (xfstests_statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) {
+			fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
+			return EXIT_FAILURE;
+		}
+		if (!(statxbuf.stx_mask & STATX_MNT_ID))
+			skip_mntid = true;
+		else
+			statx_mntid_short = statxbuf.stx_mnt_id;
+	}
+
+	if (!skip_mntid_unique) {
+		if (xfstests_statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) {
+			fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname);
+			return EXIT_FAILURE;
+		}
+		/*
+		 * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
+		 * kernel doesn't give us a unique mount ID just skip it.
+		 */
+		if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE))
+			skip_mntid_unique = true;
+		else
+			statx_mntid_unique = statxbuf.stx_mnt_id;
+	}
+
+	fh->handle_bytes = bufsz;
+	ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
+	if (bufsz < fh->handle_bytes) {
+		/* Query the filesystem required bufsz and the file handle */
+		if (ret != -1 || errno != EOVERFLOW) {
+			fprintf(stderr, "%s: unexpected result from name_to_handle_at: %d (%m)\n", fname, ret);
+			return EXIT_FAILURE;
+		}
+		ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
+	}
+	if (ret < 0) {
+		fprintf(stderr, "%s: name_to_handle: %m\n", fname);
+		return EXIT_FAILURE;
+	}
+
+	if (!skip_mntid) {
+		if (mntid_short != (int) statx_mntid_short) {
+			fprintf(stderr, "%s: name_to_handle_at returned a different mount ID to STATX_MNT_ID: %u != %lu\n", fname, mntid_short, statx_mntid_short);
+			return EXIT_FAILURE;
+		}
+	}
+
+	if (!skip_mntid_unique) {
+		struct handle dummy_fh;
+		uint64_t mntid_unique = 0;
+
+		/*
+		 * Get the unique mount ID. We don't need to get another copy of the
+		 * handle so store it in a dummy struct.
+		 */
+		dummy_fh.fh.handle_bytes = fh->handle_bytes;
+		ret = name_to_handle_at(AT_FDCWD, fname, &dummy_fh.fh, (int *) &mntid_unique, AT_HANDLE_MNT_ID_UNIQUE);
+		if (ret < 0) {
+			if (errno != EINVAL) {
+				fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE): %m\n", fname);
+				return EXIT_FAILURE;
+			}
+			/* EINVAL means AT_HANDLE_MNT_ID_UNIQUE is not supported */
+			skip_mntid_unique = true;
+		} else {
+			if (mntid_unique != statx_mntid_unique) {
+				fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) returned a different mount ID to STATX_MNT_ID_UNIQUE: %lu != %lu\n", fname, mntid_unique, statx_mntid_unique);
+				return EXIT_FAILURE;
+			}
+		}
+	}
+
+	return 0;
+}
+
 int main(int argc, char **argv)
 {
 	int	i, c;
@@ -130,7 +228,7 @@ int main(int argc, char **argv)
 	char	fname2[PATH_MAX];
 	char	*test_dir;
 	char	*mount_dir;
-	int	mount_fd, mount_id;
+	int	mount_fd;
 	char	*infile = NULL, *outfile = NULL;
 	int	in_fd = 0, out_fd = 0;
 	int	numfiles = 1;
@@ -305,21 +403,9 @@ int main(int argc, char **argv)
 				return EXIT_FAILURE;
 			}
 		} else {
-			handle[i].fh.handle_bytes = bufsz;
-			ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
-			if (bufsz < handle[i].fh.handle_bytes) {
-				/* Query the filesystem required bufsz and the file handle */
-				if (ret != -1 || errno != EOVERFLOW) {
-					fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", fname);
-					return EXIT_FAILURE;
-				}
-				ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
-			}
-			if (ret < 0) {
-				strcat(fname, ": name_to_handle");
-				perror(fname);
+			ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz);
+			if (ret)
 				return EXIT_FAILURE;
-			}
 		}
 		if (keepopen) {
 			/* Open without close to keep unlinked files around */
@@ -347,21 +433,9 @@ int main(int argc, char **argv)
 				return EXIT_FAILURE;
 			}
 		} else {
-			dir_handle.fh.handle_bytes = bufsz;
-			ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
-			if (bufsz < dir_handle.fh.handle_bytes) {
-				/* Query the filesystem required bufsz and the file handle */
-				if (ret != -1 || errno != EOVERFLOW) {
-					fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", dname);
-					return EXIT_FAILURE;
-				}
-				ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
-			}
-			if (ret < 0) {
-				strcat(dname, ": name_to_handle");
-				perror(dname);
+			ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz);
+			if (ret)
 				return EXIT_FAILURE;
-			}
 		}
 		if (out_fd) {
 			ret = write(out_fd, (char *)&dir_handle, sizeof(*handle));
-- 
2.46.0

[PATCH xfstests v3 2/2] generic/756: test name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) explicitly

[Aleksa Sarai]

In order to make sure we are actually testing AT_HANDLE_MNT_ID_UNIQUE,
add a test (based on generic/426) which runs the open_by_handle in a
mode where it will error out if there is a problem with getting mount
IDs. The test is skipped if the kernel doesn't support the necessary
features.

Suggested-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 common/rc             | 24 ++++++++++++++++
 src/open_by_handle.c  | 63 ++++++++++++++++++++++++++++++++++-------
 tests/generic/756     | 65 +++++++++++++++++++++++++++++++++++++++++++
 tests/generic/756.out |  5 ++++
 4 files changed, 147 insertions(+), 10 deletions(-)
 create mode 100755 tests/generic/756
 create mode 100644 tests/generic/756.out

diff --git a/common/rc b/common/rc
index 9da9fe188297..0beaf2ff1126 100644
--- a/common/rc
+++ b/common/rc
@@ -5178,6 +5178,30 @@ _require_fibmap()
 	rm -f $file
 }
 
+_require_statx_unique_mountid()
+{
+	# statx(STATX_MNT_ID=0x1000) was added in Linux 5.8.
+	# statx(STATX_MNT_ID_UNIQUE=0x4000) was added in Linux 6.9.
+	# We only need to check the latter.
+
+	export STATX_MNT_ID_UNIQUE=0x4000
+	local statx_mask=$(
+		${XFS_IO_PROG} -c "statx -m $STATX_MNT_ID_UNIQUE -r" "$TEST_DIR" |
+		sed -En 's/stat\.mask = (0x[0-9a-f]+)/\1/p'
+	)
+
+	[[ $(( statx_mask & STATX_MNT_ID_UNIQUE )) == $((STATX_MNT_ID_UNIQUE)) ]] ||
+		_notrun "statx does not support STATX_MNT_ID_UNIQUE on this kernel"
+}
+
+_require_open_by_handle_unique_mountid()
+{
+	_require_test_program "open_by_handle"
+
+	$here/src/open_by_handle -C AT_HANDLE_MNT_ID_UNIQUE 2>&1 \
+		|| _notrun "name_to_handle_at does not support AT_HANDLE_MNT_ID_UNIQUE"
+}
+
 _try_wipe_scratch_devs()
 {
 	test -x "$WIPEFS_PROG" || return 0
diff --git a/src/open_by_handle.c b/src/open_by_handle.c
index 920ec7d9170b..b5c1a30abbbc 100644
--- a/src/open_by_handle.c
+++ b/src/open_by_handle.c
@@ -106,9 +106,11 @@ struct handle {
 
 void usage(void)
 {
-	fprintf(stderr, "usage: open_by_handle [-cludmrwapknhs] [<-i|-o> <handles_file>] <test_dir> [num_files]\n");
+	fprintf(stderr, "usage: open_by_handle [-cludmMrwapknhs] [<-i|-o> <handles_file>] <test_dir> [num_files]\n");
+	fprintf(stderr, "       open_by_handle -C <feature>\n");
 	fprintf(stderr, "\n");
 	fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, try to get file handles and exit\n");
+	fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, try to get file handles and exit\n");
 	fprintf(stderr, "open_by_handle    <test_dir> [N] - get file handles of test files, drop caches and try to open by handle\n");
 	fprintf(stderr, "open_by_handle -n <test_dir> [N] - get file handles of test files and try to open by handle without drop caches\n");
 	fprintf(stderr, "open_by_handle -k <test_dir> [N] - get file handles of files that are kept open, drop caches and try to open by handle\n");
@@ -117,19 +119,23 @@ void usage(void)
 	fprintf(stderr, "open_by_handle -a <test_dir> [N] - write data to test files after open by handle\n");
 	fprintf(stderr, "open_by_handle -l <test_dir> [N] - create hardlinks to test files, drop caches and try to open by handle\n");
 	fprintf(stderr, "open_by_handle -u <test_dir> [N] - unlink (hardlinked) test files, drop caches and try to open by handle\n");
-	fprintf(stderr, "open_by_handle -U <test_dir> [N] - verify the mount ID returned with AT_HANDLE_MNT_ID_UNIQUE is correct\n");
 	fprintf(stderr, "open_by_handle -d <test_dir> [N] - unlink test files and hardlinks, drop caches and try to open by handle\n");
 	fprintf(stderr, "open_by_handle -m <test_dir> [N] - rename test files, drop caches and try to open by handle\n");
+	fprintf(stderr, "open_by_handle -M <test_dir> [N] - do not silently skip the mount ID verifications\n");
 	fprintf(stderr, "open_by_handle -p <test_dir>     - create/delete and try to open by handle also test_dir itself\n");
 	fprintf(stderr, "open_by_handle -i <handles_file> <test_dir> [N] - read test files handles from file and try to open by handle\n");
 	fprintf(stderr, "open_by_handle -o <handles_file> <test_dir> [N] - get file handles of test files and write handles to file\n");
 	fprintf(stderr, "open_by_handle -s <test_dir> [N] - wait in sleep loop after opening files by handle to keep them open\n");
 	fprintf(stderr, "open_by_handle -z <test_dir> [N] - query filesystem required buffer size\n");
+	fprintf(stderr, "\n");
+	fprintf(stderr, "open_by_handle -C <feature>      - check if <feature> is supported by the kernel.\n");
+	fprintf(stderr, "  <feature> can be any of the following values:\n");
+	fprintf(stderr, "  - AT_HANDLE_MNT_ID_UNIQUE\n");
 	exit(EXIT_FAILURE);
 }
 
 static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
-				int bufsz)
+				int bufsz, bool force_check_mountid)
 {
 	int ret;
 	int mntid_short;
@@ -145,10 +151,15 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
 			fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
 			return EXIT_FAILURE;
 		}
-		if (!(statxbuf.stx_mask & STATX_MNT_ID))
+		if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
+			if (force_check_mountid) {
+				fprintf(stderr, "%s: statx(STATX_MNT_ID) not supported by running kernel\n", fname);
+				return EXIT_FAILURE;
+			}
 			skip_mntid = true;
-		else
+		} else {
 			statx_mntid_short = statxbuf.stx_mnt_id;
+		}
 	}
 
 	if (!skip_mntid_unique) {
@@ -160,10 +171,15 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
 		 * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
 		 * kernel doesn't give us a unique mount ID just skip it.
 		 */
-		if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE))
+		if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)) {
+			if (force_check_mountid) {
+				fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE) not supported by running kernel\n", fname);
+				return EXIT_FAILURE;
+			}
 			skip_mntid_unique = true;
-		else
+		} else {
 			statx_mntid_unique = statxbuf.stx_mnt_id;
+		}
 	}
 
 	fh->handle_bytes = bufsz;
@@ -204,6 +220,10 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
 				return EXIT_FAILURE;
 			}
 			/* EINVAL means AT_HANDLE_MNT_ID_UNIQUE is not supported */
+			if (force_check_mountid) {
+				fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel\n", fname);
+				return EXIT_FAILURE;
+			}
 			skip_mntid_unique = true;
 		} else {
 			if (mntid_unique != statx_mntid_unique) {
@@ -216,6 +236,22 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
 	return 0;
 }
 
+static int check_feature(const char *feature)
+{
+	if (!strcmp(feature, "AT_HANDLE_MNT_ID_UNIQUE")) {
+		int ret = name_to_handle_at(AT_FDCWD, ".", NULL, NULL, AT_HANDLE_MNT_ID_UNIQUE);
+		/* If AT_HANDLE_MNT_ID_UNIQUE is supported, we get EFAULT. */
+		if (ret < 0 && errno == EINVAL) {
+			fprintf(stderr, "name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel\n");
+			return EXIT_FAILURE;
+		}
+		return 0;
+	}
+
+	fprintf(stderr, "unknown feature name '%s'\n", feature);
+	return EXIT_FAILURE;
+}
+
 int main(int argc, char **argv)
 {
 	int	i, c;
@@ -235,16 +271,20 @@ int main(int argc, char **argv)
 	int	create = 0, delete = 0, nlink = 1, move = 0;
 	int	rd = 0, wr = 0, wrafter = 0, parent = 0;
 	int	keepopen = 0, drop_caches = 1, sleep_loop = 0;
+	int force_check_mountid = 0;
 	int	bufsz = MAX_HANDLE_SZ;
 
 	if (argc < 2)
 		usage();
 
-	while ((c = getopt(argc, argv, "cludmrwapknhi:o:sz")) != -1) {
+	while ((c = getopt(argc, argv, "cC:ludmMrwapknhi:o:sz")) != -1) {
 		switch (c) {
 		case 'c':
 			create = 1;
 			break;
+		case 'C':
+			/* Check kernel feature support. */
+			return check_feature(optarg);
 		case 'w':
 			/* Write data before open_by_handle_at() */
 			wr = 1;
@@ -271,6 +311,9 @@ int main(int argc, char **argv)
 		case 'm':
 			move = 1;
 			break;
+		case 'M':
+			force_check_mountid = 1;
+			break;
 		case 'p':
 			parent = 1;
 			break;
@@ -403,7 +446,7 @@ int main(int argc, char **argv)
 				return EXIT_FAILURE;
 			}
 		} else {
-			ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz);
+			ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz, force_check_mountid);
 			if (ret)
 				return EXIT_FAILURE;
 		}
@@ -433,7 +476,7 @@ int main(int argc, char **argv)
 				return EXIT_FAILURE;
 			}
 		} else {
-			ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz);
+			ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz, force_check_mountid);
 			if (ret)
 				return EXIT_FAILURE;
 		}
diff --git a/tests/generic/756 b/tests/generic/756
new file mode 100755
index 000000000000..c7a82cfd25f4
--- /dev/null
+++ b/tests/generic/756
@@ -0,0 +1,65 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2017 CTERA Networks. All Rights Reserved.
+# Copyright (C) 2024 Aleksa Sarai <cyphar@cyphar.com>
+#
+# FS QA Test No. 756
+#
+# Check stale handles pointing to unlinked files and non-stale handles pointing
+# to linked files while verifying that u64 mount IDs are correctly returned.
+#
+. ./common/preamble
+_begin_fstest auto quick exportfs
+
+# Import common functions.
+. ./common/filter
+
+
+# Modify as appropriate.
+_require_test
+# _require_exportfs and  already requires open_by_handle, but let's not count on it
+_require_test_program "open_by_handle"
+_require_exportfs
+# We need both STATX_MNT_ID_UNIQUE and AT_HANDLE_MNT_ID_UNIQUE.
+_require_statx_unique_mountid
+_require_open_by_handle_unique_mountid
+
+NUMFILES=1024
+testdir=$TEST_DIR/$seq-dir
+mkdir -p $testdir
+
+# Create empty test files in test dir
+create_test_files()
+{
+	local dir=$1
+
+	mkdir -p $dir
+	rm -f $dir/*
+	$here/src/open_by_handle -c $dir $NUMFILES
+}
+
+# Test encode/decode file handles
+test_file_handles()
+{
+	local dir=$1
+	local opt=$2
+
+	echo test_file_handles $* | _filter_test_dir
+	$here/src/open_by_handle $opt $dir $NUMFILES
+}
+
+# Check stale handles to deleted files
+create_test_files $testdir
+test_file_handles $testdir -Md
+
+# Check non-stale handles to linked files
+create_test_files $testdir
+test_file_handles $testdir -M
+
+# Check non-stale handles to files that were hardlinked and original deleted
+create_test_files $testdir
+test_file_handles $testdir -Ml
+test_file_handles $testdir -Mu
+
+status=0
+exit
diff --git a/tests/generic/756.out b/tests/generic/756.out
new file mode 100644
index 000000000000..48aed88d87b9
--- /dev/null
+++ b/tests/generic/756.out
@@ -0,0 +1,5 @@
+QA output created by 756
+test_file_handles TEST_DIR/756-dir -Md
+test_file_handles TEST_DIR/756-dir -M
+test_file_handles TEST_DIR/756-dir -Ml
+test_file_handles TEST_DIR/756-dir -Mu
-- 
2.46.0

Re: [PATCH xfstests v3 1/2] open_by_handle: verify u32 and u64 mount IDs

[Amir Goldstein]

On Wed, Sep 4, 2024 at 7:57 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
> make sure they match properly as part of the regular open_by_handle
> tests. Also, add automatic tests for the old u32 mount IDs as well.
>
> By default, we do mount ID checks but silently skip the tests if the
> syscalls are not supported by the running kernel (to ensure the tests
> continue to work for old kernels). We will add some tests explicitly
> checking the new features (with no silent skipping) in a future patch.
>
> The u32 mount ID tests require STATX_MNT_ID (Linux 5.8), while the u64
> mount ID tests require STATX_MNT_ID_UNIQUE (Linux 6.9) and
> AT_HANDLE_MNT_ID_UNIQUE (linux-next).
>
> Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
> Changed in v3:
> - Make skipping completely silent in regular open_by_handle mode. [Amir Goldstein]
> - Re-add -M to turn skipping into errors and add a new test that uses
>   -M, but is skipped on older kernels. [Amir Goldstein]
> - v2: <https://lore.kernel.org/all/20240902164554.928371-1-cyphar@cyphar.com/>
> Changed in v2:
> - Remove -M argument and always do the mount ID tests. [Amir Goldstein]
> - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
>   or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
> - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
> ---
>  src/open_by_handle.c | 132 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 103 insertions(+), 29 deletions(-)
>
> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> index 0f74ed08b1f0..920ec7d9170b 100644
> --- a/src/open_by_handle.c
> +++ b/src/open_by_handle.c
> @@ -87,6 +87,15 @@ Examples:
>  #include <errno.h>
>  #include <linux/limits.h>
>  #include <libgen.h>
> +#include <stdint.h>
> +#include <stdbool.h>
> +
> +#include <sys/stat.h>
> +#include "statx.h"
> +
> +#ifndef AT_HANDLE_MNT_ID_UNIQUE
> +#      define AT_HANDLE_MNT_ID_UNIQUE 0x001
> +#endif
>
>  #define MAXFILES 1024
>
> @@ -108,6 +117,7 @@ void usage(void)
>         fprintf(stderr, "open_by_handle -a <test_dir> [N] - write data to test files after open by handle\n");
>         fprintf(stderr, "open_by_handle -l <test_dir> [N] - create hardlinks to test files, drop caches and try to open by handle\n");
>         fprintf(stderr, "open_by_handle -u <test_dir> [N] - unlink (hardlinked) test files, drop caches and try to open by handle\n");
> +       fprintf(stderr, "open_by_handle -U <test_dir> [N] - verify the mount ID returned with AT_HANDLE_MNT_ID_UNIQUE is correct\n");
>         fprintf(stderr, "open_by_handle -d <test_dir> [N] - unlink test files and hardlinks, drop caches and try to open by handle\n");
>         fprintf(stderr, "open_by_handle -m <test_dir> [N] - rename test files, drop caches and try to open by handle\n");
>         fprintf(stderr, "open_by_handle -p <test_dir>     - create/delete and try to open by handle also test_dir itself\n");
> @@ -118,6 +128,94 @@ void usage(void)
>         exit(EXIT_FAILURE);
>  }
>
> +static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
> +                               int bufsz)
> +{
> +       int ret;
> +       int mntid_short;
> +
> +       static bool skip_mntid, skip_mntid_unique;
> +
> +       uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
> +       struct statx statxbuf;
> +
> +       /* Get both the short and unique mount id. */
> +       if (!skip_mntid) {
> +               if (xfstests_statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) {
> +                       fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
> +                       return EXIT_FAILURE;
> +               }
> +               if (!(statxbuf.stx_mask & STATX_MNT_ID))
> +                       skip_mntid = true;
> +               else
> +                       statx_mntid_short = statxbuf.stx_mnt_id;
> +       }
> +
> +       if (!skip_mntid_unique) {
> +               if (xfstests_statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) {
> +                       fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname);
> +                       return EXIT_FAILURE;
> +               }
> +               /*
> +                * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
> +                * kernel doesn't give us a unique mount ID just skip it.
> +                */
> +               if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE))
> +                       skip_mntid_unique = true;
> +               else
> +                       statx_mntid_unique = statxbuf.stx_mnt_id;
> +       }
> +
> +       fh->handle_bytes = bufsz;
> +       ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
> +       if (bufsz < fh->handle_bytes) {
> +               /* Query the filesystem required bufsz and the file handle */
> +               if (ret != -1 || errno != EOVERFLOW) {
> +                       fprintf(stderr, "%s: unexpected result from name_to_handle_at: %d (%m)\n", fname, ret);
> +                       return EXIT_FAILURE;
> +               }
> +               ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
> +       }
> +       if (ret < 0) {
> +               fprintf(stderr, "%s: name_to_handle: %m\n", fname);
> +               return EXIT_FAILURE;
> +       }
> +
> +       if (!skip_mntid) {
> +               if (mntid_short != (int) statx_mntid_short) {
> +                       fprintf(stderr, "%s: name_to_handle_at returned a different mount ID to STATX_MNT_ID: %u != %lu\n", fname, mntid_short, statx_mntid_short);
> +                       return EXIT_FAILURE;
> +               }
> +       }
> +
> +       if (!skip_mntid_unique) {
> +               struct handle dummy_fh;
> +               uint64_t mntid_unique = 0;
> +
> +               /*
> +                * Get the unique mount ID. We don't need to get another copy of the
> +                * handle so store it in a dummy struct.
> +                */
> +               dummy_fh.fh.handle_bytes = fh->handle_bytes;
> +               ret = name_to_handle_at(AT_FDCWD, fname, &dummy_fh.fh, (int *) &mntid_unique, AT_HANDLE_MNT_ID_UNIQUE);
> +               if (ret < 0) {
> +                       if (errno != EINVAL) {
> +                               fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE): %m\n", fname);
> +                               return EXIT_FAILURE;
> +                       }
> +                       /* EINVAL means AT_HANDLE_MNT_ID_UNIQUE is not supported */
> +                       skip_mntid_unique = true;
> +               } else {
> +                       if (mntid_unique != statx_mntid_unique) {
> +                               fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) returned a different mount ID to STATX_MNT_ID_UNIQUE: %lu != %lu\n", fname, mntid_unique, statx_mntid_unique);
> +                               return EXIT_FAILURE;
> +                       }
> +               }
> +       }
> +
> +       return 0;
> +}
> +
>  int main(int argc, char **argv)
>  {
>         int     i, c;
> @@ -130,7 +228,7 @@ int main(int argc, char **argv)
>         char    fname2[PATH_MAX];
>         char    *test_dir;
>         char    *mount_dir;
> -       int     mount_fd, mount_id;
> +       int     mount_fd;
>         char    *infile = NULL, *outfile = NULL;
>         int     in_fd = 0, out_fd = 0;
>         int     numfiles = 1;
> @@ -305,21 +403,9 @@ int main(int argc, char **argv)
>                                 return EXIT_FAILURE;
>                         }
>                 } else {
> -                       handle[i].fh.handle_bytes = bufsz;
> -                       ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
> -                       if (bufsz < handle[i].fh.handle_bytes) {
> -                               /* Query the filesystem required bufsz and the file handle */
> -                               if (ret != -1 || errno != EOVERFLOW) {
> -                                       fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", fname);
> -                                       return EXIT_FAILURE;
> -                               }
> -                               ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
> -                       }
> -                       if (ret < 0) {
> -                               strcat(fname, ": name_to_handle");
> -                               perror(fname);
> +                       ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz);
> +                       if (ret)
>                                 return EXIT_FAILURE;
> -                       }
>                 }
>                 if (keepopen) {
>                         /* Open without close to keep unlinked files around */
> @@ -347,21 +433,9 @@ int main(int argc, char **argv)
>                                 return EXIT_FAILURE;
>                         }
>                 } else {
> -                       dir_handle.fh.handle_bytes = bufsz;
> -                       ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
> -                       if (bufsz < dir_handle.fh.handle_bytes) {
> -                               /* Query the filesystem required bufsz and the file handle */
> -                               if (ret != -1 || errno != EOVERFLOW) {
> -                                       fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", dname);
> -                                       return EXIT_FAILURE;
> -                               }
> -                               ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
> -                       }
> -                       if (ret < 0) {
> -                               strcat(dname, ": name_to_handle");
> -                               perror(dname);
> +                       ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz);
> +                       if (ret)
>                                 return EXIT_FAILURE;
> -                       }
>                 }
>                 if (out_fd) {
>                         ret = write(out_fd, (char *)&dir_handle, sizeof(*handle));
> --
> 2.46.0
>

Re: [PATCH xfstests v3 2/2] generic/756: test name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) explicitly

[Amir Goldstein]

On Wed, Sep 4, 2024 at 7:57 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> In order to make sure we are actually testing AT_HANDLE_MNT_ID_UNIQUE,
> add a test (based on generic/426) which runs the open_by_handle in a
> mode where it will error out if there is a problem with getting mount
> IDs. The test is skipped if the kernel doesn't support the necessary
> features.
>
> Suggested-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

Apart from one minor nits below, you may add:

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  common/rc             | 24 ++++++++++++++++
>  src/open_by_handle.c  | 63 ++++++++++++++++++++++++++++++++++-------
>  tests/generic/756     | 65 +++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/756.out |  5 ++++
>  4 files changed, 147 insertions(+), 10 deletions(-)
>  create mode 100755 tests/generic/756
>  create mode 100644 tests/generic/756.out
>
> diff --git a/common/rc b/common/rc
> index 9da9fe188297..0beaf2ff1126 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -5178,6 +5178,30 @@ _require_fibmap()
>         rm -f $file
>  }
>
> +_require_statx_unique_mountid()
> +{
> +       # statx(STATX_MNT_ID=0x1000) was added in Linux 5.8.
> +       # statx(STATX_MNT_ID_UNIQUE=0x4000) was added in Linux 6.9.
> +       # We only need to check the latter.
> +
> +       export STATX_MNT_ID_UNIQUE=0x4000
> +       local statx_mask=$(
> +               ${XFS_IO_PROG} -c "statx -m $STATX_MNT_ID_UNIQUE -r" "$TEST_DIR" |
> +               sed -En 's/stat\.mask = (0x[0-9a-f]+)/\1/p'
> +       )
> +
> +       [[ $(( statx_mask & STATX_MNT_ID_UNIQUE )) == $((STATX_MNT_ID_UNIQUE)) ]] ||
> +               _notrun "statx does not support STATX_MNT_ID_UNIQUE on this kernel"
> +}
> +
> +_require_open_by_handle_unique_mountid()
> +{
> +       _require_test_program "open_by_handle"
> +
> +       $here/src/open_by_handle -C AT_HANDLE_MNT_ID_UNIQUE 2>&1 \
> +               || _notrun "name_to_handle_at does not support AT_HANDLE_MNT_ID_UNIQUE"
> +}
> +
>  _try_wipe_scratch_devs()
>  {
>         test -x "$WIPEFS_PROG" || return 0
> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> index 920ec7d9170b..b5c1a30abbbc 100644
> --- a/src/open_by_handle.c
> +++ b/src/open_by_handle.c
> @@ -106,9 +106,11 @@ struct handle {
>
>  void usage(void)
>  {
> -       fprintf(stderr, "usage: open_by_handle [-cludmrwapknhs] [<-i|-o> <handles_file>] <test_dir> [num_files]\n");
> +       fprintf(stderr, "usage: open_by_handle [-cludmMrwapknhs] [<-i|-o> <handles_file>] <test_dir> [num_files]\n");
> +       fprintf(stderr, "       open_by_handle -C <feature>\n");
>         fprintf(stderr, "\n");
>         fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, try to get file handles and exit\n");
> +       fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, try to get file handles and exit\n");
>         fprintf(stderr, "open_by_handle    <test_dir> [N] - get file handles of test files, drop caches and try to open by handle\n");
>         fprintf(stderr, "open_by_handle -n <test_dir> [N] - get file handles of test files and try to open by handle without drop caches\n");
>         fprintf(stderr, "open_by_handle -k <test_dir> [N] - get file handles of files that are kept open, drop caches and try to open by handle\n");
> @@ -117,19 +119,23 @@ void usage(void)
>         fprintf(stderr, "open_by_handle -a <test_dir> [N] - write data to test files after open by handle\n");
>         fprintf(stderr, "open_by_handle -l <test_dir> [N] - create hardlinks to test files, drop caches and try to open by handle\n");
>         fprintf(stderr, "open_by_handle -u <test_dir> [N] - unlink (hardlinked) test files, drop caches and try to open by handle\n");
> -       fprintf(stderr, "open_by_handle -U <test_dir> [N] - verify the mount ID returned with AT_HANDLE_MNT_ID_UNIQUE is correct\n");

I guess this was not supposed to be in the first patch

>         fprintf(stderr, "open_by_handle -d <test_dir> [N] - unlink test files and hardlinks, drop caches and try to open by handle\n");
>         fprintf(stderr, "open_by_handle -m <test_dir> [N] - rename test files, drop caches and try to open by handle\n");
> +       fprintf(stderr, "open_by_handle -M <test_dir> [N] - do not silently skip the mount ID verifications\n");
>         fprintf(stderr, "open_by_handle -p <test_dir>     - create/delete and try to open by handle also test_dir itself\n");
>         fprintf(stderr, "open_by_handle -i <handles_file> <test_dir> [N] - read test files handles from file and try to open by handle\n");
>         fprintf(stderr, "open_by_handle -o <handles_file> <test_dir> [N] - get file handles of test files and write handles to file\n");
>         fprintf(stderr, "open_by_handle -s <test_dir> [N] - wait in sleep loop after opening files by handle to keep them open\n");
>         fprintf(stderr, "open_by_handle -z <test_dir> [N] - query filesystem required buffer size\n");
> +       fprintf(stderr, "\n");
> +       fprintf(stderr, "open_by_handle -C <feature>      - check if <feature> is supported by the kernel.\n");
> +       fprintf(stderr, "  <feature> can be any of the following values:\n");
> +       fprintf(stderr, "  - AT_HANDLE_MNT_ID_UNIQUE\n");
>         exit(EXIT_FAILURE);
>  }
>
>  static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
> -                               int bufsz)
> +                               int bufsz, bool force_check_mountid)
>  {
>         int ret;
>         int mntid_short;
> @@ -145,10 +151,15 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
>                         fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
>                         return EXIT_FAILURE;
>                 }
> -               if (!(statxbuf.stx_mask & STATX_MNT_ID))
> +               if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
> +                       if (force_check_mountid) {
> +                               fprintf(stderr, "%s: statx(STATX_MNT_ID) not supported by running kernel\n", fname);
> +                               return EXIT_FAILURE;
> +                       }
>                         skip_mntid = true;
> -               else
> +               } else {
>                         statx_mntid_short = statxbuf.stx_mnt_id;
> +               }
>         }
>
>         if (!skip_mntid_unique) {
> @@ -160,10 +171,15 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
>                  * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
>                  * kernel doesn't give us a unique mount ID just skip it.
>                  */
> -               if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE))
> +               if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)) {
> +                       if (force_check_mountid) {
> +                               fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE) not supported by running kernel\n", fname);
> +                               return EXIT_FAILURE;
> +                       }
>                         skip_mntid_unique = true;
> -               else
> +               } else {
>                         statx_mntid_unique = statxbuf.stx_mnt_id;
> +               }
>         }
>
>         fh->handle_bytes = bufsz;
> @@ -204,6 +220,10 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
>                                 return EXIT_FAILURE;
>                         }
>                         /* EINVAL means AT_HANDLE_MNT_ID_UNIQUE is not supported */
> +                       if (force_check_mountid) {
> +                               fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel\n", fname);
> +                               return EXIT_FAILURE;
> +                       }
>                         skip_mntid_unique = true;
>                 } else {
>                         if (mntid_unique != statx_mntid_unique) {
> @@ -216,6 +236,22 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
>         return 0;
>  }
>
> +static int check_feature(const char *feature)
> +{
> +       if (!strcmp(feature, "AT_HANDLE_MNT_ID_UNIQUE")) {
> +               int ret = name_to_handle_at(AT_FDCWD, ".", NULL, NULL, AT_HANDLE_MNT_ID_UNIQUE);
> +               /* If AT_HANDLE_MNT_ID_UNIQUE is supported, we get EFAULT. */
> +               if (ret < 0 && errno == EINVAL) {
> +                       fprintf(stderr, "name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel\n");
> +                       return EXIT_FAILURE;
> +               }
> +               return 0;
> +       }
> +
> +       fprintf(stderr, "unknown feature name '%s'\n", feature);
> +       return EXIT_FAILURE;
> +}
> +
>  int main(int argc, char **argv)
>  {
>         int     i, c;
> @@ -235,16 +271,20 @@ int main(int argc, char **argv)
>         int     create = 0, delete = 0, nlink = 1, move = 0;
>         int     rd = 0, wr = 0, wrafter = 0, parent = 0;
>         int     keepopen = 0, drop_caches = 1, sleep_loop = 0;
> +       int force_check_mountid = 0;
>         int     bufsz = MAX_HANDLE_SZ;
>
>         if (argc < 2)
>                 usage();
>
> -       while ((c = getopt(argc, argv, "cludmrwapknhi:o:sz")) != -1) {
> +       while ((c = getopt(argc, argv, "cC:ludmMrwapknhi:o:sz")) != -1) {
>                 switch (c) {
>                 case 'c':
>                         create = 1;
>                         break;
> +               case 'C':
> +                       /* Check kernel feature support. */
> +                       return check_feature(optarg);
>                 case 'w':
>                         /* Write data before open_by_handle_at() */
>                         wr = 1;
> @@ -271,6 +311,9 @@ int main(int argc, char **argv)
>                 case 'm':
>                         move = 1;
>                         break;
> +               case 'M':
> +                       force_check_mountid = 1;
> +                       break;
>                 case 'p':
>                         parent = 1;
>                         break;
> @@ -403,7 +446,7 @@ int main(int argc, char **argv)
>                                 return EXIT_FAILURE;
>                         }
>                 } else {
> -                       ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz);
> +                       ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz, force_check_mountid);
>                         if (ret)
>                                 return EXIT_FAILURE;
>                 }
> @@ -433,7 +476,7 @@ int main(int argc, char **argv)
>                                 return EXIT_FAILURE;
>                         }
>                 } else {
> -                       ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz);
> +                       ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz, force_check_mountid);
>                         if (ret)
>                                 return EXIT_FAILURE;
>                 }
> diff --git a/tests/generic/756 b/tests/generic/756
> new file mode 100755
> index 000000000000..c7a82cfd25f4
> --- /dev/null
> +++ b/tests/generic/756
> @@ -0,0 +1,65 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2017 CTERA Networks. All Rights Reserved.
> +# Copyright (C) 2024 Aleksa Sarai <cyphar@cyphar.com>
> +#
> +# FS QA Test No. 756
> +#
> +# Check stale handles pointing to unlinked files and non-stale handles pointing
> +# to linked files while verifying that u64 mount IDs are correctly returned.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick exportfs
> +
> +# Import common functions.
> +. ./common/filter
> +
> +
> +# Modify as appropriate.
> +_require_test
> +# _require_exportfs and  already requires open_by_handle, but let's not count on it
> +_require_test_program "open_by_handle"
> +_require_exportfs
> +# We need both STATX_MNT_ID_UNIQUE and AT_HANDLE_MNT_ID_UNIQUE.
> +_require_statx_unique_mountid
> +_require_open_by_handle_unique_mountid
> +
> +NUMFILES=1024
> +testdir=$TEST_DIR/$seq-dir
> +mkdir -p $testdir
> +
> +# Create empty test files in test dir
> +create_test_files()
> +{
> +       local dir=$1
> +
> +       mkdir -p $dir
> +       rm -f $dir/*
> +       $here/src/open_by_handle -c $dir $NUMFILES
> +}
> +
> +# Test encode/decode file handles
> +test_file_handles()
> +{
> +       local dir=$1
> +       local opt=$2
> +
> +       echo test_file_handles $* | _filter_test_dir
> +       $here/src/open_by_handle $opt $dir $NUMFILES
> +}
> +
> +# Check stale handles to deleted files
> +create_test_files $testdir
> +test_file_handles $testdir -Md
> +
> +# Check non-stale handles to linked files
> +create_test_files $testdir
> +test_file_handles $testdir -M
> +
> +# Check non-stale handles to files that were hardlinked and original deleted
> +create_test_files $testdir
> +test_file_handles $testdir -Ml
> +test_file_handles $testdir -Mu
> +
> +status=0
> +exit
> diff --git a/tests/generic/756.out b/tests/generic/756.out
> new file mode 100644
> index 000000000000..48aed88d87b9
> --- /dev/null
> +++ b/tests/generic/756.out
> @@ -0,0 +1,5 @@
> +QA output created by 756
> +test_file_handles TEST_DIR/756-dir -Md
> +test_file_handles TEST_DIR/756-dir -M
> +test_file_handles TEST_DIR/756-dir -Ml
> +test_file_handles TEST_DIR/756-dir -Mu
> --
> 2.46.0
>

Re: [PATCH xfstests v3 2/2] generic/756: test name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) explicitly

[Aleksa Sarai]

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

On 2024-09-04, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, Sep 4, 2024 at 7:57 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> >
> > In order to make sure we are actually testing AT_HANDLE_MNT_ID_UNIQUE,
> > add a test (based on generic/426) which runs the open_by_handle in a
> > mode where it will error out if there is a problem with getting mount
> > IDs. The test is skipped if the kernel doesn't support the necessary
> > features.
> >
> > Suggested-by: Amir Goldstein <amir73il@gmail.com>
> > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> 
> Apart from one minor nits below, you may add:
> 
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> 
> > ---
> >  common/rc             | 24 ++++++++++++++++
> >  src/open_by_handle.c  | 63 ++++++++++++++++++++++++++++++++++-------
> >  tests/generic/756     | 65 +++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/756.out |  5 ++++
> >  4 files changed, 147 insertions(+), 10 deletions(-)
> >  create mode 100755 tests/generic/756
> >  create mode 100644 tests/generic/756.out
> >
> > diff --git a/common/rc b/common/rc
> > index 9da9fe188297..0beaf2ff1126 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -5178,6 +5178,30 @@ _require_fibmap()
> >         rm -f $file
> >  }
> >
> > +_require_statx_unique_mountid()
> > +{
> > +       # statx(STATX_MNT_ID=0x1000) was added in Linux 5.8.
> > +       # statx(STATX_MNT_ID_UNIQUE=0x4000) was added in Linux 6.9.
> > +       # We only need to check the latter.
> > +
> > +       export STATX_MNT_ID_UNIQUE=0x4000
> > +       local statx_mask=$(
> > +               ${XFS_IO_PROG} -c "statx -m $STATX_MNT_ID_UNIQUE -r" "$TEST_DIR" |
> > +               sed -En 's/stat\.mask = (0x[0-9a-f]+)/\1/p'
> > +       )
> > +
> > +       [[ $(( statx_mask & STATX_MNT_ID_UNIQUE )) == $((STATX_MNT_ID_UNIQUE)) ]] ||
> > +               _notrun "statx does not support STATX_MNT_ID_UNIQUE on this kernel"
> > +}
> > +
> > +_require_open_by_handle_unique_mountid()
> > +{
> > +       _require_test_program "open_by_handle"
> > +
> > +       $here/src/open_by_handle -C AT_HANDLE_MNT_ID_UNIQUE 2>&1 \
> > +               || _notrun "name_to_handle_at does not support AT_HANDLE_MNT_ID_UNIQUE"
> > +}
> > +
> >  _try_wipe_scratch_devs()
> >  {
> >         test -x "$WIPEFS_PROG" || return 0
> > diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> > index 920ec7d9170b..b5c1a30abbbc 100644
> > --- a/src/open_by_handle.c
> > +++ b/src/open_by_handle.c
> > @@ -106,9 +106,11 @@ struct handle {
> >
> >  void usage(void)
> >  {
> > -       fprintf(stderr, "usage: open_by_handle [-cludmrwapknhs] [<-i|-o> <handles_file>] <test_dir> [num_files]\n");
> > +       fprintf(stderr, "usage: open_by_handle [-cludmMrwapknhs] [<-i|-o> <handles_file>] <test_dir> [num_files]\n");
> > +       fprintf(stderr, "       open_by_handle -C <feature>\n");
> >         fprintf(stderr, "\n");
> >         fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, try to get file handles and exit\n");
> > +       fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, try to get file handles and exit\n");
> >         fprintf(stderr, "open_by_handle    <test_dir> [N] - get file handles of test files, drop caches and try to open by handle\n");
> >         fprintf(stderr, "open_by_handle -n <test_dir> [N] - get file handles of test files and try to open by handle without drop caches\n");
> >         fprintf(stderr, "open_by_handle -k <test_dir> [N] - get file handles of files that are kept open, drop caches and try to open by handle\n");
> > @@ -117,19 +119,23 @@ void usage(void)
> >         fprintf(stderr, "open_by_handle -a <test_dir> [N] - write data to test files after open by handle\n");
> >         fprintf(stderr, "open_by_handle -l <test_dir> [N] - create hardlinks to test files, drop caches and try to open by handle\n");
> >         fprintf(stderr, "open_by_handle -u <test_dir> [N] - unlink (hardlinked) test files, drop caches and try to open by handle\n");
> > -       fprintf(stderr, "open_by_handle -U <test_dir> [N] - verify the mount ID returned with AT_HANDLE_MNT_ID_UNIQUE is correct\n");
> 
> I guess this was not supposed to be in the first patch

Yeah, it got mixed up when splitting the patch. I'll fix it up.

> >         fprintf(stderr, "open_by_handle -d <test_dir> [N] - unlink test files and hardlinks, drop caches and try to open by handle\n");
> >         fprintf(stderr, "open_by_handle -m <test_dir> [N] - rename test files, drop caches and try to open by handle\n");
> > +       fprintf(stderr, "open_by_handle -M <test_dir> [N] - do not silently skip the mount ID verifications\n");
> >         fprintf(stderr, "open_by_handle -p <test_dir>     - create/delete and try to open by handle also test_dir itself\n");
> >         fprintf(stderr, "open_by_handle -i <handles_file> <test_dir> [N] - read test files handles from file and try to open by handle\n");
> >         fprintf(stderr, "open_by_handle -o <handles_file> <test_dir> [N] - get file handles of test files and write handles to file\n");
> >         fprintf(stderr, "open_by_handle -s <test_dir> [N] - wait in sleep loop after opening files by handle to keep them open\n");
> >         fprintf(stderr, "open_by_handle -z <test_dir> [N] - query filesystem required buffer size\n");
> > +       fprintf(stderr, "\n");
> > +       fprintf(stderr, "open_by_handle -C <feature>      - check if <feature> is supported by the kernel.\n");
> > +       fprintf(stderr, "  <feature> can be any of the following values:\n");
> > +       fprintf(stderr, "  - AT_HANDLE_MNT_ID_UNIQUE\n");
> >         exit(EXIT_FAILURE);
> >  }
> >
> >  static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
> > -                               int bufsz)
> > +                               int bufsz, bool force_check_mountid)
> >  {
> >         int ret;
> >         int mntid_short;
> > @@ -145,10 +151,15 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
> >                         fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
> >                         return EXIT_FAILURE;
> >                 }
> > -               if (!(statxbuf.stx_mask & STATX_MNT_ID))
> > +               if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
> > +                       if (force_check_mountid) {
> > +                               fprintf(stderr, "%s: statx(STATX_MNT_ID) not supported by running kernel\n", fname);
> > +                               return EXIT_FAILURE;
> > +                       }
> >                         skip_mntid = true;
> > -               else
> > +               } else {
> >                         statx_mntid_short = statxbuf.stx_mnt_id;
> > +               }
> >         }
> >
> >         if (!skip_mntid_unique) {
> > @@ -160,10 +171,15 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
> >                  * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
> >                  * kernel doesn't give us a unique mount ID just skip it.
> >                  */
> > -               if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE))
> > +               if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)) {
> > +                       if (force_check_mountid) {
> > +                               fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE) not supported by running kernel\n", fname);
> > +                               return EXIT_FAILURE;
> > +                       }
> >                         skip_mntid_unique = true;
> > -               else
> > +               } else {
> >                         statx_mntid_unique = statxbuf.stx_mnt_id;
> > +               }
> >         }
> >
> >         fh->handle_bytes = bufsz;
> > @@ -204,6 +220,10 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
> >                                 return EXIT_FAILURE;
> >                         }
> >                         /* EINVAL means AT_HANDLE_MNT_ID_UNIQUE is not supported */
> > +                       if (force_check_mountid) {
> > +                               fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel\n", fname);
> > +                               return EXIT_FAILURE;
> > +                       }
> >                         skip_mntid_unique = true;
> >                 } else {
> >                         if (mntid_unique != statx_mntid_unique) {
> > @@ -216,6 +236,22 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
> >         return 0;
> >  }
> >
> > +static int check_feature(const char *feature)
> > +{
> > +       if (!strcmp(feature, "AT_HANDLE_MNT_ID_UNIQUE")) {
> > +               int ret = name_to_handle_at(AT_FDCWD, ".", NULL, NULL, AT_HANDLE_MNT_ID_UNIQUE);
> > +               /* If AT_HANDLE_MNT_ID_UNIQUE is supported, we get EFAULT. */
> > +               if (ret < 0 && errno == EINVAL) {
> > +                       fprintf(stderr, "name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel\n");
> > +                       return EXIT_FAILURE;
> > +               }
> > +               return 0;
> > +       }
> > +
> > +       fprintf(stderr, "unknown feature name '%s'\n", feature);
> > +       return EXIT_FAILURE;
> > +}
> > +
> >  int main(int argc, char **argv)
> >  {
> >         int     i, c;
> > @@ -235,16 +271,20 @@ int main(int argc, char **argv)
> >         int     create = 0, delete = 0, nlink = 1, move = 0;
> >         int     rd = 0, wr = 0, wrafter = 0, parent = 0;
> >         int     keepopen = 0, drop_caches = 1, sleep_loop = 0;
> > +       int force_check_mountid = 0;
> >         int     bufsz = MAX_HANDLE_SZ;
> >
> >         if (argc < 2)
> >                 usage();
> >
> > -       while ((c = getopt(argc, argv, "cludmrwapknhi:o:sz")) != -1) {
> > +       while ((c = getopt(argc, argv, "cC:ludmMrwapknhi:o:sz")) != -1) {
> >                 switch (c) {
> >                 case 'c':
> >                         create = 1;
> >                         break;
> > +               case 'C':
> > +                       /* Check kernel feature support. */
> > +                       return check_feature(optarg);
> >                 case 'w':
> >                         /* Write data before open_by_handle_at() */
> >                         wr = 1;
> > @@ -271,6 +311,9 @@ int main(int argc, char **argv)
> >                 case 'm':
> >                         move = 1;
> >                         break;
> > +               case 'M':
> > +                       force_check_mountid = 1;
> > +                       break;
> >                 case 'p':
> >                         parent = 1;
> >                         break;
> > @@ -403,7 +446,7 @@ int main(int argc, char **argv)
> >                                 return EXIT_FAILURE;
> >                         }
> >                 } else {
> > -                       ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz);
> > +                       ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz, force_check_mountid);
> >                         if (ret)
> >                                 return EXIT_FAILURE;
> >                 }
> > @@ -433,7 +476,7 @@ int main(int argc, char **argv)
> >                                 return EXIT_FAILURE;
> >                         }
> >                 } else {
> > -                       ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz);
> > +                       ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz, force_check_mountid);
> >                         if (ret)
> >                                 return EXIT_FAILURE;
> >                 }
> > diff --git a/tests/generic/756 b/tests/generic/756
> > new file mode 100755
> > index 000000000000..c7a82cfd25f4
> > --- /dev/null
> > +++ b/tests/generic/756
> > @@ -0,0 +1,65 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (C) 2017 CTERA Networks. All Rights Reserved.
> > +# Copyright (C) 2024 Aleksa Sarai <cyphar@cyphar.com>
> > +#
> > +# FS QA Test No. 756
> > +#
> > +# Check stale handles pointing to unlinked files and non-stale handles pointing
> > +# to linked files while verifying that u64 mount IDs are correctly returned.
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick exportfs
> > +
> > +# Import common functions.
> > +. ./common/filter
> > +
> > +
> > +# Modify as appropriate.
> > +_require_test
> > +# _require_exportfs and  already requires open_by_handle, but let's not count on it
> > +_require_test_program "open_by_handle"
> > +_require_exportfs
> > +# We need both STATX_MNT_ID_UNIQUE and AT_HANDLE_MNT_ID_UNIQUE.
> > +_require_statx_unique_mountid
> > +_require_open_by_handle_unique_mountid
> > +
> > +NUMFILES=1024
> > +testdir=$TEST_DIR/$seq-dir
> > +mkdir -p $testdir
> > +
> > +# Create empty test files in test dir
> > +create_test_files()
> > +{
> > +       local dir=$1
> > +
> > +       mkdir -p $dir
> > +       rm -f $dir/*
> > +       $here/src/open_by_handle -c $dir $NUMFILES
> > +}
> > +
> > +# Test encode/decode file handles
> > +test_file_handles()
> > +{
> > +       local dir=$1
> > +       local opt=$2
> > +
> > +       echo test_file_handles $* | _filter_test_dir
> > +       $here/src/open_by_handle $opt $dir $NUMFILES
> > +}
> > +
> > +# Check stale handles to deleted files
> > +create_test_files $testdir
> > +test_file_handles $testdir -Md
> > +
> > +# Check non-stale handles to linked files
> > +create_test_files $testdir
> > +test_file_handles $testdir -M
> > +
> > +# Check non-stale handles to files that were hardlinked and original deleted
> > +create_test_files $testdir
> > +test_file_handles $testdir -Ml
> > +test_file_handles $testdir -Mu
> > +
> > +status=0
> > +exit
> > diff --git a/tests/generic/756.out b/tests/generic/756.out
> > new file mode 100644
> > index 000000000000..48aed88d87b9
> > --- /dev/null
> > +++ b/tests/generic/756.out
> > @@ -0,0 +1,5 @@
> > +QA output created by 756
> > +test_file_handles TEST_DIR/756-dir -Md
> > +test_file_handles TEST_DIR/756-dir -M
> > +test_file_handles TEST_DIR/756-dir -Ml
> > +test_file_handles TEST_DIR/756-dir -Mu
> > --
> > 2.46.0
> >

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

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

[PATCH xfstests v4 1/2] open_by_handle: verify u32 and u64 mount IDs

[Aleksa Sarai]

Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
make sure they match properly as part of the regular open_by_handle
tests. Also, add automatic tests for the old u32 mount IDs as well.

By default, we do mount ID checks but silently skip the tests if the
syscalls are not supported by the running kernel (to ensure the tests
continue to work for old kernels). We will add some tests explicitly
checking the new features (with no silent skipping) in a future patch.

The u32 mount ID tests require STATX_MNT_ID (Linux 5.8), while the u64
mount ID tests require STATX_MNT_ID_UNIQUE (Linux 6.9) and
AT_HANDLE_MNT_ID_UNIQUE (linux-next).

Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
Changed in v4:
- Fix minor flub in patch split. [Amir Goldstein]
- v3: <https://lore.kernel.org/all/20240904175639.2269694-1-cyphar@cyphar.com/>
Changed in v3:
- Make skipping completely silent in regular open_by_handle mode. [Amir Goldstein]
- Re-add -M to turn skipping into errors and add a new test that uses
  -M, but is skipped on older kernels. [Amir Goldstein]
- v2: <https://lore.kernel.org/all/20240902164554.928371-1-cyphar@cyphar.com/>
Changed in v2:
- Remove -M argument and always do the mount ID tests. [Amir Goldstein]
- Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
  or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
- v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
---
 src/open_by_handle.c | 131 +++++++++++++++++++++++++++++++++----------
 1 file changed, 102 insertions(+), 29 deletions(-)

diff --git a/src/open_by_handle.c b/src/open_by_handle.c
index 0f74ed08b1f0..dcbcd35561fb 100644
--- a/src/open_by_handle.c
+++ b/src/open_by_handle.c
@@ -87,6 +87,15 @@ Examples:
 #include <errno.h>
 #include <linux/limits.h>
 #include <libgen.h>
+#include <stdint.h>
+#include <stdbool.h>
+
+#include <sys/stat.h>
+#include "statx.h"
+
+#ifndef AT_HANDLE_MNT_ID_UNIQUE
+#	define AT_HANDLE_MNT_ID_UNIQUE 0x001
+#endif
 
 #define MAXFILES 1024
 
@@ -118,6 +127,94 @@ void usage(void)
 	exit(EXIT_FAILURE);
 }
 
+static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
+				int bufsz)
+{
+	int ret;
+	int mntid_short;
+
+	static bool skip_mntid, skip_mntid_unique;
+
+	uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
+	struct statx statxbuf;
+
+	/* Get both the short and unique mount id. */
+	if (!skip_mntid) {
+		if (xfstests_statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) {
+			fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
+			return EXIT_FAILURE;
+		}
+		if (!(statxbuf.stx_mask & STATX_MNT_ID))
+			skip_mntid = true;
+		else
+			statx_mntid_short = statxbuf.stx_mnt_id;
+	}
+
+	if (!skip_mntid_unique) {
+		if (xfstests_statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) {
+			fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname);
+			return EXIT_FAILURE;
+		}
+		/*
+		 * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
+		 * kernel doesn't give us a unique mount ID just skip it.
+		 */
+		if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE))
+			skip_mntid_unique = true;
+		else
+			statx_mntid_unique = statxbuf.stx_mnt_id;
+	}
+
+	fh->handle_bytes = bufsz;
+	ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
+	if (bufsz < fh->handle_bytes) {
+		/* Query the filesystem required bufsz and the file handle */
+		if (ret != -1 || errno != EOVERFLOW) {
+			fprintf(stderr, "%s: unexpected result from name_to_handle_at: %d (%m)\n", fname, ret);
+			return EXIT_FAILURE;
+		}
+		ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
+	}
+	if (ret < 0) {
+		fprintf(stderr, "%s: name_to_handle: %m\n", fname);
+		return EXIT_FAILURE;
+	}
+
+	if (!skip_mntid) {
+		if (mntid_short != (int) statx_mntid_short) {
+			fprintf(stderr, "%s: name_to_handle_at returned a different mount ID to STATX_MNT_ID: %u != %lu\n", fname, mntid_short, statx_mntid_short);
+			return EXIT_FAILURE;
+		}
+	}
+
+	if (!skip_mntid_unique) {
+		struct handle dummy_fh;
+		uint64_t mntid_unique = 0;
+
+		/*
+		 * Get the unique mount ID. We don't need to get another copy of the
+		 * handle so store it in a dummy struct.
+		 */
+		dummy_fh.fh.handle_bytes = fh->handle_bytes;
+		ret = name_to_handle_at(AT_FDCWD, fname, &dummy_fh.fh, (int *) &mntid_unique, AT_HANDLE_MNT_ID_UNIQUE);
+		if (ret < 0) {
+			if (errno != EINVAL) {
+				fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE): %m\n", fname);
+				return EXIT_FAILURE;
+			}
+			/* EINVAL means AT_HANDLE_MNT_ID_UNIQUE is not supported */
+			skip_mntid_unique = true;
+		} else {
+			if (mntid_unique != statx_mntid_unique) {
+				fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) returned a different mount ID to STATX_MNT_ID_UNIQUE: %lu != %lu\n", fname, mntid_unique, statx_mntid_unique);
+				return EXIT_FAILURE;
+			}
+		}
+	}
+
+	return 0;
+}
+
 int main(int argc, char **argv)
 {
 	int	i, c;
@@ -130,7 +227,7 @@ int main(int argc, char **argv)
 	char	fname2[PATH_MAX];
 	char	*test_dir;
 	char	*mount_dir;
-	int	mount_fd, mount_id;
+	int	mount_fd;
 	char	*infile = NULL, *outfile = NULL;
 	int	in_fd = 0, out_fd = 0;
 	int	numfiles = 1;
@@ -305,21 +402,9 @@ int main(int argc, char **argv)
 				return EXIT_FAILURE;
 			}
 		} else {
-			handle[i].fh.handle_bytes = bufsz;
-			ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
-			if (bufsz < handle[i].fh.handle_bytes) {
-				/* Query the filesystem required bufsz and the file handle */
-				if (ret != -1 || errno != EOVERFLOW) {
-					fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", fname);
-					return EXIT_FAILURE;
-				}
-				ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
-			}
-			if (ret < 0) {
-				strcat(fname, ": name_to_handle");
-				perror(fname);
+			ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz);
+			if (ret)
 				return EXIT_FAILURE;
-			}
 		}
 		if (keepopen) {
 			/* Open without close to keep unlinked files around */
@@ -347,21 +432,9 @@ int main(int argc, char **argv)
 				return EXIT_FAILURE;
 			}
 		} else {
-			dir_handle.fh.handle_bytes = bufsz;
-			ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
-			if (bufsz < dir_handle.fh.handle_bytes) {
-				/* Query the filesystem required bufsz and the file handle */
-				if (ret != -1 || errno != EOVERFLOW) {
-					fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", dname);
-					return EXIT_FAILURE;
-				}
-				ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
-			}
-			if (ret < 0) {
-				strcat(dname, ": name_to_handle");
-				perror(dname);
+			ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz);
+			if (ret)
 				return EXIT_FAILURE;
-			}
 		}
 		if (out_fd) {
 			ret = write(out_fd, (char *)&dir_handle, sizeof(*handle));
-- 
2.46.0

[PATCH xfstests v4 2/2] generic/756: test name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) explicitly

[Aleksa Sarai]

In order to make sure we are actually testing AT_HANDLE_MNT_ID_UNIQUE,
add a test (based on generic/426) which runs the open_by_handle in a
mode where it will error out if there is a problem with getting mount
IDs. The test is skipped if the kernel doesn't support the necessary
features.

Suggested-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 common/rc             | 24 ++++++++++++++++
 src/open_by_handle.c  | 61 ++++++++++++++++++++++++++++++++++------
 tests/generic/756     | 65 +++++++++++++++++++++++++++++++++++++++++++
 tests/generic/756.out |  5 ++++
 4 files changed, 146 insertions(+), 9 deletions(-)
 create mode 100755 tests/generic/756
 create mode 100644 tests/generic/756.out

diff --git a/common/rc b/common/rc
index 9da9fe188297..0beaf2ff1126 100644
--- a/common/rc
+++ b/common/rc
@@ -5178,6 +5178,30 @@ _require_fibmap()
 	rm -f $file
 }
 
+_require_statx_unique_mountid()
+{
+	# statx(STATX_MNT_ID=0x1000) was added in Linux 5.8.
+	# statx(STATX_MNT_ID_UNIQUE=0x4000) was added in Linux 6.9.
+	# We only need to check the latter.
+
+	export STATX_MNT_ID_UNIQUE=0x4000
+	local statx_mask=$(
+		${XFS_IO_PROG} -c "statx -m $STATX_MNT_ID_UNIQUE -r" "$TEST_DIR" |
+		sed -En 's/stat\.mask = (0x[0-9a-f]+)/\1/p'
+	)
+
+	[[ $(( statx_mask & STATX_MNT_ID_UNIQUE )) == $((STATX_MNT_ID_UNIQUE)) ]] ||
+		_notrun "statx does not support STATX_MNT_ID_UNIQUE on this kernel"
+}
+
+_require_open_by_handle_unique_mountid()
+{
+	_require_test_program "open_by_handle"
+
+	$here/src/open_by_handle -C AT_HANDLE_MNT_ID_UNIQUE 2>&1 \
+		|| _notrun "name_to_handle_at does not support AT_HANDLE_MNT_ID_UNIQUE"
+}
+
 _try_wipe_scratch_devs()
 {
 	test -x "$WIPEFS_PROG" || return 0
diff --git a/src/open_by_handle.c b/src/open_by_handle.c
index dcbcd35561fb..a99cce4b3558 100644
--- a/src/open_by_handle.c
+++ b/src/open_by_handle.c
@@ -106,7 +106,8 @@ struct handle {
 
 void usage(void)
 {
-	fprintf(stderr, "usage: open_by_handle [-cludmrwapknhs] [<-i|-o> <handles_file>] <test_dir> [num_files]\n");
+	fprintf(stderr, "usage: open_by_handle [-cludmMrwapknhs] [<-i|-o> <handles_file>] <test_dir> [num_files]\n");
+	fprintf(stderr, "       open_by_handle -C <feature>\n");
 	fprintf(stderr, "\n");
 	fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, try to get file handles and exit\n");
 	fprintf(stderr, "open_by_handle    <test_dir> [N] - get file handles of test files, drop caches and try to open by handle\n");
@@ -119,16 +120,21 @@ void usage(void)
 	fprintf(stderr, "open_by_handle -u <test_dir> [N] - unlink (hardlinked) test files, drop caches and try to open by handle\n");
 	fprintf(stderr, "open_by_handle -d <test_dir> [N] - unlink test files and hardlinks, drop caches and try to open by handle\n");
 	fprintf(stderr, "open_by_handle -m <test_dir> [N] - rename test files, drop caches and try to open by handle\n");
+	fprintf(stderr, "open_by_handle -M <test_dir> [N] - do not silently skip the mount ID verifications\n");
 	fprintf(stderr, "open_by_handle -p <test_dir>     - create/delete and try to open by handle also test_dir itself\n");
 	fprintf(stderr, "open_by_handle -i <handles_file> <test_dir> [N] - read test files handles from file and try to open by handle\n");
 	fprintf(stderr, "open_by_handle -o <handles_file> <test_dir> [N] - get file handles of test files and write handles to file\n");
 	fprintf(stderr, "open_by_handle -s <test_dir> [N] - wait in sleep loop after opening files by handle to keep them open\n");
 	fprintf(stderr, "open_by_handle -z <test_dir> [N] - query filesystem required buffer size\n");
+	fprintf(stderr, "\n");
+	fprintf(stderr, "open_by_handle -C <feature>      - check if <feature> is supported by the kernel.\n");
+	fprintf(stderr, "  <feature> can be any of the following values:\n");
+	fprintf(stderr, "  - AT_HANDLE_MNT_ID_UNIQUE\n");
 	exit(EXIT_FAILURE);
 }
 
 static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
-				int bufsz)
+				int bufsz, bool force_check_mountid)
 {
 	int ret;
 	int mntid_short;
@@ -144,10 +150,15 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
 			fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
 			return EXIT_FAILURE;
 		}
-		if (!(statxbuf.stx_mask & STATX_MNT_ID))
+		if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
+			if (force_check_mountid) {
+				fprintf(stderr, "%s: statx(STATX_MNT_ID) not supported by running kernel\n", fname);
+				return EXIT_FAILURE;
+			}
 			skip_mntid = true;
-		else
+		} else {
 			statx_mntid_short = statxbuf.stx_mnt_id;
+		}
 	}
 
 	if (!skip_mntid_unique) {
@@ -159,10 +170,15 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
 		 * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
 		 * kernel doesn't give us a unique mount ID just skip it.
 		 */
-		if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE))
+		if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)) {
+			if (force_check_mountid) {
+				fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE) not supported by running kernel\n", fname);
+				return EXIT_FAILURE;
+			}
 			skip_mntid_unique = true;
-		else
+		} else {
 			statx_mntid_unique = statxbuf.stx_mnt_id;
+		}
 	}
 
 	fh->handle_bytes = bufsz;
@@ -203,6 +219,10 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
 				return EXIT_FAILURE;
 			}
 			/* EINVAL means AT_HANDLE_MNT_ID_UNIQUE is not supported */
+			if (force_check_mountid) {
+				fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel\n", fname);
+				return EXIT_FAILURE;
+			}
 			skip_mntid_unique = true;
 		} else {
 			if (mntid_unique != statx_mntid_unique) {
@@ -215,6 +235,22 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
 	return 0;
 }
 
+static int check_feature(const char *feature)
+{
+	if (!strcmp(feature, "AT_HANDLE_MNT_ID_UNIQUE")) {
+		int ret = name_to_handle_at(AT_FDCWD, ".", NULL, NULL, AT_HANDLE_MNT_ID_UNIQUE);
+		/* If AT_HANDLE_MNT_ID_UNIQUE is supported, we get EFAULT. */
+		if (ret < 0 && errno == EINVAL) {
+			fprintf(stderr, "name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel\n");
+			return EXIT_FAILURE;
+		}
+		return 0;
+	}
+
+	fprintf(stderr, "unknown feature name '%s'\n", feature);
+	return EXIT_FAILURE;
+}
+
 int main(int argc, char **argv)
 {
 	int	i, c;
@@ -234,16 +270,20 @@ int main(int argc, char **argv)
 	int	create = 0, delete = 0, nlink = 1, move = 0;
 	int	rd = 0, wr = 0, wrafter = 0, parent = 0;
 	int	keepopen = 0, drop_caches = 1, sleep_loop = 0;
+	int	force_check_mountid = 0;
 	int	bufsz = MAX_HANDLE_SZ;
 
 	if (argc < 2)
 		usage();
 
-	while ((c = getopt(argc, argv, "cludmrwapknhi:o:sz")) != -1) {
+	while ((c = getopt(argc, argv, "cC:ludmMrwapknhi:o:sz")) != -1) {
 		switch (c) {
 		case 'c':
 			create = 1;
 			break;
+		case 'C':
+			/* Check kernel feature support. */
+			return check_feature(optarg);
 		case 'w':
 			/* Write data before open_by_handle_at() */
 			wr = 1;
@@ -270,6 +310,9 @@ int main(int argc, char **argv)
 		case 'm':
 			move = 1;
 			break;
+		case 'M':
+			force_check_mountid = 1;
+			break;
 		case 'p':
 			parent = 1;
 			break;
@@ -402,7 +445,7 @@ int main(int argc, char **argv)
 				return EXIT_FAILURE;
 			}
 		} else {
-			ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz);
+			ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz, force_check_mountid);
 			if (ret)
 				return EXIT_FAILURE;
 		}
@@ -432,7 +475,7 @@ int main(int argc, char **argv)
 				return EXIT_FAILURE;
 			}
 		} else {
-			ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz);
+			ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz, force_check_mountid);
 			if (ret)
 				return EXIT_FAILURE;
 		}
diff --git a/tests/generic/756 b/tests/generic/756
new file mode 100755
index 000000000000..c7a82cfd25f4
--- /dev/null
+++ b/tests/generic/756
@@ -0,0 +1,65 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2017 CTERA Networks. All Rights Reserved.
+# Copyright (C) 2024 Aleksa Sarai <cyphar@cyphar.com>
+#
+# FS QA Test No. 756
+#
+# Check stale handles pointing to unlinked files and non-stale handles pointing
+# to linked files while verifying that u64 mount IDs are correctly returned.
+#
+. ./common/preamble
+_begin_fstest auto quick exportfs
+
+# Import common functions.
+. ./common/filter
+
+
+# Modify as appropriate.
+_require_test
+# _require_exportfs and  already requires open_by_handle, but let's not count on it
+_require_test_program "open_by_handle"
+_require_exportfs
+# We need both STATX_MNT_ID_UNIQUE and AT_HANDLE_MNT_ID_UNIQUE.
+_require_statx_unique_mountid
+_require_open_by_handle_unique_mountid
+
+NUMFILES=1024
+testdir=$TEST_DIR/$seq-dir
+mkdir -p $testdir
+
+# Create empty test files in test dir
+create_test_files()
+{
+	local dir=$1
+
+	mkdir -p $dir
+	rm -f $dir/*
+	$here/src/open_by_handle -c $dir $NUMFILES
+}
+
+# Test encode/decode file handles
+test_file_handles()
+{
+	local dir=$1
+	local opt=$2
+
+	echo test_file_handles $* | _filter_test_dir
+	$here/src/open_by_handle $opt $dir $NUMFILES
+}
+
+# Check stale handles to deleted files
+create_test_files $testdir
+test_file_handles $testdir -Md
+
+# Check non-stale handles to linked files
+create_test_files $testdir
+test_file_handles $testdir -M
+
+# Check non-stale handles to files that were hardlinked and original deleted
+create_test_files $testdir
+test_file_handles $testdir -Ml
+test_file_handles $testdir -Mu
+
+status=0
+exit
diff --git a/tests/generic/756.out b/tests/generic/756.out
new file mode 100644
index 000000000000..48aed88d87b9
--- /dev/null
+++ b/tests/generic/756.out
@@ -0,0 +1,5 @@
+QA output created by 756
+test_file_handles TEST_DIR/756-dir -Md
+test_file_handles TEST_DIR/756-dir -M
+test_file_handles TEST_DIR/756-dir -Ml
+test_file_handles TEST_DIR/756-dir -Mu
-- 
2.46.0