[PATCH v3 0/2] Cleanup for memfd_create()

[PATCH v3 0/2] Cleanup for memfd_create()

[Isaac J. Manjarres]

memfd_create() handles all of its logic in a single function. Some of
the logic in the function is also somewhat contrived (i.e. copying the
memfd name from userpace).

This series aims to cleanup memfd_create() by splitting out the logic
into helper functions, and simplifying the memfd name copying to make
the code easier to follow.

This has no intended functional changes.

Note: to apply this series over mm-unstable, please revert v2 of
the series:

commit 729d8812ffd5 ("mm/memfd: Use strncpy_from_user() to read memfd name")
commit 24847e33d746 ("mm/memfd: Refactor and cleanup the logic in memfd_create()")

The base commit from mm-unstable that I used was:

commit 1cdadc307463 ("mm: compaction: skip memory compaction when there are not enough migratable pages")

Thank you Alice and Lorenzo for reviewing v2 of this series and for your
feedback!

Changes from v2 ==> v3:

- Removed namespacing from the new helper functions names.
- Updated names for helper functions to better describe what they do.
- Changed the argument to the flags sanitization function to be a
  pointer to not discard changes to the flags.
- Restored original ordering of how the fd and file structure were
  allocated.
- Simplified the constant used for allocating the buffer for the memfd
  name to NAME_MAX.
- Updated a comment about the return value of strncpy_from_user().
- Added Alice's "Reviewed-by" tag to the last patch in the series.

Changes from v1 ==> v2:

