Re: Use of disowned struct filename after 3c5499fa56f5?

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

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.

> 
>> options I see
>> 1. take a reference on old/newname in the beginning.
>>
>> 2. don't return a filename from filename_parentat().
>> struct filename *name = ...;
>> int ret = filename_parentat(name, ...);
>> // use @name
>>
>> 3. (also ugly)
>> retry:
>> 	oldname = from; 
> 
> Not sure I follow - oldname == from, unless there's an error. Yes, this
> depends on filename_parentat() returning oldname or IS_ERR(), but that's
> how all the callers currently deal with it.

I think it's more explicit but still ugly, let's forget about the third


-- 
Pavel Begunkov