[PATCH] fs: fix possible extra iput() in do_unlinkat()

[PATCH] fs: fix possible extra iput() in do_unlinkat()

[Luís Henriques]

Because inode is being initialised before checking if dentry is negative,
and the ihold() is only done if the dentry is *not* negative, the cleanup
code may end-up doing an extra iput() on that inode.

Fixes: b18825a7c8e3 ("VFS: Put a small type field into struct dentry::d_flags")
Signed-off-by: Luís Henriques <lhenriques@suse.de>
---
Hi!

I was going to also remove the 'if (inode)' before the 'iput(inode)',
because 'iput()' already checks for NULL anyway.  But since I probably
wouldn't have caught this bug if it wasn't for that 'if', I decided to
keep it there.  But I can send v2 with that change too if you prefer.

Cheers,
--
Luís

 fs/namei.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 567ee547492b..156a570d7831 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4386,11 +4386,9 @@ int do_unlinkat(int dfd, struct filename *name)
 	if (!IS_ERR(dentry)) {
 
 		/* Why not before? Because we want correct error value */
-		if (last.name[last.len])
+		if (last.name[last.len] || d_is_negative(dentry))
 			goto slashes;
 		inode = dentry->d_inode;
-		if (d_is_negative(dentry))
-			goto slashes;
 		ihold(inode);
 		error = security_path_unlink(&path, dentry);
 		if (error)

Re: [PATCH] fs: fix possible extra iput() in do_unlinkat()

[Mateusz Guzik]

On Thu, Sep 28, 2023 at 02:11:29PM +0100, Luís Henriques wrote:
> Because inode is being initialised before checking if dentry is negative,
> and the ihold() is only done if the dentry is *not* negative, the cleanup
> code may end-up doing an extra iput() on that inode.
> 
> Fixes: b18825a7c8e3 ("VFS: Put a small type field into struct dentry::d_flags")
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
> Hi!
> 
> I was going to also remove the 'if (inode)' before the 'iput(inode)',
> because 'iput()' already checks for NULL anyway.  But since I probably
> wouldn't have caught this bug if it wasn't for that 'if', I decided to
> keep it there.  But I can send v2 with that change too if you prefer.
> 
> Cheers,
> --
> Luís
> 
>  fs/namei.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 567ee547492b..156a570d7831 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4386,11 +4386,9 @@ int do_unlinkat(int dfd, struct filename *name)
>  	if (!IS_ERR(dentry)) {
>  
>  		/* Why not before? Because we want correct error value */
> -		if (last.name[last.len])
> +		if (last.name[last.len] || d_is_negative(dentry))
>  			goto slashes;
>  		inode = dentry->d_inode;
> -		if (d_is_negative(dentry))
> -			goto slashes;
>  		ihold(inode);
>  		error = security_path_unlink(&path, dentry);
>  		if (error)

I ran into this myself, but I'm pretty sure there is no bug here. The
code is just incredibly misleading and it became this way from the
sweeping change introducing d_is_negative. I could not be bothered to
argue about patching it so I did not do anything. ;)

AFAICS it is an invariant that d_is_negative passes iff d_inode is NULL.

Personally I support the patch, but commit message needs to stop
claiming a bug.

Re: [PATCH] fs: fix possible extra iput() in do_unlinkat()

[Christian Brauner]

On Thu, 28 Sep 2023 14:11:29 +0100, Luís Henriques wrote:
> Because inode is being initialised before checking if dentry is negative,
> and the ihold() is only done if the dentry is *not* negative, the cleanup
> code may end-up doing an extra iput() on that inode.
> 
> 

Not a bug afaict but as Mateusz points out it's confusing.
So I took it with a changed commit message:

    fs: move d_is_negative() further up

    The code is confusing because inode is dereferenced before the
    d_is_negative() check. Make it clear that iput() isn't called further
    below.

---

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc

[1/1] fs: fix possible extra iput() in do_unlinkat()
      https://git.kernel.org/vfs/vfs/c/b99cf757431d

Re: [PATCH] fs: fix possible extra iput() in do_unlinkat()

[Luís Henriques]

Mateusz Guzik <mjguzik@gmail.com> writes:

> On Thu, Sep 28, 2023 at 02:11:29PM +0100, Luís Henriques wrote:
>> Because inode is being initialised before checking if dentry is negative,
>> and the ihold() is only done if the dentry is *not* negative, the cleanup
>> code may end-up doing an extra iput() on that inode.
>> 
>> Fixes: b18825a7c8e3 ("VFS: Put a small type field into struct dentry::d_flags")
>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>> ---
>> Hi!
>> 
>> I was going to also remove the 'if (inode)' before the 'iput(inode)',
>> because 'iput()' already checks for NULL anyway.  But since I probably
>> wouldn't have caught this bug if it wasn't for that 'if', I decided to
>> keep it there.  But I can send v2 with that change too if you prefer.
>> 
>> Cheers,
>> --
>> Luís
>> 
>>  fs/namei.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>> 
>> diff --git a/fs/namei.c b/fs/namei.c
>> index 567ee547492b..156a570d7831 100644
>> --- a/fs/namei.c
>> +++ b/fs/namei.c
>> @@ -4386,11 +4386,9 @@ int do_unlinkat(int dfd, struct filename *name)
>>  	if (!IS_ERR(dentry)) {
>>  
>>  		/* Why not before? Because we want correct error value */
>> -		if (last.name[last.len])
>> +		if (last.name[last.len] || d_is_negative(dentry))
>>  			goto slashes;
>>  		inode = dentry->d_inode;
>> -		if (d_is_negative(dentry))
>> -			goto slashes;
>>  		ihold(inode);
>>  		error = security_path_unlink(&path, dentry);
>>  		if (error)
>
> I ran into this myself, but I'm pretty sure there is no bug here. The
> code is just incredibly misleading and it became this way from the
> sweeping change introducing d_is_negative. I could not be bothered to
> argue about patching it so I did not do anything. ;)
>
> AFAICS it is an invariant that d_is_negative passes iff d_inode is NULL.

Ah! yes, of course.  Makes sense.  Thanks for your review.

> Personally I support the patch, but commit message needs to stop
> claiming a bug.

OK, I'll rephrase the commit message for v2 so that it's clear it's not
really fixing anything, it just helps clarifying some misleading code
paths.  (Although, to be fair, the commit message doesn't really
explicitly call it a bug :-) ).

Cheers,
-- 
Luís

Re: [PATCH] fs: fix possible extra iput() in do_unlinkat()

[Christian Brauner]

> OK, I'll rephrase the commit message for v2 so that it's clear it's not

I've already applied your first pach and massaged your commit message.
Authorship and all is retained. No need to resend. Probably got lost
because git send-email scrambled your mail address.

Re: [PATCH] fs: fix possible extra iput() in do_unlinkat()

[Luís Henriques]

Christian Brauner <brauner@kernel.org> writes:

>> OK, I'll rephrase the commit message for v2 so that it's clear it's not
>
> I've already applied your first pach and massaged your commit message.
> Authorship and all is retained. No need to resend.

Awesome, thanks!

> Probably got lost because git send-email scrambled your mail address.

Hmm... yeah, I should probably drop non-ascii chars from my email address.

Cheers,
-- 
Luís