[PATCH v2] fat: skip rename rollback on buffers with failed I/O

[Krystian Kaniewski]

A synchronous directory-entry write can fail after clearing BH_Uptodate.
The rename error path then attempts rollback by modifying and dirtying the
same buffer. mark_buffer_dirty() warns on the resulting !buffer_uptodate
buffer, and retrying the write cannot repair the underlying I/O failure.

Once a synchronous update fails, record whether the new entry shares that
buffer and do not touch it again. Before fat_remove_entries() releases the
old-entry buffer, record aliases so later failures also avoid rolling back
through it. For RENAME_EXCHANGE, restore the first ".." update only when it
does not share the buffer on which the second update failed.

Keep the existing forced synchronous MS-DOS rollback when it targets a
different buffer. For skipped rollbacks, retain the original error and
report filesystem corruption through fat_fs_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 v2:
- Avoid buffer locking and waiting in the normal update path, as suggested
  by OGAWA Hirofumi.
- Give up rollback after a synchronous failure on the affected buffer.
- Track buffer aliases before fat_remove_entries() releases the old buffer.
- Keep forced synchronization for an MS-DOS rollback on another buffer.

 fs/fat/namei_msdos.c | 28 ++++++++++++++++++++-----
 fs/fat/namei_vfat.c  | 50 +++++++++++++++++++++++++++++---------------
 2 files changed, 56 insertions(+), 22 deletions(-)

diff --git a/fs/fat/namei_msdos.c b/fs/fat/namei_msdos.c
index d46d1a3851f25..079f8bb61647f 100644
--- a/fs/fat/namei_msdos.c
+++ b/fs/fat/namei_msdos.c
@@ -441,6 +441,8 @@ 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;
+	bool dotdot_in_old_bh, new_in_old_bh;
+	bool new_bh_failed = false;
 	int err, old_attrs, is_dir, update_dotdot, corrupt = 0;
 
 	old_sinfo.bh = sinfo.bh = dotdot_bh = NULL;
@@ -532,14 +534,20 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name,
 				      &MSDOS_I(old_inode)->i_metadata_bhs);
 		if (IS_DIRSYNC(new_dir)) {
 			err = sync_dirty_buffer(dotdot_bh);
-			if (err)
-				goto error_dotdot;
+			if (err) {
+				corrupt = err;
+				new_bh_failed = sinfo.bh == dotdot_bh;
+				goto error_inode;
+			}
 		}
 		drop_nlink(old_dir);
 		if (!new_inode)
 			inc_nlink(new_dir);
 	}
 
+	/* Remember aliases before fat_remove_entries() releases the buffer. */
+	dotdot_in_old_bh = dotdot_bh == old_sinfo.bh;
+	new_in_old_bh = sinfo.bh == old_sinfo.bh;
 	err = fat_remove_entries(old_dir, &old_sinfo);	/* and releases bh */
 	old_sinfo.bh = NULL;
 	if (err)
@@ -564,12 +572,22 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name,
 error_dotdot:
 	/* data cluster is shared, serious corruption */
 	corrupt = 1;
+	if (dotdot_in_old_bh || new_in_old_bh) {
+		/* Give up rollback on the buffer whose write failed. */
+		corrupt = err;
+		new_bh_failed = new_in_old_bh;
+	}
+
+	if (update_dotdot && !dotdot_in_old_bh) {
+		int dotdot_err;
 
-	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);
+		dotdot_err = sync_dirty_buffer(dotdot_bh);
+		corrupt |= dotdot_err;
+		if (dotdot_err && sinfo.bh == dotdot_bh)
+			new_bh_failed = true;
 	}
 error_inode:
 	fat_detach(old_inode);
@@ -581,7 +599,7 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name,
 			mark_inode_dirty(new_inode);
 			corrupt |= sync_inode_metadata(new_inode, 1);
 		}
