Use of disowned struct filename after 3c5499fa56f5?

Use of disowned struct filename after 3c5499fa56f5?

[Dmitry Kadashev]

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);
+       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

Re: Use of disowned struct filename after 3c5499fa56f5?

[Pavel Begunkov]

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

Re: Use of disowned struct filename after 3c5499fa56f5?

[Pavel Begunkov]

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

BTW, I'll double check later today internals and the patch you've mentioned.

-- 
Pavel Begunkov

Re: Use of disowned struct filename after 3c5499fa56f5?

[Pavel Begunkov]

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

Re: Use of disowned struct filename after 3c5499fa56f5?

[Jens Axboe]

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

Re: Use of disowned struct filename after 3c5499fa56f5?

[Pavel Begunkov]

On 05/11/2020 19:37, Jens Axboe wrote:
> 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:
>> 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.

Retry should work with a comment below because it uses @oldname knowing that
it aliases to @from, which still have a refcount, but I don't like this
implicit ref passing. If someone would change filename_parentat() to return
a new filename, that would be a nasty bug.

options I see
1. take a reference on old/newname in the beginning.

2. don't return a filename from filename_parentat().
struct filename *name = ...;
int ret = filename_parentat(name, ...);
// use @name

3. (also ugly)
retry:
	oldname = from; 

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

I don't see oldname to be cleared after filename_parentat(),
so it puts both @from and @oldname, but there is only 1 ref.

>  	if (!IS_ERR(oldname))
>  		putname(oldname);
> 

-- 
Pavel Begunkov

Re: Use of disowned struct filename after 3c5499fa56f5?

[Pavel Begunkov]

On 05/11/2020 20:04, Pavel Begunkov wrote:
> On 05/11/2020 19:37, Jens Axboe wrote:
>> 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:
>>> 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.
> 
> Retry should work with a comment below because it uses @oldname knowing that
> it aliases to @from, which still have a refcount, but I don't like this
> implicit ref passing. If someone would change filename_parentat() to return
> a new filename, that would be a nasty bug.
> 
> options I see
> 1. take a reference on old/newname in the beginning.
> 
> 2. don't return a filename from filename_parentat().
> struct filename *name = ...;
> int ret = filename_parentat(name, ...);
> // use @name
> 
> 3. (also ugly)
> retry:
> 	oldname = from; 
> 
>>
>>
>> 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:
> 
> I don't see oldname to be cleared after filename_parentat(),
> so it puts both @from and @oldname, but there is only 1 ref.

I'm wrong here, you don't put @from.

Still filename_parentat() may fail, put oldname inside, destroy
it and return an error, but as we don't clear oldname put_both:
and below would do putname(oldname) again.

> 
>>  	if (!IS_ERR(oldname))
>>  		putname(oldname);
>>
> 

-- 
Pavel Begunkov

Re: Use of disowned struct filename after 3c5499fa56f5?

[Jens Axboe]

On 11/5/20 1:04 PM, Pavel Begunkov wrote:
> On 05/11/2020 19:37, Jens Axboe wrote:
>> 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:
>>> 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.
> 
> Retry should work with a comment below because it uses @oldname
> knowing that it aliases to @from, which still have a refcount, but I
> don't like this implicit ref passing. If someone would change
> filename_parentat() to return a new filename, that would be a nasty
> bug.

Not a huge fan of how that works either, but I'm not in this to rewrite
namei.c...

> options I see
> 1. take a reference on old/newname in the beginning.
> 
> 2. don't return a filename from filename_parentat().
> struct filename *name = ...;
> int ret = filename_parentat(name, ...);
> // use @name
> 
> 3. (also ugly)
> retry:
> 	oldname = from; 

Not sure I follow - oldname == from, unless there's an error. Yes, this
depends on filename_parentat() returning oldname or IS_ERR(), but that's
how all the callers currently deal with it.

-- 
Jens Axboe

Re: Use of disowned struct filename after 3c5499fa56f5?

[Pavel Begunkov]

On 05/11/2020 20:26, Jens Axboe wrote:
> On 11/5/20 1:04 PM, Pavel Begunkov wrote:
>> On 05/11/2020 19:37, Jens Axboe wrote:
>>> 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:
>>>> 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.
>>
>> Retry should work with a comment below because it uses @oldname
>> knowing that it aliases to @from, which still have a refcount, but I
>> don't like this implicit ref passing. If someone would change
>> filename_parentat() to return a new filename, that would be a nasty
>> bug.
> 
> Not a huge fan of how that works either, but I'm not in this to rewrite
> namei.c...

There are 6 call sites including do_renameat2(), a separate patch would
change just ~15-30 lines, doesn't seem like a big rewrite.

