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
