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

[OGAWA Hirofumi]

Krystian Kaniewski <krystianmkaniewski@xxxxxxxxx> 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().
>
> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=b0aebd03565f5774f7f8
> Signed-off-by: Krystian Kaniewski <krystianmkaniewski@xxxxxxxxx>
> ---
> 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@xxxxxxxxx/
>
>  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)) {

This check is ok with 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.

[...]

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

[...]

I guess this check is needed for msdos too? If so, maybe we want the
helper function in fat module.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx>