[PATCH 0/2] fs: rename follow-up fixes

[PATCH 0/2] fs: rename follow-up fixes

[Christian Brauner]

Jan,

Two minor fixes for the rename work for this cycle. First one is based
on a static analysis report from earlier today. The second one is based
on manual review.

Thanks!
Christian

---



---
base-commit: a901a3568fd26ca9c4a82d8bc5ed5b3ed844d451
change-id: 20230703-vfs-rename-source-a6093ca69595

[PATCH 1/2] fs: no need to check source

[Christian Brauner]

The @source inode must be valid. It is even checked via IS_SWAPFILE()
above making it pretty clear. So no need to check it when we unlock.

What doesn't need to exist is the @target inode. The lock_two_inodes()
helper currently swaps the @inode1 and @inode2 arguments if @inode1 is
NULL to have consistent lock class usage. However, we know that at least
for vfs_rename() that @inode1 is @source and thus is never NULL as per
above. We also know that @source is a different inode than @target as
that is checked right at the beginning of vfs_rename(). So we know that
@source is valid and locked and that @target is locked. So drop the
check whether @source is non-NULL.

Fixes: 28eceeda130f ("fs: Lock moved directories")
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/202307030026.9sE2pk2x-lkp@intel.com
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/namei.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 91171da719c5..e56ff39a79bc 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4874,8 +4874,7 @@ int vfs_rename(struct renamedata *rd)
 			d_exchange(old_dentry, new_dentry);
 	}
 out:
-	if (source)
-		inode_unlock(source);
+	inode_unlock(source);
 	if (target)
 		inode_unlock(target);
 	dput(new_dentry);

-- 
2.34.1

[PATCH 2/2] fs: don't assume arguments are non-NULL

[Christian Brauner]

The helper is explicitly documented as locking zero, one, or two
arguments. While all current callers do pass non-NULL arguments there's
no need or requirement for them to do so according to the code and the
unlock_two_nondirectories() helper is pretty clear about it as well. So
only call WARN_ON_ONCE() if the checked inode is valid.

Cc: Jan Kara <jack@suse.cz>
Fixes: 2454ad83b90a ("fs: Restrict lock_two_nondirectories() to non-directory inodes")
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/inode.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index d37fad91c8da..8fefb69e1f84 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1156,8 +1156,10 @@ void lock_two_inodes(struct inode *inode1, struct inode *inode2,
  */
 void lock_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)
+		WARN_ON_ONCE(S_ISDIR(inode1->i_mode));
+	if (inode2)
+		WARN_ON_ONCE(S_ISDIR(inode2->i_mode));
 	lock_two_inodes(inode1, inode2, I_MUTEX_NORMAL, I_MUTEX_NONDIR2);
 }
 EXPORT_SYMBOL(lock_two_nondirectories);

-- 
2.34.1

Re: [PATCH 0/2] fs: rename follow-up fixes

[Jan Kara]

Hello Christian!

On Mon 03-07-23 16:49:10, Christian Brauner wrote:
> Two minor fixes for the rename work for this cycle. First one is based
> on a static analysis report from earlier today. The second one is based
> on manual review.

Thanks! In fact I've sent you a very same patch as patch 1 just a while ago
[1] and your patch 2 already back in June [2]. But feel free to use your
fixups, they look good to me so also feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

[1] https://lore.kernel.org/all/20230703144306.32639-1-jack@suse.cz
[2] https://lore.kernel.org/all/20230606095625.zowqbpfi7hktfbwh@quack3

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

Re: [PATCH 0/2] fs: rename follow-up fixes

[Christian Brauner]

On Mon, Jul 03, 2023 at 06:23:21PM +0200, Jan Kara wrote:
> Hello Christian!
> 
> On Mon 03-07-23 16:49:10, Christian Brauner wrote:
> > Two minor fixes for the rename work for this cycle. First one is based
> > on a static analysis report from earlier today. The second one is based
> > on manual review.
> 
> Thanks! In fact I've sent you a very same patch as patch 1 just a while ago
> [1] and your patch 2 already back in June [2]. But feel free to use your
> fixups, they look good to me so also feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>

I'll take yours and just massage the commit message. :)

Re: [PATCH 0/2] fs: rename follow-up fixes

[Christian Brauner]

On Mon, 03 Jul 2023 16:49:10 +0200, Christian Brauner wrote:
> Jan,
> 
> Two minor fixes for the rename work for this cycle. First one is based
> on a static analysis report from earlier today. The second one is based
> on manual review.
> 
> Thanks!
> Christian
> 
> [...]

I've used your authorship on this, Jan and used my commit message.

---

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes 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.fixes

[1/2] fs: no need to check source
      https://git.kernel.org/vfs/vfs/c/66d8fc0539b0
[2/2] fs: don't assume arguments are non-NULL
      https://git.kernel.org/vfs/vfs/c/33ab231f83cc