[PATCH v5] fat: make rename recovery safe after metadata write errors

[Krystian Kaniewski]

A failed synchronous FAT metadata write clears BH_Uptodate. Rename
recovery can then modify and dirty the same retained buffer, which
triggers the !buffer_uptodate warning in mark_buffer_dirty().

Check the retained buffers in the rename error paths and skip only the
recovery step whose buffer is no longer uptodate. The other steps run as
before, including the rollback after a failed source-entry removal,
where only the source-entry buffer failed. VFAT keeps no slot
information for a replaced entry. Resolve its saved position with a
cache-only lookup before dirtying the inode, because FAT inode
writeback would otherwise reread and retry a failed block.

For exchange, do not rewrite the dotdot buffer whose write failed,
restore an earlier dotdot update only while its buffer is uptodate, and
restore both inode positions as before. A skipped step is reported as
filesystem corruption, and the original error is returned.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: syzbot+b0aebd03565f5774f7f8@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=b0aebd03565f5774f7f8
Signed-off-by: Krystian Kaniewski <krystianmkaniewski@xxxxxxxxx>
---
Changes in v5:
- Check buffer_uptodate() directly in the rename error paths and drop the
  helper and the buffer lock of v4.
- Skip only the recovery step of a buffer that is no longer uptodate and
  keep the rest of the existing recovery, also after a failed dotdot write
  and in exchange.
- Keep the existing rollback after a failed source-entry removal. The reply
  to v4 proposed stopping there, but fat_remove_entries() fails only when
  the old_sinfo buffer write fails, so the source entry is still in place.
  Stopping left an invalid ".." entry and shared clusters in my tests,
  while the existing rollback leaves the filesystem clean.
- Look up the cached VFAT target entry block from its inode position,
  because that path has no slot information.
- Clarify the recovery-state comments without changing behavior.

v4: https://lore.kernel.org/all/20261019145146.539233-1-krystianmkaniewski@xxxxxxxxx/

 fs/fat/namei_msdos.c | 18 ++++++++++++----
 fs/fat/namei_vfat.c  | 50 +++++++++++++++++++++++++++++++-------------
 2 files changed, 49 insertions(+), 19 deletions(-)

diff --git a/fs/fat/namei_msdos.c b/fs/fat/namei_msdos.c
index d46d1a3851f25f..51d49a3ca93c14 100644
--- a/fs/fat/namei_msdos.c
+++ b/fs/fat/namei_msdos.c
@@ -562,14 +562,20 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name,
 	return err;
 
 error_dotdot:
-	/* data cluster is shared, serious corruption */
+	/*
+	 * Complete the remaining rollback. Report corruption only if a
+	 * recovery step cannot be completed.
+	 */
 	corrupt = 1;
 
