Re: [PATCH 03/10] fhandle: helper for allocating, reading struct file_handle

[Amir Goldstein <amir73il@gmail.com>]

On Thu, Sep 11, 2025 at 5:26 PM Thomas Bertschinger
<tahbertschinger@gmail.com> wrote:
>
> On Thu Sep 11, 2025 at 6:15 AM MDT, Amir Goldstein wrote:
> > On Wed, Sep 10, 2025 at 11:47 PM Thomas Bertschinger
> > <tahbertschinger@gmail.com> wrote:
> >>
> >> Pull the code for allocating and copying a struct file_handle from
> >> userspace into a helper function get_user_handle() just for this.
> >>
> >> do_handle_open() is updated to call get_user_handle() prior to calling
> >> handle_to_path(), and the latter now takes a kernel pointer as a
> >> parameter instead of a __user pointer.
> >>
> >> This new helper, as well as handle_to_path(), are also exposed in
> >> fs/internal.h. In a subsequent commit, io_uring will use these helpers
> >> to support open_by_handle_at(2) in io_uring.
> >>
> >> Signed-off-by: Thomas Bertschinger <tahbertschinger@gmail.com>
> >> ---
> >>  fs/fhandle.c  | 64 +++++++++++++++++++++++++++++----------------------
> >>  fs/internal.h |  3 +++
> >>  2 files changed, 40 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/fs/fhandle.c b/fs/fhandle.c
> >> index 605ad8e7d93d..36e194dd4cb6 100644
> >> --- a/fs/fhandle.c
> >> +++ b/fs/fhandle.c
> >> @@ -330,25 +330,45 @@ static inline int may_decode_fh(struct handle_to_path_ctx *ctx,
> >>         return 0;
> >>  }
> >>
> >> -static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
> >> -                  struct path *path, unsigned int o_flags)
> >> +struct file_handle *get_user_handle(struct file_handle __user *ufh)
> >>  {
> >> -       int retval = 0;
> >>         struct file_handle f_handle;
> >> -       struct file_handle *handle __free(kfree) = NULL;
> >> -       struct handle_to_path_ctx ctx = {};
> >> -       const struct export_operations *eops;
> >> +       struct file_handle *handle;
> >>
> >>         if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle)))
> >> -               return -EFAULT;
> >> +               return ERR_PTR(-EFAULT);
> >>
> >>         if ((f_handle.handle_bytes > MAX_HANDLE_SZ) ||
> >>             (f_handle.handle_bytes == 0))
> >> -               return -EINVAL;
> >> +               return ERR_PTR(-EINVAL);
> >>
> >>         if (f_handle.handle_type < 0 ||
> >>             FILEID_USER_FLAGS(f_handle.handle_type) & ~FILEID_VALID_USER_FLAGS)
> >> -               return -EINVAL;
> >> +               return ERR_PTR(-EINVAL);
> >> +
> >> +       handle = kmalloc(struct_size(handle, f_handle, f_handle.handle_bytes),
> >> +                        GFP_KERNEL);
> >> +       if (!handle) {
> >> +               return ERR_PTR(-ENOMEM);
> >> +       }
> >> +
> >> +       /* copy the full handle */
> >> +       *handle = f_handle;
> >> +       if (copy_from_user(&handle->f_handle,
> >> +                          &ufh->f_handle,
> >> +                          f_handle.handle_bytes)) {
> >> +               return ERR_PTR(-EFAULT);
> >> +       }
> >> +
> >> +       return handle;
> >> +}
> >> +
> >> +int handle_to_path(int mountdirfd, struct file_handle *handle,
> >> +                  struct path *path, unsigned int o_flags)
> >> +{
> >> +       int retval = 0;
> >> +       struct handle_to_path_ctx ctx = {};
> >> +       const struct export_operations *eops;
> >>
> >>         retval = get_path_anchor(mountdirfd, &ctx.root);
> >>         if (retval)
> >> @@ -362,31 +382,16 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
> >>         if (retval)
> >>                 goto out_path;
> >>
> >> -       handle = kmalloc(struct_size(handle, f_handle, f_handle.handle_bytes),
> >> -                        GFP_KERNEL);
> >> -       if (!handle) {
> >> -               retval = -ENOMEM;
> >> -               goto out_path;
> >> -       }
> >> -       /* copy the full handle */
> >> -       *handle = f_handle;
> >> -       if (copy_from_user(&handle->f_handle,
> >> -                          &ufh->f_handle,
> >> -                          f_handle.handle_bytes)) {
> >> -               retval = -EFAULT;
> >> -               goto out_path;
> >> -       }
> >> -
> >>         /*
> >>          * If handle was encoded with AT_HANDLE_CONNECTABLE, verify that we
> >>          * are decoding an fd with connected path, which is accessible from
> >>          * the mount fd path.
> >>          */
> >> -       if (f_handle.handle_type & FILEID_IS_CONNECTABLE) {
> >> +       if (handle->handle_type & FILEID_IS_CONNECTABLE) {
> >>                 ctx.fh_flags |= EXPORT_FH_CONNECTABLE;
> >>                 ctx.flags |= HANDLE_CHECK_SUBTREE;
> >>         }
> >> -       if (f_handle.handle_type & FILEID_IS_DIR)
> >> +       if (handle->handle_type & FILEID_IS_DIR)
> >>                 ctx.fh_flags |= EXPORT_FH_DIR_ONLY;
> >>         /* Filesystem code should not be exposed to user flags */
> >>         handle->handle_type &= ~FILEID_USER_FLAGS_MASK;
> >> @@ -400,12 +405,17 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
> >>  static long do_handle_open(int mountdirfd, struct file_handle __user *ufh,
> >>                            int open_flag)
> >>  {
> >> +       struct file_handle *handle __free(kfree) = NULL;
> >>         long retval = 0;
> >>         struct path path __free(path_put) = {};
> >>         struct file *file;
> >>         const struct export_operations *eops;
> >>
> >> -       retval = handle_to_path(mountdirfd, ufh, &path, open_flag);
> >> +       handle = get_user_handle(ufh);
> >> +       if (IS_ERR(handle))
> >> +               return PTR_ERR(handle);
> >
> > I don't think you can use __free(kfree) for something that can be an ERR_PTR.
> >
> > Thanks,
> > Amir.
>
> It looks like the error pointer is correctly handled?
>
> in include/linux/slab.h:
>
> DEFINE_FREE(kfree, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T))

Right! feel free to add for v3:
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

Thanks,
Amir.