Re: Use of disowned struct filename after 3c5499fa56f5?

[Jens Axboe <axboe@kernel.dk>]

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...

> 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.

-- 
Jens Axboe