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

[PATCH v1 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.

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 | 101 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 59 insertions(+), 42 deletions(-)

-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v1 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 check, name creation, and file creation into
their own helper functions, and create the file structure before
creating the memfd. This allows for simplifying the error handling path
in memfd_create().

No functional change.

Signed-off-by: Isaac J. Manjarres <isaacmanjarres@google.com>
---
 mm/memfd.c | 87 +++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 56 insertions(+), 31 deletions(-)

diff --git a/mm/memfd.c b/mm/memfd.c
index c17c3ea701a1..2372b9b1dc18 100644
--- a/mm/memfd.c
+++ b/mm/memfd.c
@@ -327,16 +327,8 @@ static int check_sysctl_memfd_noexec(unsigned int *flags)
 	return 0;
 }
 
-SYSCALL_DEFINE2(memfd_create,
-		const char __user *, uname,
-		unsigned int, flags)
+static int memfd_validate_flags(unsigned int flags)
 {
-	unsigned int *file_seals;
-	struct file *file;
-	int fd, error;
-	char *name;
-	long len;
-
 	if (!(flags & MFD_HUGETLB)) {
 		if (flags & ~(unsigned int)MFD_ALL_FLAGS)
 			return -EINVAL;
@@ -351,20 +343,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);
+}
+
+static char *memfd_create_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)) {
@@ -378,11 +375,22 @@ 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 *memfd_file_create(const char *name, unsigned int flags)
+{
+	unsigned int *file_seals;
+	struct file *file;
+	int error;
+
+	error = memfd_validate_flags(flags);
+	if (error < 0)
+		return ERR_PTR(error);
 
 	if (flags & MFD_HUGETLB) {
 		file = hugetlb_file_setup(name, 0, VM_NORESERVE,
@@ -391,10 +399,8 @@ SYSCALL_DEFINE2(memfd_create,
 					MFD_HUGE_MASK);
 	} 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;
 
@@ -414,13 +420,32 @@ SYSCALL_DEFINE2(memfd_create,
 			*file_seals &= ~F_SEAL_SEAL;
 	}
 
-	fd_install(fd, file);
-	kfree(name);
-	return fd;
+	return file;
+}
 
-err_fd:
-	put_unused_fd(fd);
-err_name:
+SYSCALL_DEFINE2(memfd_create,
+		const char __user *, uname,
+		unsigned int, flags)
+{
+	struct file *file;
+	int fd;
+	char *name;
+
+	name = memfd_create_name(uname);
+	if (IS_ERR(name))
+		return PTR_ERR(name);
+
+	file = memfd_file_create(name, flags);
+	/* name is not needed beyond this point. */
 	kfree(name);
-	return error;
+	if (IS_ERR(file))
+		return PTR_ERR(file);
+
+	fd = get_unused_fd_flags((flags & MFD_CLOEXEC) ? O_CLOEXEC : 0);
+	if (fd >= 0)
+		fd_install(fd, file);
+	else
+		fput(file);
+
+	return fd;
 }
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v1 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.

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 2372b9b1dc18..e14ac2c41fbb 100644
--- a/mm/memfd.c
+++ b/mm/memfd.c
@@ -352,26 +352,18 @@ static char *memfd_create_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(MFD_NAME_PREFIX_LEN + MFD_NAME_MAX_LEN + 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)) {
+	/* 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 v1 0/2] Cleanup for memfd_create()

[Lorenzo Stoakes]

On Thu, Jan 02, 2025 at 03:06:53PM -0800, Isaac J. Manjarres wrote:
> 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.
>

What is this against? I tried b4 shazam'ing it against mm-unstable and it
didn't apply. Could you rebase on mm-unstable?

Thanks!

> 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 | 101 +++++++++++++++++++++++++++++++----------------------
>  1 file changed, 59 insertions(+), 42 deletions(-)
>
> --
> 2.47.1.613.gc27f4b7a9f-goog
>

Re: [PATCH v1 0/2] Cleanup for memfd_create()

[Isaac Manjarres]

On Tue, Jan 07, 2025 at 03:09:49PM +0000, Lorenzo Stoakes wrote:
> On Thu, Jan 02, 2025 at 03:06:53PM -0800, Isaac J. Manjarres wrote:
> > 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.
> >
> 
> What is this against? I tried b4 shazam'ing it against mm-unstable and it
> didn't apply. Could you rebase on mm-unstable?
> 
> Thanks!
> 

Hi Lorenzo,

I had uploaded this against Linus' master branch. However, I've uploaded
a second version that is just a rebase on mm-unstable, as requested:
https://lore.kernel.org/all/20250107184804.4074147-1-isaacmanjarres@google.com/

Thanks,
Isaac