Re: Use of disowned struct filename after 3c5499fa56f5?

[Pavel Begunkov <asml.silence@gmail.com>]

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);
}


-- 
Pavel Begunkov