[PATCH v3] 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().

Stop on-disk rollback after a failed dotdot update or old-entry removal,
where a complete rollback can no longer be guaranteed. At an
inode-position failure no dotdot update or source-entry removal has
started, so rereading the affected entry block and restoring the changed
entry slots can still complete the rollback.

Read a retained new-entry buffer again before removing the entry.
Replacement and exchange recovery use inode writeback, which reacquires
the directory-entry buffer. When the second exchange position write
fails, the first has already reached disk, so attempt both restorations
even if retrying the failed block also fails.

Report recovery errors returned by these steps as corruption and preserve
the original forward error.

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 v3:
- Give up the on-disk rollback entirely after a failed dotdot update or
  old-entry removal, as suggested by OGAWA Hirofumi, instead of tracking
  buffer aliases and reverting what is still possible.
- After a failed inode-position write, read the retained new-entry buffer
  again with bh_read() before removing the entry, and write a replaced
  target entry back through inode writeback.
- New subject, since rollback is no longer skipped per buffer.

v2: https://lore.kernel.org/all/1f18819d20c92960b8b2d6ccc8bc4506d346150e.1790779353.git.krystianmkaniewski@xxxxxxxxx/

 fs/fat/namei_msdos.c | 39 ++++++++++----------------
 fs/fat/namei_vfat.c  | 66 +++++++++++++++++---------------------------
 2 files changed, 39 insertions(+), 66 deletions(-)

diff --git a/fs/fat/namei_msdos.c b/fs/fat/namei_msdos.c
index d46d1a3..0aab2b1 100644
--- a/fs/fat/namei_msdos.c
+++ b/fs/fat/namei_msdos.c
@@ -441,7 +441,7 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name,
 	struct fat_slot_info old_sinfo, sinfo;
 	struct timespec64 ts;
 	loff_t new_i_pos;
-	int err, old_attrs, is_dir, update_dotdot, corrupt = 0;
+	int err, old_attrs, is_dir, update_dotdot, rollback_err = 0;
 
 	old_sinfo.bh = sinfo.bh = dotdot_bh = NULL;
 	old_inode = d_inode(old_dentry);
@@ -533,7 +533,7 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name,
 		if (IS_DIRSYNC(new_dir)) {
 			err = sync_dirty_buffer(dotdot_bh);
 			if (err)
-				goto error_dotdot;
+				goto error_no_rollback;
 		}
 		drop_nlink(old_dir);
 		if (!new_inode)
@@ -543,7 +543,7 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name,
 	err = fat_remove_entries(old_dir, &old_sinfo);	/* and releases bh */
 	old_sinfo.bh = NULL;
 	if (err)
-		goto error_dotdot;
+		goto error_no_rollback;
 	inode_inc_iversion(old_dir);
 	fat_truncate_time(old_dir, &ts, FAT_UPDATE_CMTIME);
 	mark_inode_dirty(old_dir);
@@ -561,37 +561,26 @@ out:
 	brelse(old_sinfo.bh);
 	return err;
 
