1. Patchew
  2. linux
  3. [PATCH v3 0/2] Cleanup for memfd_create()
  4. View patch

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

Isaac J. Manjarres posted 2 patches 1 year ago
There is a newer version of this series
[PATCH v3 1/2] mm/memfd: Refactor and cleanup the logic in memfd_create()
Posted by Isaac J. Manjarres 1 year ago 
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>
---
 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. */
 	kfree(name);
 	return fd;
 
-- 
2.47.1.613.gc27f4b7a9f-goog
Re: [PATCH v3 1/2] mm/memfd: Refactor and cleanup the logic in memfd_create()
Posted by Lorenzo Stoakes 1 year ago 
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
>
Re: [PATCH v3 1/2] mm/memfd: Refactor and cleanup the logic in memfd_create()
Posted by Isaac Manjarres 1 year ago 
On Fri, Jan 10, 2025 at 12:47:42PM +0000, Lorenzo Stoakes wrote:
> 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.
> 

Thanks! Yes, I'll go ahead and do that.

--Isaac
Re: [PATCH v3 1/2] mm/memfd: Refactor and cleanup the logic in memfd_create()
Posted by Alice Ryhl 1 year ago 
On Thu, Jan 9, 2025 at 7:59 PM Isaac J. Manjarres
<isaacmanjarres@google.com> 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>

This looks reasonable to me. One nit below, but:

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

>         fd_install(fd, file);
> +       /* name is not needed beyond this point. */
>         kfree(name);
>         return fd;

This comment seems superfluous at this point, since kfree(name) is the
last statement of the function.

Alice
Re: [PATCH v3 1/2] mm/memfd: Refactor and cleanup the logic in memfd_create()
Posted by Isaac Manjarres 1 year ago 
On Fri, Jan 10, 2025 at 10:12:13AM +0100, Alice Ryhl wrote:
> On Thu, Jan 9, 2025 at 7:59 PM Isaac J. Manjarres
> <isaacmanjarres@google.com> 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>
> 
> This looks reasonable to me. One nit below, but:
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> 
> >         fd_install(fd, file);
> > +       /* name is not needed beyond this point. */
> >         kfree(name);
> >         return fd;
> 
> This comment seems superfluous at this point, since kfree(name) is the
> last statement of the function.
> 
> Alice

Thanks! I'll go ahead and remove that and send out a new version.

--Isaac

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.