Re: Use of disowned struct filename after 3c5499fa56f5?

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

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!

p.s. just noticed that you listed the original patch, not yours

> 
> struct filename {
> 	...
> 	int			refcnt;
> };
> 
> The easiest solution is to take an additional ref. Looks like it's not atomic,
> but double check to not add additional overhead.
> 
>> +       from = filename_parentat(olddfd, oldname, lookup_flags, &old_path,
>> +                                       &old_last, &old_type);
>>
>> With the new code on the first run oldname ownership is released. And if
>> we do end up on the retry path then it is used again erroneously (also
>> `from` was already put by that time).
>>
>> Am I getting it wrong or is there a bug?
>>
>> do_unlinkat that you reference does things a bit differently, as far as
>> I can tell the problem does not exist there.
>>
>> Thanks,
>> Dmitry
>>
> 

-- 
Pavel Begunkov