-	if (update_dotdot) {
+	/* A failed sync write clears BH_Uptodate, leave that buffer alone. */
+	if (update_dotdot && buffer_uptodate(dotdot_bh)) {
 		fat_set_start(dotdot_de, MSDOS_I(old_dir)->i_logstart);
 		mmb_mark_buffer_dirty(dotdot_bh,
 				      &MSDOS_I(old_inode)->i_metadata_bhs);
 		corrupt |= sync_dirty_buffer(dotdot_bh);
+	} else if (update_dotdot) {
+		corrupt = err;
 	}
 error_inode:
 	fat_detach(old_inode);
@@ -577,11 +583,13 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name,
 	MSDOS_I(old_inode)->i_attrs = old_attrs;
 	if (new_inode) {
 		fat_attach(new_inode, new_i_pos);
-		if (corrupt) {
+		if (corrupt && buffer_uptodate(sinfo.bh)) {
 			mark_inode_dirty(new_inode);
 			corrupt |= sync_inode_metadata(new_inode, 1);
+		} else if (corrupt) {
+			corrupt = err;
 		}
-	} else {
+	} else if (buffer_uptodate(sinfo.bh)) {
 		/*
 		 * If new entry was not sharing the data cluster, it
 		 * shouldn't be serious corruption.
@@ -590,6 +598,8 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name,
 		if (corrupt)
 			corrupt |= err2;
 		sinfo.bh = NULL;
+	} else {
+		corrupt = err;
 	}
 	if (corrupt < 0) {
 		fat_fs_error(new_dir->i_sb,
diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
index da3e89c0b16ac8..31f9882f0a9ffe 100644
--- a/fs/fat/namei_vfat.c
+++ b/fs/fat/namei_vfat.c
@@ -1010,12 +1010,18 @@ static int vfat_rename(struct inode *old_dir, struct dentry *old_dentry,
 	return err;
 
 error_dotdot:
-	/* data cluster is shared, serious corruption */
+	/*
+	 * Complete the remaining rollback. Report corruption only if a
+	 * recovery step cannot be completed.
+	 */
 	corrupt = 1;
 
-	if (dotdot_de) {
+	/* A failed sync write clears BH_Uptodate, leave that buffer alone. */
+	if (dotdot_de && buffer_uptodate(dotdot_bh)) {
 		corrupt |= vfat_update_dotdot_de(old_dir, old_inode, dotdot_bh,
 						 dotdot_de);
+	} else if (dotdot_de) {
+		corrupt = err;
 	}
 error_inode:
 	fat_detach(old_inode);
@@ -1023,10 +1029,27 @@ static int vfat_rename(struct inode *old_dir, struct dentry *old_dentry,
 	if (new_inode) {
 		fat_attach(new_inode, new_i_pos);
 		if (corrupt) {
-			mark_inode_dirty(new_inode);
-			corrupt |= sync_inode_metadata(new_inode, 1);
+			struct buffer_head *target_bh;
+			sector_t blocknr;
+			int offset;
+
+			/*
+			 * A replaced entry has no slot info. Use its saved
+			 * position for a cache-only lookup so recovery does
+			 * not reread a failed block.
+			 */
+			fat_get_blknr_offset(MSDOS_SB(sb), new_i_pos,
+					     &blocknr, &offset);
+			target_bh = sb_find_get_block_nonatomic(sb, blocknr);
+			if (target_bh && buffer_uptodate(target_bh)) {
+				mark_inode_dirty(new_inode);
+				corrupt |= sync_inode_metadata(new_inode, 1);
+			} else {
+				corrupt = err;
+			}
+			brelse(target_bh);
 		}
-	} else {
+	} else if (buffer_uptodate(sinfo.bh)) {
 		/*
 		 * If new entry was not sharing the data cluster, it
 		 * shouldn't be serious corruption.
@@ -1035,6 +1058,8 @@ static int vfat_rename(struct inode *old_dir, struct dentry *old_dentry,
 		if (corrupt)
 			corrupt |= err2;
 		sinfo.bh = NULL;
+	} else {
+		corrupt = err;
 	}
 	if (corrupt < 0) {
 		fat_fs_error(new_dir->i_sb,
@@ -1106,13 +1131,13 @@ static int vfat_rename_exchange(struct inode *old_dir, struct dentry *old_dentry
 		err = vfat_update_dotdot_de(new_dir, old_inode, old_dotdot_bh,
 					    old_dotdot_de);
 		if (err)
-			goto error_old_dotdot;
+			goto error_dotdot;
 	}
 	if (new_dotdot_de) {
 		err = vfat_update_dotdot_de(old_dir, new_inode, new_dotdot_bh,
 					    new_dotdot_de);
 		if (err)
-			goto error_new_dotdot;
+			goto error_dotdot;
 	}
 
 	/* if cross directory and only one is a directory, adjust nlink */
@@ -1135,14 +1160,9 @@ static int vfat_rename_exchange(struct inode *old_dir, struct dentry *old_dentry
 
 	return err;
 
-error_new_dotdot:
-	if (new_dotdot_de) {
-		corrupt |= vfat_update_dotdot_de(new_dir, new_inode,
-						 new_dotdot_bh, new_dotdot_de);
-	}
-
-error_old_dotdot:
-	if (old_dotdot_de) {
+error_dotdot:
+	corrupt = err;
+	if (old_dotdot_de && buffer_uptodate(old_dotdot_bh)) {
 		corrupt |= vfat_update_dotdot_de(old_dir, old_inode,
 						 old_dotdot_bh, old_dotdot_de);
 	}

base-commit: 93f51579e7df248780214094418f205253383cc5
-- 
2.53.0