On Thu, Jan 09, 2025 at 10:59:04AM -0800, Isaac J. Manjarres wrote:
> memfd_create() is a pretty busy function that could be easier to read
> if some of the logic was split out into helper functions.
>
> Therefore, split the flags sanitization, name allocation, and file
> structure allocation into their own helper functions.
>
> No functional change.
>
> Signed-off-by: Isaac J. Manjarres <isaacmanjarres@xxxxxxxxxx>
Great this looks good now, thanks!
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
I agree with Alice that the comment re: kfree() is superfluous, not
critical, but if you want to do a v4 feel free to migrate tags to that.
> ---
> mm/memfd.c | 82 +++++++++++++++++++++++++++++++++++++++---------------
> 1 file changed, 59 insertions(+), 23 deletions(-)
>
> diff --git a/mm/memfd.c b/mm/memfd.c
> index 5f5a23c9051d..bf0c2d97b940 100644
> --- a/mm/memfd.c
> +++ b/mm/memfd.c
> @@ -369,15 +369,9 @@ int memfd_check_seals_mmap(struct file *file, unsigned long *vm_flags_ptr)
> return err;
> }
>
> -SYSCALL_DEFINE2(memfd_create,
> - const char __user *, uname,
> - unsigned int, flags)
> +static int sanitize_flags(unsigned int *flags_ptr)
> {
> - unsigned int *file_seals;
> - struct file *file;
> - int fd, error;
> - char *name;
> - long len;
> + unsigned int flags = *flags_ptr;
>
> if (!(flags & MFD_HUGETLB)) {
> if (flags & ~(unsigned int)MFD_ALL_FLAGS)
> @@ -393,20 +387,25 @@ SYSCALL_DEFINE2(memfd_create,
> if ((flags & MFD_EXEC) && (flags & MFD_NOEXEC_SEAL))
> return -EINVAL;
>
> - error = check_sysctl_memfd_noexec(&flags);
> - if (error < 0)
> - return error;
> + return check_sysctl_memfd_noexec(flags_ptr);
> +}
> +
> +static char *alloc_name(const char __user *uname)
> +{
> + int error;
> + char *name;
> + long len;
>
> /* length includes terminating zero */
> len = strnlen_user(uname, MFD_NAME_MAX_LEN + 1);
> if (len <= 0)
> - return -EFAULT;
> + return ERR_PTR(-EFAULT);
> if (len > MFD_NAME_MAX_LEN + 1)
> - return -EINVAL;
> + return ERR_PTR(-EINVAL);
>
> name = kmalloc(len + MFD_NAME_PREFIX_LEN, GFP_KERNEL);
> if (!name)
> - return -ENOMEM;
> + return ERR_PTR(-ENOMEM);
>
> strcpy(name, MFD_NAME_PREFIX);
> if (copy_from_user(&name[MFD_NAME_PREFIX_LEN], uname, len)) {
> @@ -420,23 +419,28 @@ SYSCALL_DEFINE2(memfd_create,
> goto err_name;
> }
>
> - fd = get_unused_fd_flags((flags & MFD_CLOEXEC) ? O_CLOEXEC : 0);
> - if (fd < 0) {
> - error = fd;
> - goto err_name;
> - }
> + return name;
> +
> +err_name:
> + kfree(name);
> + return ERR_PTR(error);
> +}
> +
> +static struct file *alloc_file(const char *name, unsigned int flags)
> +{
> + unsigned int *file_seals;
> + struct file *file;
>
> if (flags & MFD_HUGETLB) {
> file = hugetlb_file_setup(name, 0, VM_NORESERVE,
> HUGETLB_ANONHUGE_INODE,
> (flags >> MFD_HUGE_SHIFT) &
> MFD_HUGE_MASK);
> - } else
> + } else {
> file = shmem_file_setup(name, 0, VM_NORESERVE);
> - if (IS_ERR(file)) {
> - error = PTR_ERR(file);
> - goto err_fd;
> }
> + if (IS_ERR(file))
> + return file;
> file->f_mode |= FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE;
> file->f_flags |= O_LARGEFILE;
>
> @@ -456,7 +460,39 @@ SYSCALL_DEFINE2(memfd_create,
> *file_seals &= ~F_SEAL_SEAL;
> }
>
> + return file;
> +}
> +
> +SYSCALL_DEFINE2(memfd_create,
> + const char __user *, uname,
> + unsigned int, flags)
> +{
> + struct file *file;
> + int fd, error;
> + char *name;
> +
> + error = sanitize_flags(&flags);
> + if (error < 0)
> + return error;
> +
> + name = alloc_name(uname);
> + if (IS_ERR(name))
> + return PTR_ERR(name);
> +
> + fd = get_unused_fd_flags((flags & MFD_CLOEXEC) ? O_CLOEXEC : 0);
> + if (fd < 0) {
> + error = fd;
> + goto err_name;
> + }
> +
> + file = alloc_file(name, flags);
> + if (IS_ERR(file)) {
> + error = PTR_ERR(file);
> + goto err_fd;
> + }
> +
> fd_install(fd, file);
> + /* name is not needed beyond this point. */
As per Alice, we don't really need this any more.
> kfree(name);
> return fd;
>
> --
> 2.47.1.613.gc27f4b7a9f-goog
>
