Re: [PATCH] fat: avoid freeing clusters on failed directory validation or read-only mounts

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

Hui Peng <benquike@gmail.com> writes:

> In fat_fill_inode(), set_nlink(inode, fat_subdirs(inode)) is called
> before fat_validate_dir(inode). If a directory has zero valid
> subdirectories (so fat_subdirs(inode) returns 0) and fat_validate_dir()
> subsequently returns -EIO (due to a missing or malformed '.'/'..'
> entry), fat_build_inode() calls iput(inode) with inode->i_nlink == 0 and
> inode->i_start still populated.
>
> fat_evict_inode() then checks !inode->i_nlink without checking
> is_bad_inode(inode) or sb_rdonly(inode->i_sb) and calls
> fat_truncate_blocks(inode, 0), freeing the cluster chain on disk even on
> a read-only (MS_RDONLY) mount.
>
> Validate the directory before setting i_nlink in fat_fill_inode(), mark
> the inode bad on error in fat_build_inode(), and guard cluster
> truncation and metadata writeback in fat_evict_inode() with
> !is_bad_inode(inode) && !sb_rdonly(inode->i_sb).
>
> Fixes: a3082d526f2d ("fat: add simple validation for directory inode")
> Assisted-by: LLM
> Signed-off-by: Hui Peng <benquike@gmail.com>
> ---
>
> diff --git a/fs/fat/inode.c b/fs/fat/inode.c
> --- a/fs/fat/inode.c
> +++ b/fs/fat/inode.c
> @@ -535,11 +535,11 @@ int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de)
>  			return error;
>  		MSDOS_I(inode)->mmu_private = inode->i_size;
>  
> -		set_nlink(inode, fat_subdirs(inode));
> -
>  		error = fat_validate_dir(inode);

Without set_nlink(), fat_validate_dir() will always return the error,
isn't it? If so, this patch is not tested?

>  		if (error < 0)
>  			return error;
> +
> +		set_nlink(inode, fat_subdirs(inode));
>  	} else { /* not a directory */
>  		inode->i_generation |= 1;
>  		inode->i_mode = fat_make_mode(sbi, de->attr,
> @@ -610,6 +610,7 @@ struct inode *fat_build_inode(struct super_block *sb,
>  	inode_set_iversion(inode, 1);
>  	err = fat_fill_inode(inode, de);
>  	if (err) {
> +		make_bad_inode(inode);
>  		iput(inode);
>  		inode = ERR_PTR(err);
>  		goto out;
> @@ -688,12 +689,14 @@ static void fat_free_eofblocks(struct inode *inode)
>  static void fat_evict_inode(struct inode *inode)
>  {
>  	truncate_inode_pages_final(&inode->i_data);
> -	if (!inode->i_nlink) {
> -		inode->i_size = 0;
> -		fat_truncate_blocks(inode, 0);
> -	} else {
> -		mmb_sync(&MSDOS_I(inode)->i_metadata_bhs);
> -		fat_free_eofblocks(inode);
> +	if (!is_bad_inode(inode) && !sb_rdonly(inode->i_sb)) {
> +		if (!inode->i_nlink) {
> +			inode->i_size = 0;
> +			fat_truncate_blocks(inode, 0);
> +		} else {
> +			mmb_sync(&MSDOS_I(inode)->i_metadata_bhs);
> +			fat_free_eofblocks(inode);
> +		}
>  	}
>  
>  	mmb_invalidate(&MSDOS_I(inode)->i_metadata_bhs);

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