> 
>> options I see
>> 1. take a reference on old/newname in the beginning.
>>
>> 2. don't return a filename from filename_parentat().
>> struct filename *name = ...;
>> int ret = filename_parentat(name, ...);
>> // use @name
>>
>> 3. (also ugly)
>> retry:
>> 	oldname = from; 
> 
> Not sure I follow - oldname == from, unless there's an error. Yes, this
> depends on filename_parentat() returning oldname or IS_ERR(), but that's
> how all the callers currently deal with it.

I think it's more explicit but still ugly, let's forget about the third


-- 
Pavel Begunkov

Re: Use of disowned struct filename after 3c5499fa56f5?

[Jens Axboe]

On 11/5/20 1:35 PM, Pavel Begunkov wrote:
> On 05/11/2020 20:26, Jens Axboe wrote:
>> On 11/5/20 1:04 PM, Pavel Begunkov wrote:
>>> On 05/11/2020 19:37, Jens Axboe wrote:
>>>> 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:
>>>>> 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.
>>>
>>> Retry should work with a comment below because it uses @oldname
>>> knowing that it aliases to @from, which still have a refcount, but I
>>> don't like this implicit ref passing. If someone would change
>>> filename_parentat() to return a new filename, that would be a nasty
>>> bug.
>>
>> Not a huge fan of how that works either, but I'm not in this to rewrite
>> namei.c...
> 
> There are 6 call sites including do_renameat2(), a separate patch would
> change just ~15-30 lines, doesn't seem like a big rewrite.

It just seems like an utterly pointless exercise to me, something you'd
go through IFF you're changing filename_parentat() to return a _new_
entry instead of just the same one. And given that this isn't the only
callsite, there's precedence there for it working like that. I'd
essentially just be writing useless code.

I can add a comment about it, but again, there are 6 other call sites.

-- 
Jens Axboe

Re: Use of disowned struct filename after 3c5499fa56f5?

[Pavel Begunkov]

On 05/11/2020 20:49, Jens Axboe wrote:
> On 11/5/20 1:35 PM, Pavel Begunkov wrote:
>> On 05/11/2020 20:26, Jens Axboe wrote:
>>> On 11/5/20 1:04 PM, Pavel Begunkov wrote:
>>>> On 05/11/2020 19:37, Jens Axboe wrote:
>>>>> 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:
>>>>>> 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.
>>>>
>>>> Retry should work with a comment below because it uses @oldname
>>>> knowing that it aliases to @from, which still have a refcount, but I
>>>> don't like this implicit ref passing. If someone would change
>>>> filename_parentat() to return a new filename, that would be a nasty
>>>> bug.
>>>
>>> Not a huge fan of how that works either, but I'm not in this to rewrite
>>> namei.c...
>>
>> There are 6 call sites including do_renameat2(), a separate patch would
>> change just ~15-30 lines, doesn't seem like a big rewrite.
> 
> It just seems like an utterly pointless exercise to me, something you'd
> go through IFF you're changing filename_parentat() to return a _new_
> entry instead of just the same one. And given that this isn't the only
> callsite, there's precedence there for it working like that. I'd
> essentially just be writing useless code.
> 
> I can add a comment about it, but again, there are 6 other call sites.

Ok, but that's how things get broken. There is one more idea then,
instead of keeping both oldname and from, just have from. May make
the whole thing easier.

int do_renameat2(struct filename *from)
{
...
retry:
    from = filename_parentat(from, ...);
...
exit:
    if (!IS_ERR(from))
        putname(from);
}


-- 
Pavel Begunkov

Re: Use of disowned struct filename after 3c5499fa56f5?

[Jens Axboe]

On 11/5/20 1:57 PM, Pavel Begunkov wrote:
> On 05/11/2020 20:49, Jens Axboe wrote:
>> On 11/5/20 1:35 PM, Pavel Begunkov wrote:
>>> On 05/11/2020 20:26, Jens Axboe wrote:
>>>> On 11/5/20 1:04 PM, Pavel Begunkov wrote:
>>>>> On 05/11/2020 19:37, Jens Axboe wrote:
>>>>>> 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:
>>>>>>> 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.
>>>>>
>>>>> Retry should work with a comment below because it uses @oldname
>>>>> knowing that it aliases to @from, which still have a refcount, but I
>>>>> don't like this implicit ref passing. If someone would change
>>>>> filename_parentat() to return a new filename, that would be a nasty
>>>>> bug.
>>>>
>>>> Not a huge fan of how that works either, but I'm not in this to rewrite
>>>> namei.c...
>>>
>>> There are 6 call sites including do_renameat2(), a separate patch would
>>> change just ~15-30 lines, doesn't seem like a big rewrite.
>>
>> It just seems like an utterly pointless exercise to me, something you'd
>> go through IFF you're changing filename_parentat() to return a _new_
>> entry instead of just the same one. And given that this isn't the only
>> callsite, there's precedence there for it working like that. I'd
>> essentially just be writing useless code.
>>
>> I can add a comment about it, but again, there are 6 other call sites.
> 
> Ok, but that's how things get broken.

