Re: Use of disowned struct filename after 3c5499fa56f5?

[Jens Axboe <axboe@kernel.dk>]

On 11/5/20 1:57 PM, 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.

I'm not arguing it's great code, just saying that's how it already works...

> 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 not a bad idea, and eliminates the extra variables. I'll add that.
Need to get this sent out for review soonish anyway.

-- 
Jens Axboe