-	} else {
+	} else if (!new_bh_failed) {
 		/*
 		 * If new entry was not sharing the data cluster, it
 		 * shouldn't be serious corruption.
diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
index da3e89c0b16ac..b6a32f39a6f88 100644
--- a/fs/fat/namei_vfat.c
+++ b/fs/fat/namei_vfat.c
@@ -938,6 +938,8 @@ 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;
+	bool dotdot_in_old_bh, new_in_old_bh;
+	bool new_bh_failed = false;
 	int err, is_dir, corrupt = 0;
 	struct super_block *sb = old_dir->i_sb;
 
@@ -983,13 +985,19 @@ static int vfat_rename(struct inode *old_dir, struct dentry *old_dentry,
 	if (dotdot_de) {
 		err = vfat_update_dotdot_de(new_dir, old_inode, dotdot_bh,
 					    dotdot_de);
-		if (err)
-			goto error_dotdot;
+		if (err) {
+			corrupt = err;
+			new_bh_failed = sinfo.bh == dotdot_bh;
+			goto error_inode;
+		}
 		drop_nlink(old_dir);
 		if (!new_inode)
  			inc_nlink(new_dir);
 	}
 
+	/* Remember aliases before fat_remove_entries() releases the buffer. */
+	dotdot_in_old_bh = dotdot_bh == old_sinfo.bh;
+	new_in_old_bh = sinfo.bh == old_sinfo.bh;
 	err = fat_remove_entries(old_dir, &old_sinfo);	/* and releases bh */
 	old_sinfo.bh = NULL;
 	if (err)
@@ -1012,10 +1020,19 @@ static int vfat_rename(struct inode *old_dir, struct dentry *old_dentry,
 error_dotdot:
 	/* data cluster is shared, serious corruption */
 	corrupt = 1;
+	if (dotdot_in_old_bh || new_in_old_bh) {
+		/* Give up rollback on the buffer whose write failed. */
+		corrupt = err;
+		new_bh_failed = new_in_old_bh;
+	}
 
-	if (dotdot_de) {
-		corrupt |= vfat_update_dotdot_de(old_dir, old_inode, dotdot_bh,
-						 dotdot_de);
+	if (dotdot_de && !dotdot_in_old_bh) {
+		int dotdot_err = vfat_update_dotdot_de(old_dir, old_inode,
+						       dotdot_bh, dotdot_de);
+
+		corrupt |= dotdot_err;
+		if (dotdot_err && sinfo.bh == dotdot_bh)
+			new_bh_failed = true;
 	}
 error_inode:
 	fat_detach(old_inode);
@@ -1026,7 +1043,7 @@ static int vfat_rename(struct inode *old_dir, struct dentry *old_dentry,
 			mark_inode_dirty(new_inode);
 			corrupt |= sync_inode_metadata(new_inode, 1);
 		}
-	} else {
+	} else if (!new_bh_failed) {
 		/*
 		 * If new entry was not sharing the data cluster, it
 		 * shouldn't be serious corruption.
@@ -1105,14 +1122,18 @@ static int vfat_rename_exchange(struct inode *old_dir, struct dentry *old_dentry
 	if (old_dotdot_de) {
 		err = vfat_update_dotdot_de(new_dir, old_inode, old_dotdot_bh,
 					    old_dotdot_de);
-		if (err)
-			goto error_old_dotdot;
+		if (err) {
+			corrupt = err;
+			goto error_exchange;
+		}
 	}
 	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;
+		if (err) {
+			corrupt = err;
+			goto error_old_dotdot;
+		}
 	}
 
 	/* if cross directory and only one is a directory, adjust nlink */
@@ -1135,14 +1156,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) {
+	/* Both entries may share the buffer whose write failed. */
+	if (old_dotdot_de && old_dotdot_bh != new_dotdot_bh) {
 		corrupt |= vfat_update_dotdot_de(old_dir, old_inode,
 						 old_dotdot_bh, old_dotdot_de);
 	}

base-commit: 93f51579e7df248780214094418f205253383cc5
-- 
2.53.0