Re: Use of disowned struct filename after 3c5499fa56f5?

[Dmitry Kadashev <dkadashev@gmail.com>]

On Thu, Nov 05, 2020 at 08:57:43PM +0000, Pavel Begunkov wrote:
> On 05/11/2020 20:49, Jens Axboe wrote:
> > On 11/5/20 1:35 PM, Pavel Begunkov wrote:
> >> On 05/11/2020 20:26, Jens Axboe wrote:
> >>> On 11/5/20 1:04 PM, Pavel Begunkov wrote:
> >>>> On 05/11/2020 19:37, Jens Axboe wrote:
> >>>>> On 11/5/20 7:55 AM, Pavel Begunkov wrote:
> >>>>>> On 05/11/2020 14:22, Pavel Begunkov wrote:
> >>>>>>> On 05/11/2020 12:36, Dmitry Kadashev wrote:
> >>>>>> Hah, basically filename_parentat() returns back the passed in filename if not
> >>>>>> an error, so @oldname and @from are aliased, then in the end for retry path
> >>>>>> it does.
> >>>>>>
> >>>>>> ```
> >>>>>> put(from);
> >>>>>> goto retry;
> >>>>>> ```
> >>>>>>
> >>>>>> And continues to use oldname. The same for to/newname.
> >>>>>> Looks buggy to me, good catch!
> >>>>>
> >>>>> How about we just cleanup the return path? We should only put these names
> >>>>> when we're done, not for the retry path. Something ala the below - untested,
> >>>>> I'll double check, test, and see if it's sane.
> >>>>
> >>>> Retry should work with a comment below because it uses @oldname
> >>>> knowing that it aliases to @from, which still have a refcount, but I
> >>>> don't like this implicit ref passing. If someone would change
> >>>> filename_parentat() to return a new filename, that would be a nasty
> >>>> bug.
> >>>
> >>> Not a huge fan of how that works either, but I'm not in this to rewrite
> >>> namei.c...
> >>
> >> There are 6 call sites including do_renameat2(), a separate patch would
> >> change just ~15-30 lines, doesn't seem like a big rewrite.
> > 
> > It just seems like an utterly pointless exercise to me, something you'd
> > go through IFF you're changing filename_parentat() to return a _new_
> > entry instead of just the same one. And given that this isn't the only
> > callsite, there's precedence there for it working like that. I'd
> > essentially just be writing useless code.
> > 
> > I can add a comment about it, but again, there are 6 other call sites.
> 
> Ok, but that's how things get broken. There is one more idea then,
> instead of keeping both oldname and from, just have from. May make
> the whole thing easier.
> 
> int do_renameat2(struct filename *from)
> {
> ...
> retry:
>     from = filename_parentat(from, ...);
> ...
> exit:
>     if (!IS_ERR(from))
>         putname(from);
> }

That's pretty much what do_unlinkat() does btw. Thanks Pavel for looking
into this!

Can I pick your brain some more? do_mkdirat() case is slightly
different:

static long do_mkdirat(int dfd, const char __user *pathname, umode_t mode)
{
	struct dentry *dentry;
	struct path path;
	int error;
	unsigned int lookup_flags = LOOKUP_DIRECTORY;

retry:
	dentry = user_path_create(dfd, pathname, &path, lookup_flags);

If we just change @pathname to struct filename, then user_path_create
can be swapped for filename_create(). But the same problem on retry
arises. Is there some more or less "idiomatic" way to solve this?

-- 
Dmitry