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