I'm not arguing it's great code, just saying that's how it already works...

> There is one more idea then,
> instead of keeping both oldname and from, just have from. May make
> the whole thing easier.
> 
> int do_renameat2(struct filename *from)
> {
> ...
> retry:
>     from = filename_parentat(from, ...);
> ...
> exit:
>     if (!IS_ERR(from))
>         putname(from);
> }

That's not a bad idea, and eliminates the extra variables. I'll add that.
Need to get this sent out for review soonish anyway.

-- 
Jens Axboe

Re: Use of disowned struct filename after 3c5499fa56f5?

[Dmitry Kadashev]

On Thu, Nov 05, 2020 at 08:57:43PM +0000, Pavel Begunkov wrote:
> On 05/11/2020 20:49, Jens Axboe wrote:
> > On 11/5/20 1:35 PM, Pavel Begunkov wrote:
> >> On 05/11/2020 20:26, Jens Axboe wrote:
> >>> On 11/5/20 1:04 PM, Pavel Begunkov wrote:
> >>>> On 05/11/2020 19:37, Jens Axboe wrote:
> >>>>> 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:
> >>>>>> 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.
> >>>>
> >>>> Retry should work with a comment below because it uses @oldname
> >>>> knowing that it aliases to @from, which still have a refcount, but I
> >>>> don't like this implicit ref passing. If someone would change
> >>>> filename_parentat() to return a new filename, that would be a nasty
> >>>> bug.
> >>>
> >>> Not a huge fan of how that works either, but I'm not in this to rewrite
> >>> namei.c...
> >>
> >> There are 6 call sites including do_renameat2(), a separate patch would
> >> change just ~15-30 lines, doesn't seem like a big rewrite.
> > 
> > It just seems like an utterly pointless exercise to me, something you'd
> > go through IFF you're changing filename_parentat() to return a _new_
> > entry instead of just the same one. And given that this isn't the only
> > callsite, there's precedence there for it working like that. I'd
> > essentially just be writing useless code.
> > 
> > I can add a comment about it, but again, there are 6 other call sites.
> 
> Ok, but that's how things get broken. There is one more idea then,
> instead of keeping both oldname and from, just have from. May make
> the whole thing easier.
> 
> int do_renameat2(struct filename *from)
> {
> ...
> retry:
>     from = filename_parentat(from, ...);
> ...
> exit:
>     if (!IS_ERR(from))
>         putname(from);
> }

That's pretty much what do_unlinkat() does btw. Thanks Pavel for looking
into this!

Can I pick your brain some more? do_mkdirat() case is slightly
different:

