Re: Use of disowned struct filename after 3c5499fa56f5?

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

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