Re: [PATCH v3 1/2] mm/memfd: Refactor and cleanup the logic in memfd_create()

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

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@google.com>

Great this looks good now, thanks!

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

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
>