Re: Use of disowned struct filename after 3c5499fa56f5?

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

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.

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

-- 
Pavel Begunkov