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@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=b0aebd03565f5774f7f8 Signed-off-by: Krystian Kaniewski --- 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@gmail.com/ 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