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

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

Krystian Kaniewski <krystianmkaniewski@gmail.com> writes:

> 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.
>
> An inode-position write can also fail through another associated metadata
> buffer. In that case the retained new-entry buffer remains uptodate, so
> remove the added entry directly. Check the buffer state and dirty the
> entry while holding the buffer lock. If the entry buffer itself failed
> writeback, leave it untouched instead of reading it again and submitting
> another write from FAT.
>
> Keep the existing replacement and exchange inode-position recovery. When
> recovery starts after an exchange position write fails, attempt both
> restorations so a failure of one does not suppress the other.
>
> Report recovery errors as corruption and preserve the original forward
> error.
>
> 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 <krystianmkaniewski@gmail.com>
> ---
> Changes in v4:
> - Trust a final block-layer write error. Do not reread or retry the retained
>   new-entry buffer from FAT when it is no longer uptodate.
> - Remove the added entry directly when its retained buffer remains uptodate.
>   Hold the buffer lock across the state check, entry mutation and dirty mark.
> - Keep failed dotdot writes and failed old-entry removal on the v3
>   no-rollback path.
>
> v3: https://lore.kernel.org/all/20261014082315.190262-1-krystianmkaniewski@gmail.com/
>
>  fs/fat/dir.c         | 27 +++++++++++++++++--
>  fs/fat/fat.h         |  2 ++
>  fs/fat/namei_msdos.c | 37 +++++++++----------------
>  fs/fat/namei_vfat.c  | 64 ++++++++++++++++----------------------------
>  4 files changed, 62 insertions(+), 68 deletions(-)
>
> diff --git a/fs/fat/dir.c b/fs/fat/dir.c
> index 35bdb62944a2e..70433361359cb 100644
> --- a/fs/fat/dir.c
> +++ b/fs/fat/dir.c
> @@ -1066,7 +1066,9 @@ static int __fat_remove_entries(struct inode *dir, loff_t pos, int nr_slots)
>  	return err;
>  }
>  
> -int fat_remove_entries(struct inode *dir, struct fat_slot_info *sinfo)
> +static int fat_remove_entries_internal(struct inode *dir,
> +				       struct fat_slot_info *sinfo,
> +				       bool require_uptodate)
>  {
>  	struct super_block *sb = dir->i_sb;
>  	struct msdos_dir_entry *de;
> @@ -1079,8 +1081,15 @@ int fat_remove_entries(struct inode *dir, struct fat_slot_info *sinfo)
>  	 */
>  	nr_slots = sinfo->nr_slots;
>  	de = sinfo->de;
> -	sinfo->de = NULL;
>  	bh = sinfo->bh;
> +	if (require_uptodate) {
> +		lock_buffer(bh);
> +		if (!buffer_uptodate(bh)) {
> +			unlock_buffer(bh);
> +			return -EIO;
> +		}
> +	}
> +	sinfo->de = NULL;

Looks like strange to check buffer_uptodate() (or
buffer_write_io_error()?) at here. Why lock_buffer is required (rename
locks are not enough? which race?), and why not checked at rename error
path?

>  	sinfo->bh = NULL;
>  	while (nr_slots && de >= (struct msdos_dir_entry *)bh->b_data) {
>  		de->name[0] = DELETED_FLAG;
> @@ -1088,6 +1097,8 @@ int fat_remove_entries(struct inode *dir, struct fat_slot_info *sinfo)
>  		nr_slots--;
>  	}
>  	mmb_mark_buffer_dirty(bh, &MSDOS_I(dir)->i_metadata_bhs);
> +	if (require_uptodate)
> +		unlock_buffer(bh);
>  	if (IS_DIRSYNC(dir))
>  		err = sync_dirty_buffer(bh);
>  	brelse(bh);

[...]

> diff --git a/fs/fat/namei_msdos.c b/fs/fat/namei_msdos.c
> index d46d1a3851f25..6c833ed1affb3 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;

Hm, I may be confused more or less though, do we have to give up whole
if dotdot_bh and here are failure? Maybe we give up only if old_sinfo.bh
was the cause of I/O error actually? Checking and tracking the error
state is too complex or something?

>  	inode_inc_iversion(old_dir);
>  	fat_truncate_time(old_dir, &ts, FAT_UPDATE_CMTIME);
>  	mark_inode_dirty(old_dir);
> @@ -561,37 +561,24 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name,
>  	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) {
> +		/* Do not retry a write error reported for this buffer. */
> +		rollback_err = fat_remove_entries_if_uptodate(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);

[...]

> @@ -1067,7 +1056,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 +1095,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 +1124,17 @@ 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) {
> -		corrupt |= vfat_update_dotdot_de(old_dir, old_inode,
> -						 old_dotdot_bh, old_dotdot_de);
> -	}
> -

Here gives up whole if a buffer had error too?

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>