Re: Use of disowned struct filename after 3c5499fa56f5?

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

On 05/11/2020 20:04, 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.
> 
> 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; 
> 
>>
>>
>> diff --git a/fs/namei.c b/fs/namei.c
>> index a696f99eef5c..becb23ec07a8 100644
>> --- a/fs/namei.c
>> +++ b/fs/namei.c
>> @@ -4473,16 +4473,13 @@ int do_renameat2(int olddfd, struct filename *oldname, int newdfd,
>>  	if (retry_estale(error, lookup_flags))
>>  		should_retry = true;
>>  	path_put(&new_path);
>> -	putname(to);
>>  exit1:
>>  	path_put(&old_path);
>> -	putname(from);
>>  	if (should_retry) {	
>>  		should_retry = false;
>>  		lookup_flags |= LOOKUP_REVAL;
>>  		goto retry;
>>  	}
>> -	return error;
>>  put_both:
> 
> I don't see oldname to be cleared after filename_parentat(),
> so it puts both @from and @oldname, but there is only 1 ref.

I'm wrong here, you don't put @from.

Still filename_parentat() may fail, put oldname inside, destroy
it and return an error, but as we don't clear oldname put_both:
and below would do putname(oldname) again.

> 
>>  	if (!IS_ERR(oldname))
>>  		putname(oldname);
>>
> 

-- 
Pavel Begunkov