linux-kernel - Re: [PATCH v2 1/2] mm/memfd: Refactor and cleanup the logic in memfd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z37Gll66D47lY_fu@google.com>
Date: Wed, 8 Jan 2025 10:40:22 -0800
From: Isaac Manjarres <isaacmanjarres@...gle.com>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: lorenzo.stoakes@...cle.com, Andrew Morton <akpm@...ux-foundation.org>,
	kaleshsingh@...gle.com, jstultz@...gle.com, surenb@...gle.com,
	kernel-team@...roid.com, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] mm/memfd: Refactor and cleanup the logic in
 memfd_create()

On Wed, Jan 08, 2025 at 02:31:58PM +0100, Alice Ryhl wrote:
> On Tue, Jan 7, 2025 at 7:48 PM Isaac J. Manjarres
> <isaacmanjarres@...gle.com> wrote:
> > +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);
> 
> You changed the order so that get_unused_fd_flags() happens after
> creating the file, so the error path now does fput(file) instead of
> put_unused_fd(fd). Is there a reason for this? I would generally
> assume that calling get_unused_fd_flags() first is better.

Thanks for taking a look at this, Alice!

I changed the order so that the code had a more logical structure
where we create objects and use them right away, as opposed to getting
an fd, then creating the file to associate with that file descriptor,
and then actually associating it.

I also structured the code to get rid of the gotos in this function to
make it easier to follow. It also made sense to me to fold the flags
validation into memfd_file_create() since that's where the flags are
used the most anyway, and it also makes sense to validate the flags
first, so reordering the file creation and fd creation allowed me to
do that.

I'm open to restoring the order back to how it was though. Is there a
reason for why get_unused_fd_flags() first is better?

Thanks,
Isaac