On Thu, Sep 11, 2025 at 5:26 PM Thomas Bertschinger
<tahbertschinger@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxx>
Thanks,
Amir.