- Rebased on top of the mm-unstable branch instead of Linus' master
  branch. Base commit on mm-unstable: ca95745c20ad ("mm/memmap: prevent
  double scanning of memmap by kmemleak").

Links:

v1: https://lore.kernel.org/all/20250102230658.1112261-1-isaacmanjarres@google.com/#t
v2: https://lore.kernel.org/all/20250107184804.4074147-1-isaacmanjarres@google.com/

Isaac J. Manjarres (2):
  mm/memfd: Refactor and cleanup the logic in memfd_create()
  mm/memfd: Use strncpy_from_user() to read memfd name

 mm/memfd.c | 96 +++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 62 insertions(+), 34 deletions(-)

-- 
2.47.1.613.gc27f4b7a9f-goog

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

[Isaac J. Manjarres]

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

[PATCH v3 2/2] mm/memfd: Use strncpy_from_user() to read memfd name

[Isaac J. Manjarres]

The existing logic uses strnlen_user() to calculate the length of the
memfd name from userspace and then copies the string into a buffer using
copy_from_user(). This is error-prone, as the string length
could have changed between the time when it was calculated and when the
string was copied. The existing logic handles this by ensuring that the
last byte in the buffer is the terminating zero.

This handling is contrived and can better be handled by using
strncpy_from_user(), which gets the length of the string and copies
it in one shot. Therefore, simplify the logic for copying the memfd
name by using strncpy_from_user().

No functional change.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Isaac J. Manjarres <isaacmanjarres@google.com>
---
 mm/memfd.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/mm/memfd.c b/mm/memfd.c
index bf0c2d97b940..5b7c5892ba64 100644
--- a/mm/memfd.c
+++ b/mm/memfd.c
@@ -396,26 +396,18 @@ static char *alloc_name(const char __user *uname)
 	char *name;
 	long len;
 
-	/* length includes terminating zero */
-	len = strnlen_user(uname, MFD_NAME_MAX_LEN + 1);
-	if (len <= 0)
-		return ERR_PTR(-EFAULT);
-	if (len > MFD_NAME_MAX_LEN + 1)
-		return ERR_PTR(-EINVAL);
-
-	name = kmalloc(len + MFD_NAME_PREFIX_LEN, GFP_KERNEL);
+	name = kmalloc(NAME_MAX + 1, GFP_KERNEL);
 	if (!name)
 		return ERR_PTR(-ENOMEM);
 
 	strcpy(name, MFD_NAME_PREFIX);
-	if (copy_from_user(&name[MFD_NAME_PREFIX_LEN], uname, len)) {
+	/* returned length does not include terminating zero */
+	len = strncpy_from_user(&name[MFD_NAME_PREFIX_LEN], uname, MFD_NAME_MAX_LEN + 1);
+	if (len < 0) {
 		error = -EFAULT;
 		goto err_name;
-	}
-
-	/* terminating-zero may have changed after strnlen_user() returned */
-	if (name[len + MFD_NAME_PREFIX_LEN - 1]) {
-		error = -EFAULT;
+	} else if (len > MFD_NAME_MAX_LEN) {
+		error = -EINVAL;
 		goto err_name;
 	}
 
-- 
2.47.1.613.gc27f4b7a9f-goog

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

[Alice Ryhl]

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()

[Lorenzo Stoakes]

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 2/2] mm/memfd: Use strncpy_from_user() to read memfd name

[Lorenzo Stoakes]

On Thu, Jan 09, 2025 at 10:59:05AM -0800, Isaac J. Manjarres wrote:
> The existing logic uses strnlen_user() to calculate the length of the
> memfd name from userspace and then copies the string into a buffer using
> copy_from_user(). This is error-prone, as the string length
> could have changed between the time when it was calculated and when the
> string was copied. The existing logic handles this by ensuring that the
> last byte in the buffer is the terminating zero.
>
> This handling is contrived and can better be handled by using
> strncpy_from_user(), which gets the length of the string and copies
> it in one shot. Therefore, simplify the logic for copying the memfd
> name by using strncpy_from_user().
>
> No functional change.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Isaac J. Manjarres <isaacmanjarres@google.com>

LGTM,

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

> ---
>  mm/memfd.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/mm/memfd.c b/mm/memfd.c
> index bf0c2d97b940..5b7c5892ba64 100644
> --- a/mm/memfd.c
> +++ b/mm/memfd.c
> @@ -396,26 +396,18 @@ static char *alloc_name(const char __user *uname)
>  	char *name;
>  	long len;
>
> -	/* length includes terminating zero */
> -	len = strnlen_user(uname, MFD_NAME_MAX_LEN + 1);
> -	if (len <= 0)
> -		return ERR_PTR(-EFAULT);
> -	if (len > MFD_NAME_MAX_LEN + 1)
> -		return ERR_PTR(-EINVAL);
> -
> -	name = kmalloc(len + MFD_NAME_PREFIX_LEN, GFP_KERNEL);
> +	name = kmalloc(NAME_MAX + 1, GFP_KERNEL);
>  	if (!name)
>  		return ERR_PTR(-ENOMEM);
>
>  	strcpy(name, MFD_NAME_PREFIX);
> -	if (copy_from_user(&name[MFD_NAME_PREFIX_LEN], uname, len)) {
> +	/* returned length does not include terminating zero */
> +	len = strncpy_from_user(&name[MFD_NAME_PREFIX_LEN], uname, MFD_NAME_MAX_LEN + 1);
> +	if (len < 0) {
>  		error = -EFAULT;
>  		goto err_name;
> -	}
> -
> -	/* terminating-zero may have changed after strnlen_user() returned */
> -	if (name[len + MFD_NAME_PREFIX_LEN - 1]) {
> -		error = -EFAULT;
> +	} else if (len > MFD_NAME_MAX_LEN) {
> +		error = -EINVAL;
>  		goto err_name;
>  	}
>
> --
> 2.47.1.613.gc27f4b7a9f-goog
>

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

[Isaac Manjarres]

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

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

[Isaac Manjarres]

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 2/2] mm/memfd: Use strncpy_from_user() to read memfd name

[Isaac Manjarres]

On Fri, Jan 10, 2025 at 12:48:44PM +0000, Lorenzo Stoakes wrote:
> On Thu, Jan 09, 2025 at 10:59:05AM -0800, Isaac J. Manjarres wrote:
> > The existing logic uses strnlen_user() to calculate the length of the
> > memfd name from userspace and then copies the string into a buffer using
> > copy_from_user(). This is error-prone, as the string length
> > could have changed between the time when it was calculated and when the
> > string was copied. The existing logic handles this by ensuring that the
> > last byte in the buffer is the terminating zero.
> >
> > This handling is contrived and can better be handled by using
> > strncpy_from_user(), which gets the length of the string and copies
> > it in one shot. Therefore, simplify the logic for copying the memfd
> > name by using strncpy_from_user().
> >
> > No functional change.
> >
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > Signed-off-by: Isaac J. Manjarres <isaacmanjarres@google.com>
> 
> LGTM,
> 
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> 

Thank you!

--Isaac