Re: [PATCH 1/6] fhandle: create helper for name_to_handle_at(2)

[Amir Goldstein <amir73il@gmail.com>]

On Fri, Aug 15, 2025 at 1:51 AM Thomas Bertschinger
<tahbertschinger@gmail.com> wrote:
>
> Create a helper do_name_to_handle_at() that takes an additional argument,
> lookup_flags, beyond the syscall arguments.
>
> Because name_to_handle_at(2) doesn't take any lookup flags, it always
> passes 0 for this argument.
>
> Future callers like io_uring may pass LOOKUP_CACHED in order to request
> a non-blocking lookup.
>
> This helper's name is confusingly similar to do_sys_name_to_handle()
> which takes care of returning the file handle, once the filename has
> been turned into a struct path. To distinguish the names more clearly,
> rename the latter to do_path_to_handle().
>
> Signed-off-by: Thomas Bertschinger <tahbertschinger@gmail.com>
> ---
>  fs/fhandle.c  | 61 ++++++++++++++++++++++++++++-----------------------
>  fs/internal.h |  7 ++++++
>  2 files changed, 41 insertions(+), 27 deletions(-)
>
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 7c236f64cdea..57da648ca866 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -14,10 +14,10 @@
>  #include "internal.h"
>  #include "mount.h"
>
> -static long do_sys_name_to_handle(const struct path *path,
> -                                 struct file_handle __user *ufh,
> -                                 void __user *mnt_id, bool unique_mntid,
> -                                 int fh_flags)
> +static long do_path_to_handle(const struct path *path,
> +                             struct file_handle __user *ufh,
> +                             void __user *mnt_id, bool unique_mntid,
> +                             int fh_flags)
>  {
>         long retval;
>         struct file_handle f_handle;
> @@ -111,27 +111,11 @@ static long do_sys_name_to_handle(const struct path *path,
>         return retval;
>  }
>
> -/**
> - * sys_name_to_handle_at: convert name to handle
> - * @dfd: directory relative to which name is interpreted if not absolute
> - * @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.
> - *
> - * @handle->handle_size indicate the space available to store the
> - * variable part of the file handle in bytes. If there is not
> - * enough space, the field is updated to return the minimum
> - * value required.
> - */
> -SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
> -               struct file_handle __user *, handle, void __user *, mnt_id,
> -               int, flag)
> +long do_name_to_handle_at(int dfd, const char __user *name,
> +                         struct file_handle __user *handle, void __user *mnt_id,
> +                         int flag, int lookup_flags)
>  {
>         struct path path;
> -       int lookup_flags;
>         int fh_flags = 0;
>         int err;
>
> @@ -155,19 +139,42 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
>         else if (flag & AT_HANDLE_CONNECTABLE)
>                 fh_flags |= EXPORT_FH_CONNECTABLE;
>
> -       lookup_flags = (flag & AT_SYMLINK_FOLLOW) ? LOOKUP_FOLLOW : 0;
> +       if (flag & AT_SYMLINK_FOLLOW)
> +               lookup_flags |= LOOKUP_FOLLOW;
>         if (flag & AT_EMPTY_PATH)
>                 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,
> -                                           flag & AT_HANDLE_MNT_ID_UNIQUE,
> -                                           fh_flags);
> +               err = do_path_to_handle(&path, handle, mnt_id,
> +                                       flag & AT_HANDLE_MNT_ID_UNIQUE,
> +                                       fh_flags);
>                 path_put(&path);
>         }
>         return err;
>  }
>
> +/**
> + * sys_name_to_handle_at: convert name to handle
> + * @dfd: directory relative to which name is interpreted if not absolute
> + * @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.
> + *
> + * @handle->handle_size indicate the space available to store the
> + * variable part of the file handle in bytes. If there is not
> + * enough space, the field is updated to return the minimum
> + * value required.
> + */
> +SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
> +               struct file_handle __user *, handle, void __user *, mnt_id,
> +               int, flag)
> +{
> +       return do_name_to_handle_at(dfd, name, handle, mnt_id, flag, 0);
> +}
> +
>  static int get_path_anchor(int fd, struct path *root)
>  {
>         if (fd >= 0) {
> diff --git a/fs/internal.h b/fs/internal.h
> index 38e8aab27bbd..af7e0810a90d 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -355,3 +355,10 @@ int anon_inode_getattr(struct mnt_idmap *idmap, const struct path *path,
>  int anon_inode_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>                        struct iattr *attr);
>  void pidfs_get_root(struct path *path);
> +
> +/*
> + * fs/fhandle.c
> + */
> +long do_name_to_handle_at(int dfd, const char __user *name,
> +                         struct file_handle __user *handle,
> +                         void __user *mnt_id, int flag, int lookup_flags);

I really dislike do_XXX() helpers because we use them interchangeably
sometimes to wrap vfs_XXX() helpers and sometimes the other way around,
so exporting them in the vfs internal interface is a very bad pattern IMO.

io_uring has a common pattern that requires a helper with all the syscall
args and for that purpose, it uses do_renameat2(), do_unlinkat(), ...

I would much rather that we stop this pattern and start with following
the do_sys_XXX() pattern as in the do_sys_ftruncate() helper.

Lucky for us, you just renamed the confusing helper named
do_sys_name_to_handle(), so you are free to reuse this name
(+ _at) in a non confusing placement.

Thanks,
Amir.