Re: Use of disowned struct filename after 3c5499fa56f5?

[Jens Axboe <axboe@kernel.dk>]

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:
>>> Hi Jens,
>>>
>>> I am trying to implement mkdirat support in io_uring and was using
>>> commit 3c5499fa56f5 ("fs: make do_renameat2() take struct filename") as
>>> an example (kernel newbie here). But either I do not understand how it
>>> works, or on retry struct filename is used that is not owned anymore
>>> (and is probably freed).
>>>
>>> Here is the relevant part of the patch:
>>>
>>> diff --git a/fs/namei.c b/fs/namei.c
>>> index d4a6dd772303..a696f99eef5c 100644
>>> --- a/fs/namei.c
>>> +++ b/fs/namei.c
>>> @@ -4346,8 +4346,8 @@ int vfs_rename(struct inode *old_dir, struct
>>> dentry *old_dentry,
>>>  }
>>>  EXPORT_SYMBOL(vfs_rename);
>>>
>>> -static int do_renameat2(int olddfd, const char __user *oldname, int newdfd,
>>> -                       const char __user *newname, unsigned int flags)
>>> +int do_renameat2(int olddfd, struct filename *oldname, int newdfd,
>>> +                struct filename *newname, unsigned int flags)
>>>  {
>>>         struct dentry *old_dentry, *new_dentry;
>>>         struct dentry *trap;
>>> @@ -4359,28 +4359,28 @@ static int do_renameat2(int olddfd, const char
>>> __user *oldname, int newdfd,
>>>         struct filename *to;
>>>         unsigned int lookup_flags = 0, target_flags = LOOKUP_RENAME_TARGET;
>>>         bool should_retry = false;
>>> -       int error;
>>> +       int error = -EINVAL;
>>>
>>>         if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE | RENAME_WHITEOUT))
>>> -               return -EINVAL;
>>> +               goto put_both;
>>>
>>>         if ((flags & (RENAME_NOREPLACE | RENAME_WHITEOUT)) &&
>>>             (flags & RENAME_EXCHANGE))
>>> -               return -EINVAL;
>>> +               goto put_both;
>>>
>>>         if (flags & RENAME_EXCHANGE)
>>>                 target_flags = 0;
>>>
>>>  retry:
>>> -       from = filename_parentat(olddfd, getname(oldname), lookup_flags,
>>> -                               &old_path, &old_last, &old_type);
>>
>> filename_parentat(getname(oldname), ...)
>>
>> It's passing a filename directly, so filename_parentat() also takes ownership
>> of the passed filename together with responsibility to put it. Yes, it should
>> be destroying it inside.
> 
> 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.


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:
 	if (!IS_ERR(oldname))
 		putname(oldname);

-- 
Jens Axboe