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