-error_dotdot:
-	/* data cluster is shared, serious corruption */
-	corrupt = 1;
-
-	if (update_dotdot) {
-		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);
-	}
+error_no_rollback:
+	/* Do not continue an on-disk rollback that cannot be completed. */
+	rollback_err = err;
 error_inode:
 	fat_detach(old_inode);
 	fat_attach(old_inode, old_sinfo.i_pos);
 	MSDOS_I(old_inode)->i_attrs = old_attrs;
 	if (new_inode) {
 		fat_attach(new_inode, new_i_pos);
-		if (corrupt) {
+		if (!rollback_err) {
 			mark_inode_dirty(new_inode);
-			corrupt |= sync_inode_metadata(new_inode, 1);
+			rollback_err = sync_inode_metadata(new_inode, 1);
 		}
-	} else {
-		/*
-		 * If new entry was not sharing the data cluster, it
-		 * shouldn't be serious corruption.
-		 */
-		int err2 = fat_remove_entries(new_dir, &sinfo);
-		if (corrupt)
-			corrupt |= err2;
-		sinfo.bh = NULL;
+	} else if (!rollback_err) {
+		/* Read the entry block again if a failed write invalidated it. */
+		rollback_err = bh_read(sinfo.bh, 0);
+		if (rollback_err >= 0)
+			rollback_err = fat_remove_entries(new_dir, &sinfo);
 	}
-	if (corrupt < 0) {
+	if (rollback_err < 0) {
 		fat_fs_error(new_dir->i_sb,
 			     "%s: Filesystem corrupted (i_pos %lld)",
 			     __func__, sinfo.i_pos);
diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
index da3e89c..f10828c 100644
--- a/fs/fat/namei_vfat.c
+++ b/fs/fat/namei_vfat.c
@@ -938,7 +938,7 @@ static int vfat_rename(struct inode *old_dir, struct dentry *old_dentry,
 	struct fat_slot_info old_sinfo, sinfo;
 	struct timespec64 ts;
 	loff_t new_i_pos;
-	int err, is_dir, corrupt = 0;
+	int err, is_dir, rollback_err = 0;
 	struct super_block *sb = old_dir->i_sb;
 
 	old_sinfo.bh = sinfo.bh = dotdot_bh = NULL;
@@ -984,7 +984,7 @@ static int vfat_rename(struct inode *old_dir, struct dentry *old_dentry,
 		err = vfat_update_dotdot_de(new_dir, old_inode, dotdot_bh,
 					    dotdot_de);
 		if (err)
-			goto error_dotdot;
+			goto error_no_rollback;
 		drop_nlink(old_dir);
 		if (!new_inode)
  			inc_nlink(new_dir);
@@ -993,7 +993,7 @@ static int vfat_rename(struct inode *old_dir, struct dentry *old_dentry,
 	err = fat_remove_entries(old_dir, &old_sinfo);	/* and releases bh */
 	old_sinfo.bh = NULL;
 	if (err)
-		goto error_dotdot;
+		goto error_no_rollback;
 	vfat_update_dir_metadata(old_dir, &ts);
 
 	if (new_inode) {
@@ -1009,34 +1009,25 @@ out:
 
 	return err;
 
-error_dotdot:
-	/* data cluster is shared, serious corruption */
-	corrupt = 1;
-
-	if (dotdot_de) {
-		corrupt |= vfat_update_dotdot_de(old_dir, old_inode, dotdot_bh,
-						 dotdot_de);
-	}
+error_no_rollback:
+	/* Do not continue an on-disk rollback that cannot be completed. */
+	rollback_err = err;
 error_inode:
 	fat_detach(old_inode);
 	fat_attach(old_inode, old_sinfo.i_pos);
 	if (new_inode) {
 		fat_attach(new_inode, new_i_pos);
-		if (corrupt) {
+		if (!rollback_err) {
 			mark_inode_dirty(new_inode);
-			corrupt |= sync_inode_metadata(new_inode, 1);
+			rollback_err = sync_inode_metadata(new_inode, 1);
 		}
-	} else {
-		/*
-		 * If new entry was not sharing the data cluster, it
-		 * shouldn't be serious corruption.
-		 */
-		int err2 = fat_remove_entries(new_dir, &sinfo);
-		if (corrupt)
-			corrupt |= err2;
-		sinfo.bh = NULL;
+	} else if (!rollback_err) {
+		/* Read the entry block again if a failed write invalidated it. */
+		rollback_err = bh_read(sinfo.bh, 0);
+		if (rollback_err >= 0)
+			rollback_err = fat_remove_entries(new_dir, &sinfo);
 	}
-	if (corrupt < 0) {
+	if (rollback_err < 0) {
 		fat_fs_error(new_dir->i_sb,
 			     "%s: Filesystem corrupted (i_pos %lld)",
 			     __func__, new_i_pos);
@@ -1067,7 +1058,7 @@ static int vfat_rename_exchange(struct inode *old_dir, struct dentry *old_dentry
 	struct inode *old_inode, *new_inode;
 	struct timespec64 ts = current_time(old_dir);
 	loff_t old_i_pos, new_i_pos;
-	int err, corrupt = 0;
+	int err, rollback_err = 0;
 	struct super_block *sb = old_dir->i_sb;
 
 	old_inode = d_inode(old_dentry);
@@ -1106,13 +1097,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_no_rollback;
 	}
 	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_no_rollback;
 	}
 
 	/* if cross directory and only one is a directory, adjust nlink */
@@ -1135,24 +1126,17 @@ out:
 
 	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) {
-		corrupt |= vfat_update_dotdot_de(old_dir, old_inode,
-						 old_dotdot_bh, old_dotdot_de);
-	}
-
+error_no_rollback:
+	/* Do not start a partial on-disk rollback after a failed ".." write. */
+	rollback_err = err;
 error_exchange:
 	vfat_exchange_ipos(old_inode, new_inode, new_i_pos, old_i_pos);
-	corrupt |= vfat_sync_ipos(new_dir, new_inode);
-	corrupt |= vfat_sync_ipos(old_dir, old_inode);
+	if (!rollback_err) {
+		rollback_err |= vfat_sync_ipos(new_dir, new_inode);
+		rollback_err |= vfat_sync_ipos(old_dir, old_inode);
+	}
 
-	if (corrupt < 0) {
+	if (rollback_err < 0) {
 		fat_fs_error(new_dir->i_sb,
 			     "%s: Filesystem corrupted (i_pos %lld, %lld)",
 			     __func__, old_i_pos, new_i_pos);
-- 
2.53.0