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

What is wrong to try to revert dotdot_bh?

>  	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);

Why do this read the bh from on disk?

> +		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);

Same above questions for vfat with msdos.

Thanks.

> 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);

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