static long do_mkdirat(int dfd, const char __user *pathname, umode_t mode)
{
	struct dentry *dentry;
	struct path path;
	int error;
	unsigned int lookup_flags = LOOKUP_DIRECTORY;

retry:
	dentry = user_path_create(dfd, pathname, &path, lookup_flags);

If we just change @pathname to struct filename, then user_path_create
can be swapped for filename_create(). But the same problem on retry
arises. Is there some more or less "idiomatic" way to solve this?

-- 
Dmitry

Re: Use of disowned struct filename after 3c5499fa56f5?

[Pavel Begunkov]

On 06/11/2020 10:08, Dmitry Kadashev wrote:
> On Thu, Nov 05, 2020 at 08:57:43PM +0000, Pavel Begunkov wrote:
> That's pretty much what do_unlinkat() does btw. Thanks Pavel for looking
> into this!
> 
> Can I pick your brain some more? do_mkdirat() case is slightly
> different:
> 
> static long do_mkdirat(int dfd, const char __user *pathname, umode_t mode)
> {
> 	struct dentry *dentry;
> 	struct path path;
> 	int error;
> 	unsigned int lookup_flags = LOOKUP_DIRECTORY;
> 
> retry:
> 	dentry = user_path_create(dfd, pathname, &path, lookup_flags);
> 
> If we just change @pathname to struct filename, then user_path_create
> can be swapped for filename_create(). But the same problem on retry
> arises. Is there some more or less "idiomatic" way to solve this?

I don't think there is, fs guys may have a different opinion but
sometimes it's hard to get through them.

I'd take a filename reference before "retry:" and putname(filename) on
exit. Skimmed quickly and looks like nobody changes it anywhere in
user_path_create(). Since you're doing it for io_uring and so going to
push it through Jens's tree it's better to keep it small and localised
in do_mkdirat() to not cause merging troubles.

-- 
Pavel Begunkov

Re: Use of disowned struct filename after 3c5499fa56f5?

[Dmitry Kadashev]

On Fri, Nov 06, 2020 at 12:49:05PM +0000, Pavel Begunkov wrote:
> On 06/11/2020 10:08, Dmitry Kadashev wrote:
> > On Thu, Nov 05, 2020 at 08:57:43PM +0000, Pavel Begunkov wrote:
> > That's pretty much what do_unlinkat() does btw. Thanks Pavel for looking
> > into this!
> > 
> > Can I pick your brain some more? do_mkdirat() case is slightly
> > different:
> > 
> > static long do_mkdirat(int dfd, const char __user *pathname, umode_t mode)
> > {
> > 	struct dentry *dentry;
> > 	struct path path;
> > 	int error;
> > 	unsigned int lookup_flags = LOOKUP_DIRECTORY;
> > 
> > retry:
> > 	dentry = user_path_create(dfd, pathname, &path, lookup_flags);
> > 
> > If we just change @pathname to struct filename, then user_path_create
> > can be swapped for filename_create(). But the same problem on retry
> > arises. Is there some more or less "idiomatic" way to solve this?
> 
> I don't think there is, fs guys may have a different opinion but
> sometimes it's hard to get through them.
> 
> I'd take a filename reference before "retry:"

How do I do that? Just `++name.refcnt` or is there a helper function /
better way?

-- 
Dmitry

Re: Use of disowned struct filename after 3c5499fa56f5?

[Pavel Begunkov]

On 06/11/2020 13:15, Dmitry Kadashev wrote:
> On Fri, Nov 06, 2020 at 12:49:05PM +0000, Pavel Begunkov wrote:
>> On 06/11/2020 10:08, Dmitry Kadashev wrote:
>>> On Thu, Nov 05, 2020 at 08:57:43PM +0000, Pavel Begunkov wrote:
>>> That's pretty much what do_unlinkat() does btw. Thanks Pavel for looking
>>> into this!
>>>
>>> Can I pick your brain some more? do_mkdirat() case is slightly
>>> different:
>>>
>>> static long do_mkdirat(int dfd, const char __user *pathname, umode_t mode)
>>> {
>>> 	struct dentry *dentry;
>>> 	struct path path;
>>> 	int error;
>>> 	unsigned int lookup_flags = LOOKUP_DIRECTORY;
>>>
>>> retry:
>>> 	dentry = user_path_create(dfd, pathname, &path, lookup_flags);
>>>
>>> If we just change @pathname to struct filename, then user_path_create
>>> can be swapped for filename_create(). But the same problem on retry
>>> arises. Is there some more or less "idiomatic" way to solve this?
>>
>> I don't think there is, fs guys may have a different opinion but
>> sometimes it's hard to get through them.
>>
>> I'd take a filename reference before "retry:"
> 
> How do I do that? Just `++name.refcnt` or is there a helper function /
> better way?

I don't know, take a look around if there is one. In the end, a review and
guys familiar with this code will hopefully suggest a better way (if any).

-- 
Pavel Begunkov

Re: Use of disowned struct filename after 3c5499fa56f5?

[Dmitry Kadashev]

On Fri, Nov 06, 2020 at 01:27:51PM +0000, Pavel Begunkov wrote:
> On 06/11/2020 13:15, Dmitry Kadashev wrote:
> > On Fri, Nov 06, 2020 at 12:49:05PM +0000, Pavel Begunkov wrote:
> >> On 06/11/2020 10:08, Dmitry Kadashev wrote:
> >>> On Thu, Nov 05, 2020 at 08:57:43PM +0000, Pavel Begunkov wrote:
> >>> That's pretty much what do_unlinkat() does btw. Thanks Pavel for looking
> >>> into this!
> >>>
> >>> Can I pick your brain some more? do_mkdirat() case is slightly
> >>> different:
> >>>
> >>> static long do_mkdirat(int dfd, const char __user *pathname, umode_t mode)
> >>> {
> >>> 	struct dentry *dentry;
> >>> 	struct path path;
> >>> 	int error;
> >>> 	unsigned int lookup_flags = LOOKUP_DIRECTORY;
> >>>
> >>> retry:
> >>> 	dentry = user_path_create(dfd, pathname, &path, lookup_flags);
> >>>
> >>> If we just change @pathname to struct filename, then user_path_create
> >>> can be swapped for filename_create(). But the same problem on retry
> >>> arises. Is there some more or less "idiomatic" way to solve this?
> >>
> >> I don't think there is, fs guys may have a different opinion but
> >> sometimes it's hard to get through them.
> >>
> >> I'd take a filename reference before "retry:"
> > 
> > How do I do that? Just `++name.refcnt` or is there a helper function /
> > better way?
> 
> I don't know, take a look around if there is one. In the end, a review and
> guys familiar with this code will hopefully suggest a better way (if any).

OK, thanks Pavel! I'll look around.

-- 
Dmitry