Re: [PATCH 1/2] fs: make do_mkdirat() take struct filename

[Dmitry Kadashev <dkadashev@gmail.com>]

On Wed, Jan 27, 2021 at 5:55 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Sun, Jan 24, 2021 at 09:38:19PM -0700, Jens Axboe wrote:
> > On 11/15/20 9:45 PM, Dmitry Kadashev wrote:
> > > Pass in the struct filename pointers instead of the user string, and
> > > update the three callers to do the same. This is heavily based on
> > > commit dbea8d345177 ("fs: make do_renameat2() take struct filename").
> > >
> > > This behaves like do_unlinkat() and do_renameat2().
> >
> > Al, are you OK with this patch? Leaving it quoted, though you should
> > have the original too.
>
> > > -static long do_mkdirat(int dfd, const char __user *pathname, umode_t mode)
> > > +long do_mkdirat(int dfd, struct filename *name, umode_t mode)
> > >  {
> > >     struct dentry *dentry;
> > >     struct path path;
> > >     int error;
> > >     unsigned int lookup_flags = LOOKUP_DIRECTORY;
> > >
> > > +   if (IS_ERR(name))
> > > +           return PTR_ERR(name);
> > > +
> > >  retry:
> > > -   dentry = user_path_create(dfd, pathname, &path, lookup_flags);
> > > -   if (IS_ERR(dentry))
> > > -           return PTR_ERR(dentry);
> > > +   name->refcnt++; /* filename_create() drops our ref */
> > > +   dentry = filename_create(dfd, name, &path, lookup_flags);
> > > +   if (IS_ERR(dentry)) {
> > > +           error = PTR_ERR(dentry);
> > > +           goto out;
> > > +   }
>
> No.  This is going to be a source of confusion from hell.  If anything,
> you want a variant of filename_create() that does not drop name on
> success.  With filename_create() itself being an inlined wrapper
> for it.

Hi Al,

I think I need more guidance here. First of all, I've based that code on
commit 7cdfa44227b0 ("vfs: Fix refcounting of filenames in fs_parser"), which
does exactly the same refcount bump in fs_parser.c for filename_lookup().  I'm
not saying it's a good excuse to introduce more code like that if that's a bad
code though.

What I _am_ saying is we probably want to make the approaches consistent (at
least eventually), which means we'd need the same "don't drop the name" variant
of filename_lookup? And given the fact filename_parentat (used from
filename_create) drops the name on error it looks like we'd need another copy of
it too? Do you think it's really worth it or maybe all of these functions will
make things more confusing? (from the looks of it right now the convention is
that the `struct filename` ownership is always transferred when it is passed as
an arg)

Also, do you have a good name for such functions that do not drop the name?

And, just for my education, can you explain why the reference counting for
struct filename exists if it's considered a bad practice to increase the
reference counter (assuming the cleanup code is correct)?

Thanks.

-- 
Dmitry Kadashev