Now that we provide a unique 64-bit mount ID interface in statx, 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.
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);
Unlike AT_HANDLE_FID, as per Amir's suggestion, AT_HANDLE_MNT_ID_UNIQUE
uses a new AT_* bit from the historically-unused 0xFF range (which we
now define as being the "per-syscall" range for AT_* bits).
Signed-off-by: Aleksa Sarai <[email protected]>
---
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/[email protected]>
---
fs/fhandle.c | 29 ++++++++++++++++++++++-------
include/linux/syscalls.h | 2 +-
include/uapi/linux/fcntl.h | 28 +++++++++++++++++++++-------
tools/include/uapi/linux/fcntl.h | 28 +++++++++++++++++++++-------
4 files changed, 65 insertions(+), 22 deletions(-)
diff --git a/fs/fhandle.c b/fs/fhandle.c
index 8a7f86c2139a..c6e90161b900 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 e619ac10cd23..99c5a783a01e 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -863,7 +863,7 @@ asmlinkage long sys_fanotify_mark(int fanotify_fd, unsigned int flags,
const char __user *pathname);
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 c0bcc185fa48..9ed9d65842c1 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -87,22 +87,24 @@
#define DN_ATTRIB 0x00000020 /* File changed attibutes */
#define DN_MULTISHOT 0x80000000 /* Don't remove notifier */
+#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. */
+
/*
- * 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
+ * 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.
*/
-#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_EACCESS 0x200 /* Test access permitted for
effective IDs, not real IDs. */
#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 */
@@ -114,10 +116,22 @@
#define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
-/* Flags for name_to_handle_at(2). We reuse AT_ flag space to save bits... */
+/*
+ * All new purely-syscall-specific AT_* flags should consider using bits from
+ * 0xFF, but the bits used by RENAME_* (0x7) should be avoided in case users
+ * decide to pass AT_* flags to renameat2() by accident. These flag bits are
+ * free for re-use by other syscall's syscall-specific flags without worry.
+ */
+
+/*
+ * Flags for name_to_handle_at(2). To save AT_ flag space we re-use the
+ * AT_EACCESS/AT_REMOVEDIR bit for AT_HANDLE_FID.
+ */
#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) */
+#define AT_HANDLE_MNT_ID_UNIQUE 0x80 /* return the u64 unique mount id */
+
#if defined(__KERNEL__)
#define AT_GETATTR_NOSEC 0x80000000
#endif
diff --git a/tools/include/uapi/linux/fcntl.h b/tools/include/uapi/linux/fcntl.h
index 282e90aeb163..f671312ca481 100644
--- a/tools/include/uapi/linux/fcntl.h
+++ b/tools/include/uapi/linux/fcntl.h
@@ -85,22 +85,24 @@
#define DN_ATTRIB 0x00000020 /* File changed attibutes */
#define DN_MULTISHOT 0x80000000 /* Don't remove notifier */
+#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. */
+
/*
- * 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
+ * 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.
*/
-#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_EACCESS 0x200 /* Test access permitted for
effective IDs, not real IDs. */
#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 */
@@ -112,10 +114,22 @@
#define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
-/* Flags for name_to_handle_at(2). We reuse AT_ flag space to save bits... */
+/*
+ * All new purely-syscall-specific AT_* flags should consider using bits from
+ * 0xFF, but the bits used by RENAME_* (0x7) should be avoided in case users
+ * decide to pass AT_* flags to renameat2() by accident. These flag bits are
+ * free for re-use by other syscall's syscall-specific flags without worry.
+ */
+
+/*
+ * Flags for name_to_handle_at(2). To save AT_ flag space we re-use the
+ * AT_EACCESS/AT_REMOVEDIR bit for AT_HANDLE_FID.
+ */
#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) */
+#define AT_HANDLE_MNT_ID_UNIQUE 0x80 /* returned mount id is the u64 unique mount id */
+
#if defined(__KERNEL__)
#define AT_GETATTR_NOSEC 0x80000000
#endif
---
base-commit: 584bbf439d0fa83d728ec49f3a38c581bdc828b4
change-id: 20240515-exportfs-u64-mount-id-9ebb5c58b53c
Best regards,
--
Aleksa Sarai <[email protected]>
On Thu, May 23, 2024 at 11:57 PM Aleksa Sarai <[email protected]> wrote:
>
> Now that we provide a unique 64-bit mount ID interface in statx, 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.
>
> 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);
>
> Unlike AT_HANDLE_FID, as per Amir's suggestion, AT_HANDLE_MNT_ID_UNIQUE
> uses a new AT_* bit from the historically-unused 0xFF range (which we
> now define as being the "per-syscall" range for AT_* bits).
>
Sorry for nit picking, but I think that "Unlike AT_HANDLE_FID,..." is confusing
in this statement.
AT_HANDLE_FID is using a bit that was already effectively allocated for a
"per-syscall" range.
I don't think that mentioning AT_HANDLE_FID adds any clarity to the statement
so better drop it?
> Signed-off-by: Aleksa Sarai <[email protected]>
> ---
> 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/[email protected]>
> ---
> fs/fhandle.c | 29 ++++++++++++++++++++++-------
> include/linux/syscalls.h | 2 +-
> include/uapi/linux/fcntl.h | 28 +++++++++++++++++++++-------
> tools/include/uapi/linux/fcntl.h | 28 +++++++++++++++++++++-------
> 4 files changed, 65 insertions(+), 22 deletions(-)
>
[...]
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index c0bcc185fa48..9ed9d65842c1 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -87,22 +87,24 @@
> #define DN_ATTRIB 0x00000020 /* File changed attibutes */
> #define DN_MULTISHOT 0x80000000 /* Don't remove notifier */
>
> +#define AT_FDCWD -100 /* Special value used to indicate
> + openat should use the current
> + working directory. */
(more nit picking)
If you are changing this line, please at least add a new line,
this is a different namespace :-/
and perhaps change it to "Special value of dirfd argument..."
Also, better leave a comment here to discourage allocation from this range:
+ /* Reserved for per-syscall flags 0xff */
> +#define AT_SYMLINK_NOFOLLOW 0x100 /* Do not follow symbolic links. */
> +
> /*
> - * 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
> + * 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.
> */
If you are going to add this churn in this patch, please do it otherwise.
It does not make sense to have this long explanation about pre-syscall
AT_* flags in a different location from the comment you added about
"All new purely-syscall-specific AT_* flags.."
if this explanation is needed at all, it should be after the new comment
as an example.
> -#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_EACCESS 0x200 /* Test access permitted for
> effective IDs, not real IDs. */
> #define AT_REMOVEDIR 0x200 /* Remove directory instead of
> unlinking file. */
I really prefer to move those to the per-syscall section
right next to AT_HANDLE_FID and leave a comment here:
/* 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 */
> @@ -114,10 +116,22 @@
>
> #define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
>
> -/* Flags for name_to_handle_at(2). We reuse AT_ flag space to save bits... */
> +/*
> + * All new purely-syscall-specific AT_* flags should consider using bits from
> + * 0xFF, but the bits used by RENAME_* (0x7) should be avoided in case users
> + * decide to pass AT_* flags to renameat2() by accident.
Sorry, but I find the use of my renameat2() example a bit confusing
in this sentence.
If you mention it at all, please use "For example, the bits used by..."
but I think it is more important to say "...should consider re-using bits
already used by other per-syscalls flags".
> These flag bits are
> + * free for re-use by other syscall's syscall-specific flags without worry.
> + */
> +
> +/*
> + * Flags for name_to_handle_at(2). To save AT_ flag space we re-use the
> + * AT_EACCESS/AT_REMOVEDIR bit for AT_HANDLE_FID.
> + */
AT_EACCESS/AT_REMOVEDIR/AT_HANDLE_FID have exact same status,
so instead of this asymmetric comment:
+/* Flags for faccessat(2) */
+#define AT_EACCESS 0x200 /* Test access permitted for
+ effective IDs, not real IDs. */
+/* Flags for unlinkat(2) */
+#define AT_REMOVEDIR 0x200 /* Remove directory instead of
+ unlinking file. */
+/* 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 to open_by_handle_at(2) */
> +#define AT_HANDLE_MNT_ID_UNIQUE 0x80 /* return the u64 unique mount id */
IDGI, I think we may have been miscommunicating :-/
If 0x7 range is to be avoided for generic AT_ flags, then it *should* be used
for new per-syscall flags such as this one.
The reservation of 0xff is not a strong guarantee.
As long as people re-use new per-syscalls flags efficiently, we could
decide to reclaim some of this space for generic AT_ flags in the future
if it is needed.
I know most of the mess was here before your patch, but I think
it got to a point where we must put a little order before introducing
the new per-syscall flag.
Thanks,
Amir.
On 2024-05-26, Christoph Hellwig <[email protected]> wrote:
> On Thu, May 23, 2024 at 01:57:32PM -0700, Aleksa Sarai wrote:
> > Now that we provide a unique 64-bit mount ID interface in statx, 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.
>
> file handles are not tied to mounts, they are tied to super_blocks,
> and they can survive reboots or (less relevant) remounts. This thus
> seems like a very confusing if not wrong interfaces.
The existing interface already provides a mount ID which is not even
safe without rebooting.
The purpose of the mount ID (in the existing API) is to allow userspace
to make sure they know which filesystem mount the file handle came from
so they can store the relevant information (fsid, etc) to make sure they
know which superblock the mount came from when it comes to doing
open_by_handle_at(2). However, there is a race between getting the mount
ID from name_to_handle_at() and parsing /proc/self/mountinfo, which
means that userspace cannot ever be sure that they know the correct
mount the file handle came from (the man page for open_by_handle_at
mentions this issue explicitly).
Getting the ability to get a 64-bit mount ID would allow userspace to
use statmount(2) directly, avoiding this race. You're quite right that
obviously you wouldn't want to just store the mount ID.
An alternative would be to return something unique to the filesystem
superblock, but as far as I can tell there is no guarantee that every
Linux filesystem's fsid is sufficiently unique to act as a globally
unique identifier. At least with a 64-bit mount ID and statmount(2),
userspace can decide what information is needed to get sufficiently
unique information about the source filesystem.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
On Sun, 2024-05-26 at 02:25 -0700, Christoph Hellwig wrote:
> On Thu, May 23, 2024 at 01:57:32PM -0700, Aleksa Sarai wrote:
> > Now that we provide a unique 64-bit mount ID interface in statx, 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.
>
> file handles are not tied to mounts, they are tied to super_blocks,
> and they can survive reboots or (less relevant) remounts. This thus
> seems like a very confusing if not wrong interfaces.
I assume the reason is to give the caller a race free way to figure out
which submount the path resolves to. The problem is that nothing stops
another process from calling umount() before you're done parsing
/proc/mountinfo and have resolved the mount id.
If we're looking to change the API, then perhaps returning a file
descriptor might be a better alternative?
Most userland NFS servers are in any case going to follow up obtaining
the filehandle with a stat() or even a full blown open() in order to
get file attributes, set up file state, etc. By returning an open file
descriptor to the resolved file (even if it is only an O_PATH
descriptor) we could accelerate those operations in addition to solving
the umount() race.
Alternatively, just remove the path argument altogether, and require
the descriptor argument to be an O_PATH or regular open file descriptor
that resolves to the file we want to get a filehandle for. However this
would require a userland NFS server to generally do a
open_by_handle_at() to resolve the parent directory handle, then do an
openat(O_PATH) to get the file to look up, before being able to call
the name_to_handle_at() replacement.
i.e. there would be 1 extra syscall.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
On Sun, May 26, 2024 at 12:01:08PM -0700, Aleksa Sarai wrote:
> The existing interface already provides a mount ID which is not even
> safe without rebooting.
And that seems to be a big part of the problem where the Linux by handle
syscall API deviated from all know precedence for no good reason. NFS
file handles which were the start of this do (and have to) encode a
persistent file system identifier. As do the xfs handles (although they
do the decoding in the userspace library on Linux for historic reasons),
as do the FreeBSD equivalents to these syscalls.
> An alternative would be to return something unique to the filesystem
> superblock, but as far as I can tell there is no guarantee that every
> Linux filesystem's fsid is sufficiently unique to act as a globally
> unique identifier. At least with a 64-bit mount ID and statmount(2),
> userspace can decide what information is needed to get sufficiently
> unique information about the source filesystem.
Well, every file system that supports export ops already needs a
globally unique ID for NFS to work properly. We might not have good
enough interfaces for that, but that shouldn't be too hard.
On Sun, May 26, 2024 at 02:25:34AM -0700, Christoph Hellwig wrote:
> On Thu, May 23, 2024 at 01:57:32PM -0700, Aleksa Sarai wrote:
> > Now that we provide a unique 64-bit mount ID interface in statx, 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.
>
> file handles are not tied to mounts, they are tied to super_blocks,
> and they can survive reboots or (less relevant) remounts. This thus
> seems like a very confusing if not wrong interfaces.
I'm not fond of the interface as I've said on an earlier version, but
name_to_handle_at() has always exposed the mount id. IOW, this isn't
adding a new concept to the system call.
On Mon, May 27, 2024 at 04:47:56AM -0700, Christoph Hellwig wrote:
> On Sun, May 26, 2024 at 12:01:08PM -0700, Aleksa Sarai wrote:
> > The existing interface already provides a mount ID which is not even
> > safe without rebooting.
>
> And that seems to be a big part of the problem where the Linux by handle
> syscall API deviated from all know precedence for no good reason. NFS
> file handles which were the start of this do (and have to) encode a
> persistent file system identifier. As do the xfs handles (although they
> do the decoding in the userspace library on Linux for historic reasons),
> as do the FreeBSD equivalents to these syscalls.
>
> > An alternative would be to return something unique to the filesystem
> > superblock, but as far as I can tell there is no guarantee that every
> > Linux filesystem's fsid is sufficiently unique to act as a globally
> > unique identifier. At least with a 64-bit mount ID and statmount(2),
> > userspace can decide what information is needed to get sufficiently
> > unique information about the source filesystem.
>
> Well, every file system that supports export ops already needs a
> globally unique ID for NFS to work properly. We might not have good
> enough interfaces for that, but that shouldn't be too hard.
I see not inherent problem with exposing the 64 bit mount id through
name_to_handle_at() as we already to expose the old one anyway.
But I agree that it is useful if we had the guarantee that file handles
are unique in the way you describe. As it currently stands that doesn't
seem to be the case and userspace doesn't seem to have a way of figuring
out if the handle provided by name_to_handle_at() is indeed unique as
you describe and can be reliably passed to open_by_handle_at().
Yes, we should fix it but that's really orthogonal to the mount id. It
is separately useful and we already do expose it anyway.
On Mon, May 27, 2024 at 02:29:02PM +0200, Christian Brauner wrote:
> On Mon, May 27, 2024 at 04:47:56AM -0700, Christoph Hellwig wrote:
> > On Sun, May 26, 2024 at 12:01:08PM -0700, Aleksa Sarai wrote:
> > > The existing interface already provides a mount ID which is not even
> > > safe without rebooting.
> >
> > And that seems to be a big part of the problem where the Linux by handle
> > syscall API deviated from all know precedence for no good reason. NFS
> > file handles which were the start of this do (and have to) encode a
> > persistent file system identifier. As do the xfs handles (although they
> > do the decoding in the userspace library on Linux for historic reasons),
> > as do the FreeBSD equivalents to these syscalls.
> >
> > > An alternative would be to return something unique to the filesystem
> > > superblock, but as far as I can tell there is no guarantee that every
> > > Linux filesystem's fsid is sufficiently unique to act as a globally
> > > unique identifier. At least with a 64-bit mount ID and statmount(2),
> > > userspace can decide what information is needed to get sufficiently
> > > unique information about the source filesystem.
> >
> > Well, every file system that supports export ops already needs a
> > globally unique ID for NFS to work properly. We might not have good
> > enough interfaces for that, but that shouldn't be too hard.
>
> I see not inherent problem with exposing the 64 bit mount id through
> name_to_handle_at() as we already to expose the old one anyway.
>
> But I agree that it is useful if we had the guarantee that file handles
> are unique in the way you describe. As it currently stands that doesn't
> seem to be the case and userspace doesn't seem to have a way of figuring
> out if the handle provided by name_to_handle_at() is indeed unique as
> you describe and can be reliably passed to open_by_handle_at().
>
> Yes, we should fix it but that's really orthogonal to the mount id. It
> is separately useful and we already do expose it anyway.
Put another way, name_to_handle_at(2) currently states:
Obtaining a persistent filesystem ID
The mount IDs in /proc/self/mountinfo can be reused as
filesystems are unmounted and mounted. Therefore, the mount ID
returned by name_to_handle_at() (in *mount_id) should not be
treated as a persistent identifier for the corresponding
mounted filesystem. However, an application can use the
information in the mountinfo record that corresponds to the mount
ID to derive a persistent identifier.
For example, one can use the device name in the fifth field of
the mountinfo record to search for the corresponding device UUID
via the symbolic links in /dev/disks/by-uuid. (A more
comfortable way of obtaining the UUID is to use the libblkid(3)
library.) That process can then be reversed, using the UUID to
look up the device name, and then obtaining the corre‐ sponding
mount point, in order to produce the mount_fd argument used by
open_by_handle_at().
Returning the 64bit mount id makes this race-free because we now have
statmount():
u64 mnt_id = 0;
name_to_handle_at(AT_FDCWD, "/path/to/file", &handle, &mnt_id, 0);
statmount(mnt_id);
Which gets you the device number which one can use to figure out the
uuid without ever having to open a single file (We could even expose the
UUID of the filesystem through statmount() if we wanted to.).
On Mon, 2024-05-27 at 04:49 -0700, [email protected] wrote:
> On Sun, May 26, 2024 at 10:32:39PM +0000, Trond Myklebust wrote:
> > I assume the reason is to give the caller a race free way to figure
> > out
> > which submount the path resolves to.
>
> But the handle op are global to the file systems (aka super_block).
> It
> does not matter what mount you use to access it.
Sure. However if you are providing a path argument, then presumably you
need to know which file system (aka super_block) it eventually resolves
to.
>
> Snip random things about userland NFS servers I couldn't care less
> about..
>
My point was that at least for that case, you are better off using a
file descriptor and not having to care about the mount id.
If your use case isn't NFS servers, then what use case are you
targeting, and how do you expect those applications to use this API?
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
On Mon, 2024-05-27 at 15:17 +0200, Christian Brauner wrote:
>
> Returning the 64bit mount id makes this race-free because we now have
> statmount():
>
> u64 mnt_id = 0;
> name_to_handle_at(AT_FDCWD, "/path/to/file", &handle, &mnt_id, 0);
> statmount(mnt_id);
>
> Which gets you the device number which one can use to figure out the
> uuid without ever having to open a single file (We could even expose
> the
> UUID of the filesystem through statmount() if we wanted to.).
>
It is not race free. statmount() depends on the filesystem still being
mounted somewhere in your namespace, which is not guaranteed above.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
On Mon, May 27, 2024 at 03:47:33PM +0000, Trond Myklebust wrote:
> On Mon, 2024-05-27 at 15:17 +0200, Christian Brauner wrote:
> >
> > Returning the 64bit mount id makes this race-free because we now have
> > statmount():
> >
> > u64 mnt_id = 0;
> > name_to_handle_at(AT_FDCWD, "/path/to/file", &handle, &mnt_id, 0);
> > statmount(mnt_id);
> >
> > Which gets you the device number which one can use to figure out the
> > uuid without ever having to open a single file (We could even expose
> > the
> > UUID of the filesystem through statmount() if we wanted to.).
> >
>
> It is not race free. statmount() depends on the filesystem still being
> mounted somewhere in your namespace, which is not guaranteed above.
The unsigned 64bit mount is not recyclable. It is a unique identifier
for a mount for the lifetime of the system. Even if bumped on every
cycle it will still take hundreds of years to overflow.
On Mon, May 27, 2024 at 09:29:48AM -0700, [email protected] wrote:
> On Mon, May 27, 2024 at 03:38:40PM +0000, Trond Myklebust wrote:
> > > It
> > > does not matter what mount you use to access it.
> >
> > Sure. However if you are providing a path argument, then presumably you
> > need to know which file system (aka super_block) it eventually resolves
> > to.
>
> Except that you can't, at least not without running into potential
> races. The only way to fix a race vs unmount/remount is to include
> the fsid part in the kernel generated file handle.
>
> >
> > If your use case isn't NFS servers, then what use case are you
> > targeting, and how do you expect those applications to use this API?
>
> The main user of the open by handle syscalls seems to be fanotify
> magic.
It's also used by userspace for uniquely identifying cgroups via handles
as cgroups and - even without open_by_handle_at() - to check whether a
file is still valid.
And again a 64bit mount is is a simple way to race-free go to whatever
superblock uuid you want. They cannot be recycled and are unique for the
lifetime of the system.
> Well, how about we fix the thing for real:
>
> - allow file systems to provide a uniqueu identifier of at least
> uuid size (16 bytes) in the superblock or through an export operation
> - for non-persistent file systems allow to generate one at boot time
> using the normal uuid generation helpers
> - add a new flag to name_to_handle_at/open_by_handle_at to include it
> in the file handle, and thus make the file handle work more like
> the normal file handle
> - add code to nfsd to directly make use of this
>
> This would solve all the problems in this proposal as well as all the
> obvious ones it doesn't solve.
As I've said multiple times on the thread I agree that this is what we
should do next and I would be happy to take patches for this. But
exposing the 64bit mount id doesn't impede or complicate that work. It's
a simple and useful addition that can be done now and doesn't prevent us
from doing the proposed work.
Hell, if you twist my arm I'll even write the patches for this.
> > Hell, if you twist my arm I'll even write the patches for this.
>
> I'm also happy to help with that despite very limited time, but I'd
> rather avoid doing the misguided mount ID thing.
As I've said earlier, independent of the new handle type returning the
new mount id is useful and needed because it allows the caller to
reliably generate a mount fd for use with open_by_handle_at() via
statmount(). That won't be solved by a new handle type and is racy with
the old mount id. So I intend to accept a version of this patch.
On Tue, May 28, 2024 at 10:20:21AM +0200, Christian Brauner wrote:
> > This would solve all the problems in this proposal as well as all the
> > obvious ones it doesn't solve.
>
> As I've said multiple times on the thread I agree that this is what we
> should do next and I would be happy to take patches for this. But
> exposing the 64bit mount id doesn't impede or complicate that work. It's
> a simple and useful addition that can be done now and doesn't prevent us
> from doing the proposed work.
I really confuses the user story as we now have not only one but two
broken modes for the open by handle ops. We just further go down the
rabbit hole of mixing a non-persistent identifiers with a persistent
one.
> Hell, if you twist my arm I'll even write the patches for this.
I'm also happy to help with that despite very limited time, but I'd
rather avoid doing the misguided mount ID thing.
On Mon 27-05-24 09:29:48, [email protected] wrote:
> On Mon, May 27, 2024 at 03:38:40PM +0000, Trond Myklebust wrote:
> > If your use case isn't NFS servers, then what use case are you
> > targeting, and how do you expect those applications to use this API?
>
> The main user of the open by handle syscalls seems to be fanotify
> magic.
So some fanotify users may use open_by_handle_at() and name_to_handle_at()
but we specifically designed fanotify to not depend on this mount id
feature of the API (because it wasn't really usable couple of years ago
when we were designing this with Amir). fanotify returns fsid + fhandle in
its events and userspace is expected to build a mapping of fsid ->
"whatever it needs to identify a filesystem" when placing fanotify marks.
If it wants to open file / directory where events happened, then this
usually means keeping fsid -> "some open fd on fs" mapping so that it can
then use open_by_handle_at() for opening.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Tue, May 28, 2024 at 12:11:52PM +0200, Jan Kara wrote:
> So some fanotify users may use open_by_handle_at() and name_to_handle_at()
> but we specifically designed fanotify to not depend on this mount id
> feature of the API (because it wasn't really usable couple of years ago
> when we were designing this with Amir). fanotify returns fsid + fhandle in
> its events and userspace is expected to build a mapping of fsid ->
> "whatever it needs to identify a filesystem" when placing fanotify marks.
> If it wants to open file / directory where events happened, then this
> usually means keeping fsid -> "some open fd on fs" mapping so that it can
> then use open_by_handle_at() for opening.
Which seems like another argument for my version of the handles to
include the fsid. Although IIRC the fanotify fsid is only 64 bits which
isn't really good entropy, so we might have to rev that as well.
On Tue, May 28, 2024 at 03:55:29AM -0700, Christoph Hellwig wrote:
> On Tue, May 28, 2024 at 11:17:58AM +0200, Christian Brauner wrote:
> > As I've said earlier, independent of the new handle type returning the
> > new mount id is useful and needed because it allows the caller to
> > reliably generate a mount fd for use with open_by_handle_at() via
> > statmount(). That won't be solved by a new handle type and is racy with
> > the old mount id. So I intend to accept a version of this patch.
>
> The whole point is that with the fsid in the handle we do not even need
> a mount fd for open_by_handle_at.
Can you please explain how opening an fd based on a handle returned from
name_to_handle_at() and not using a mount file descriptor for
open_by_handle_at() would work?
On Tue, May 28, 2024 at 02:04:16PM +0200, Christian Brauner wrote:
> Can you please explain how opening an fd based on a handle returned from
> name_to_handle_at() and not using a mount file descriptor for
> open_by_handle_at() would work?
Same as NFS file handles:
name_to_handle_at returns a handle that includes a file system
identifier.
open_by_handle_at looks up the superblock based on that identifier.
For the identifier I could imagin three choices:
1) use the fsid as returned in statfs and returned by fsnotify.
The downside is that it is "only" 64-bit. The upside is that
we have a lot of plumbing for it
2) fixed 128-bit identifier to provide more entropy
3) a variable length identifier, which is more similar to NFS,
but also a lot more complicated
We'd need a global lookup structure to find the sb by id. The simplest
one would be a simple linear loop over super_blocks which isn't terribly
efficient, but probably better than whatever userspace is doing to
find a mount fd right now.
Let me cook up a simple prototype for 1) as it shouldn't be more than
a few hundred lines of code.
On Wed, May 29, 2024 at 09:25:49AM +1000, Dave Chinner wrote:
> But no-one has bothered to reply or acknowledge my comments so I'll
> point them out again and repeat: Filehandles generated by
> the kernel for unprivileged use *must* be self describing and self
> validating as the kernel must be able to detect and prevent
> unprivelged users from generating custom filehandles that can be
> used to access files outside the restricted scope of their
> container.
We must not generate file handle for unprivileged use at all, as they
bypass all the path based access controls.
On Tue, May 28, 2024 at 03:28:18PM +0200, Miklos Szeredi wrote:
> > open_by_handle_at looks up the superblock based on that identifier.
>
> The open file needs a specific mount, holding the superblock is not
> sufficient.
A strut file needs a vfsmount, yes. And it better be reachable by
the calling process. And maybe an optional restriction to a specific
mount by the caller might be useful, but I can't see how it is
required.
On Wed, May 29, 2024 at 9:24 AM [email protected] <[email protected]> wrote:
>
> On Wed, May 29, 2024 at 09:25:49AM +1000, Dave Chinner wrote:
> > But no-one has bothered to reply or acknowledge my comments so I'll
> > point them out again and repeat: Filehandles generated by
> > the kernel for unprivileged use *must* be self describing and self
> > validating
Very nice requirement for a very strong feature,
which is far out of scope for the proposed patch IMO.
What is "generated by the kernel for unprivileged use"?
name_to_handle_at() is unprivileged and for example, fanotify unprivileged
users can use it to compare a marked (i.e. watched) object with an
object that is the subject of an event.
This does not break any security model.
> > as the kernel must be able to detect and prevent
> > unprivelged users from generating custom filehandles that can be
> > used to access files outside the restricted scope of their
> > container.
Emphasis on "that can be used to access files".
The patch in this thread to get a unique 64bit mntid does not make any
difference wrt to the concern above, so I am having a hard time
understand what the fuss is about.
>
> We must not generate file handle for unprivileged use at all, as they
> bypass all the path based access controls.
>
Again, how is "generate file handle for unprivileged use" related to
the patch in question.
The simple solution to the race that Aleksa is trying to prevent,
as was mentioned several times in this thread, is to allow
name_to_handle_at() on an empty path, e.g.:
fd = openat(.., O_PATH); fstatat(fd,..); name_to_handle_at(fd,..);
Aleksa prefers the unique mntid solution to save 2 syscalls.
Would any of the objections here been called for letting
name_to_handle_at() take an empty path?
I would like to offer a different solution (also may have been
mentioned before).
I always disliked the fact that mount_id and mount_fd arguments of
name_to_handle_at()/open_by_handle_at() are not symmetric, when
at first glance they appear to be symmetric as the handle argument.
What if we can make them symmetric and save the openat() syscall?
int path_fd;
int ret = name_to_handle_at(dirfd, path, handle, &path_fd,
AT_HADNLE_PATH_FD);
and then kernel returns an O_PATH fd to the path object
in addition to the file handle (requires same privilege as
encoding the fh).
Userspace can use path_fd to query unique mntid, fsid, uuid
or whatever it needs to know in order to make sure that a
later call in the future to open_by_handle_at() will use
a mount_fd from the same filesystem/mount whatever,
whether in the same boot or after reboot.
If we are doing this, it also makes sense to allow mount_fd argument
to open_by_handle_at() to accept O_PATH fd, but that makes
sense anyway, because mount_fd is essentially a reference to
a path.
Thanks,
Amir.
On Tue, May 28, 2024 at 06:22:23AM -0700, Christoph Hellwig wrote:
> On Tue, May 28, 2024 at 02:04:16PM +0200, Christian Brauner wrote:
> > Can you please explain how opening an fd based on a handle returned from
> > name_to_handle_at() and not using a mount file descriptor for
> > open_by_handle_at() would work?
>
> Same as NFS file handles:
>
> name_to_handle_at returns a handle that includes a file system
> identifier.
>
> open_by_handle_at looks up the superblock based on that identifier.
>
> For the identifier I could imagin three choices:
>
> 1) use the fsid as returned in statfs and returned by fsnotify.
> The downside is that it is "only" 64-bit. The upside is that
> we have a lot of plumbing for it
> 2) fixed 128-bit identifier to provide more entropy
> 3) a variable length identifier, which is more similar to NFS,
> but also a lot more complicated
>
> We'd need a global lookup structure to find the sb by id. The simplest
> one would be a simple linear loop over super_blocks which isn't terribly
> efficient, but probably better than whatever userspace is doing to
> find a mount fd right now.
>
> Let me cook up a simple prototype for 1) as it shouldn't be more than
> a few hundred lines of code.
Yeah, that's exactly what I figured and no that's not something we
should do.
Not just can have a really large number of superblocks if you have mount
namespaces and large container workloads that interface also needs to be
highly privileged.
Plus, you do have filesystems like btrfs that can be mounted multiple
times with the same uuid.
And in general users will still need to be able to legitimately use a
mount fd and not care about the handle type used with it.
On Wed, May 29, 2024 at 09:40:01AM +0200, Christian Brauner wrote:
> Yeah, that's exactly what I figured and no that's not something we
> should do.
>
> Not just can have a really large number of superblocks if you have mount
> namespaces and large container workloads that interface also needs to be
> highly privileged.
Again, that would be the most trivial POC. We can easily do hash.
> Plus, you do have filesystems like btrfs that can be mounted multiple
> times with the same uuid.
Which doesn't matter. Just like for NFS file handles the fs identifier
identifier plus the file part of the file handle need to be unique.
> And in general users will still need to be able to legitimately use a
> mount fd and not care about the handle type used with it.
I don't understand what you mean. If we hand out file handles with
fsid that of course needs to be keyed off a new flag for both
name_to_handle and open_by_hnalde that makes them not interchangable
to handles generated without that flag.
> I don't understand what you mean. If we hand out file handles with
I don't think adding another highly privileged interface in opposition
to exposing the mount id is warranted. The new mount id exposure patch
is a performance improvement for existing use-cases to let userspace
avoid additional system calls. Because as Aleksa showed in the commit
message there's ways to make this race-free right now. The patch hasn't
gained any Acks from Amir or Jeff so it's obviously not going to get
merged. So really, I don't want another interface that goes from
arbitrary superblock to file hidden behind yet another global capability
check.
On 2024-05-28, Miklos Szeredi <[email protected]> wrote:
> On Tue, 28 May 2024 at 15:24, Christoph Hellwig <[email protected]> wrote:
> >
> > On Tue, May 28, 2024 at 02:04:16PM +0200, Christian Brauner wrote:
> > > Can you please explain how opening an fd based on a handle returned from
> > > name_to_handle_at() and not using a mount file descriptor for
> > > open_by_handle_at() would work?
> >
> > Same as NFS file handles:
> >
> > name_to_handle_at returns a handle that includes a file system
> > identifier.
> >
> > open_by_handle_at looks up the superblock based on that identifier.
>
> The open file needs a specific mount, holding the superblock is not sufficient.
Not to mention that providing a mount fd is what allows for extensions
like Christian's proposed method of allowing restricted forms of
open_by_handle_at() to be used by unprivileged users.
If file handles really are going to end up being the "correct" mechanism
of referencing inodes by userspace, then future API designs really need
to stop assuming that the user is capable(CAP_DAC_READ_SEARCH). Being
able to open any file in any superblock the kernel knows about
(presumably using a kernel-internal mount if we are getting rid of the
mount fd) is also capable(CAP_SYS_ADMIN) territory.
Would the idea be to sign or MAC every file handle to avoid userspace
being able to brute-force the file handle of anything the system sees?
What happens if the key has to change? Then the handles aren't globally
unique anymore...
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
On Sat 01-06-24 01:12:31, Aleksa Sarai wrote:
> On 2024-05-28, Miklos Szeredi <[email protected]> wrote:
> > On Tue, 28 May 2024 at 15:24, Christoph Hellwig <[email protected]> wrote:
> > >
> > > On Tue, May 28, 2024 at 02:04:16PM +0200, Christian Brauner wrote:
> > > > Can you please explain how opening an fd based on a handle returned from
> > > > name_to_handle_at() and not using a mount file descriptor for
> > > > open_by_handle_at() would work?
> > >
> > > Same as NFS file handles:
> > >
> > > name_to_handle_at returns a handle that includes a file system
> > > identifier.
> > >
> > > open_by_handle_at looks up the superblock based on that identifier.
> >
> > The open file needs a specific mount, holding the superblock is not sufficient.
>
> Not to mention that providing a mount fd is what allows for extensions
> like Christian's proposed method of allowing restricted forms of
> open_by_handle_at() to be used by unprivileged users.
>
> If file handles really are going to end up being the "correct" mechanism
> of referencing inodes by userspace, then future API designs really need
> to stop assuming that the user is capable(CAP_DAC_READ_SEARCH). Being
> able to open any file in any superblock the kernel knows about
> (presumably using a kernel-internal mount if we are getting rid of the
> mount fd) is also capable(CAP_SYS_ADMIN) territory.
Well, but this is already handled - name_to_handle_at() with AT_HANDLE_FID
is completely unpriviledged operation. Unpriviledged userspace can use
fhandle for comparisons with other file handles but that's all it is good
for (similarly as inode number you get from statx(2) but does not have the
problem with inode number uniqueness on btrfs, bcachefs, etc.). I don't
expect unpriviledged userspace to be able to more with the fhandle it got.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Sat, Jun 01, 2024 at 01:12:31AM -0700, Aleksa Sarai wrote:
> Not to mention that providing a mount fd is what allows for extensions
> like Christian's proposed method of allowing restricted forms of
> open_by_handle_at() to be used by unprivileged users.
As mentioned there I find the concept of an unprivileged
open_by_handle_at extremely questionable as it trivially gives access to
any inode on the file systems.
> If file handles really are going to end up being the "correct" mechanism
> of referencing inodes by userspace,
They aren't.
> then future API designs really need
> to stop assuming that the user is capable(CAP_DAC_READ_SEARCH).
There is no way to support open by handle for unprivileged users. The
concept of an inode number based file handle simply does not work for
that at all.