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

[Isaac Manjarres <isaacmanjarres@google.com>]

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