[bug report] fs: Restrict lock_two_nondirectories() to non-directory inodes

[bug report] fs: Restrict lock_two_nondirectories() to non-directory inodes

[Dan Carpenter]

Hello Jan Kara,

This is a semi-automatic email about new static checker warnings.

The patch afb4adc7c3ef: "fs: Restrict lock_two_nondirectories() to
non-directory inodes" from Jun 1, 2023, leads to the following Smatch
complaint:

    fs/inode.c:1174 unlock_two_nondirectories()
    warn: variable dereferenced before check 'inode1' (see line 1172)

    fs/inode.c:1176 unlock_two_nondirectories()
    warn: variable dereferenced before check 'inode2' (see line 1173)

fs/inode.c
  1171	{
  1172		WARN_ON_ONCE(S_ISDIR(inode1->i_mode));
                                     ^^^^^^^^^^^^^^
  1173		WARN_ON_ONCE(S_ISDIR(inode2->i_mode));
  1174		if (inode1)
                    ^^^^^^
  1175			inode_unlock(inode1);
  1176		if (inode2 && inode2 != inode1)
                    ^^^^^
regards,
dan carpenter

Re: [bug report] fs: Restrict lock_two_nondirectories() to non-directory inodes

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 816 bytes --]

On Tue 06-06-23 11:32:53, Dan Carpenter wrote:
> Hello Jan Kara,
> 
> This is a semi-automatic email about new static checker warnings.
> 
> The patch afb4adc7c3ef: "fs: Restrict lock_two_nondirectories() to
> non-directory inodes" from Jun 1, 2023, leads to the following Smatch
> complaint:
> 
>     fs/inode.c:1174 unlock_two_nondirectories()
>     warn: variable dereferenced before check 'inode1' (see line 1172)
> 
>     fs/inode.c:1176 unlock_two_nondirectories()
>     warn: variable dereferenced before check 'inode2' (see line 1173)

Indeed, thanks for spotting this! Luckily there are currently no in-tree
users passing NULL. Attached patch fixes this. Christian, can you please
add this to your branch or squash it into the fixed commit? Thanks!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[-- Attachment #2: 0001-fs-Fixup-unlock_two_nondirectories-for-NULL-inodes.patch --]
[-- Type: text/x-patch, Size: 1259 bytes --]

From 4afde047fb4408553b9d2c548ebe355db7c95f0f Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Tue, 6 Jun 2023 11:52:33 +0200
Subject: [PATCH] fs: Fixup unlock_two_nondirectories() for NULL inodes

When inode is NULL, we cannot WARN on S_ISDIR(inode->i_mode) as that
would be a NULL ptr dereference. Move the warnings below the NULL
checks.

Fixes: afb4adc7c3ef ("fs: Restrict lock_two_nondirectories() to non-directory inodes")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/inode.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index e2707ee88459..53ae3b76d232 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1169,12 +1169,14 @@ EXPORT_SYMBOL(lock_two_nondirectories);
  */
 void unlock_two_nondirectories(struct inode *inode1, struct inode *inode2)
 {
-	WARN_ON_ONCE(S_ISDIR(inode1->i_mode));
-	WARN_ON_ONCE(S_ISDIR(inode2->i_mode));
-	if (inode1)
+	if (inode1) {
+		WARN_ON_ONCE(S_ISDIR(inode1->i_mode));
 		inode_unlock(inode1);
-	if (inode2 && inode2 != inode1)
+	}
+	if (inode2 && inode2 != inode1) {
+		WARN_ON_ONCE(S_ISDIR(inode2->i_mode));
 		inode_unlock(inode2);
+	}
 }
 EXPORT_SYMBOL(unlock_two_nondirectories);
 
-- 
2.35.3

Re: [bug report] fs: Restrict lock_two_nondirectories() to non-directory inodes

[Christian Brauner]

On Tue, Jun 06, 2023 at 11:56:25AM +0200, Jan Kara wrote:
> On Tue 06-06-23 11:32:53, Dan Carpenter wrote:
> > Hello Jan Kara,
> > 
> > This is a semi-automatic email about new static checker warnings.
> > 
> > The patch afb4adc7c3ef: "fs: Restrict lock_two_nondirectories() to
> > non-directory inodes" from Jun 1, 2023, leads to the following Smatch
> > complaint:
> > 
> >     fs/inode.c:1174 unlock_two_nondirectories()
> >     warn: variable dereferenced before check 'inode1' (see line 1172)
> > 
> >     fs/inode.c:1176 unlock_two_nondirectories()
> >     warn: variable dereferenced before check 'inode2' (see line 1173)
> 
> Indeed, thanks for spotting this! Luckily there are currently no in-tree
> users passing NULL. Attached patch fixes this. Christian, can you please

Thanks for the fixup!

> add this to your branch or squash it into the fixed commit? Thanks!


Of course. I've squashed the fix into the original patch.

---

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