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.
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
