Re: [PATCH v2] ext4: do not check fast symlink during orphan recovery

[Jan Kara]

On Sat 31-01-26 17:11:56, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@xxxxxxxxxx>
> 
> Commit '5f920d5d6083 ("ext4: verify fast symlink length")' causes the
> generic/475 test to fail during orphan cleanup of zero-length symlinks.
> 
>   generic/475  84s ... _check_generic_filesystem: filesystem on /dev/vde is inconsistent
> 
> The fsck reports are provided below:
> 
>   Deleted inode 9686 has zero dtime.
>   Deleted inode 158230 has zero dtime.
>   ...
>   Inode bitmap differences:  -9686 -158230
>   Orphan file (inode 12) block 13 is not clean.
>   Failed to initialize orphan file.
> 
> In ext4_symlink(), a newly created symlink can be added to the orphan
> list due to ENOSPC. Its data has not been initialized, and its size is
> zero. Therefore, we need to disregard the length check of the symbolic
> link when cleaning up orphan inodes. Instead, we should ensure that the
> nlink count is zero.
> 
> Fixes: 5f920d5d6083 ("ext4: verify fast symlink length")
> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>

Looks good! Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
> Changes since v1:
>  - Improve the comment and add nlink check during orphan cleanup as Jan
>    suggested.
> 
>  fs/ext4/inode.c | 40 +++++++++++++++++++++++++++++-----------
>  1 file changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 129594bf8311..cfb66f7ad3d7 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -6073,18 +6073,36 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
>  			inode->i_op = &ext4_encrypted_symlink_inode_operations;
>  		} else if (ext4_inode_is_fast_symlink(inode)) {
>  			inode->i_op = &ext4_fast_symlink_inode_operations;
> -			if (inode->i_size == 0 ||
> -			    inode->i_size >= sizeof(ei->i_data) ||
> -			    strnlen((char *)ei->i_data, inode->i_size + 1) !=
> -								inode->i_size) {
> -				ext4_error_inode(inode, function, line, 0,
> -					"invalid fast symlink length %llu",
> -					 (unsigned long long)inode->i_size);
> -				ret = -EFSCORRUPTED;
> -				goto bad_inode;
> +
> +			/*
> +			 * Orphan cleanup can see inodes with i_size == 0
> +			 * and i_data uninitialized. Skip size checks in
> +			 * that case. This is safe because the first thing
> +			 * ext4_evict_inode() does for fast symlinks is
> +			 * clearing of i_data and i_size.
> +			 */
> +			if ((EXT4_SB(sb)->s_mount_state & EXT4_ORPHAN_FS)) {
> +				if (inode->i_nlink != 0) {
> +					ext4_error_inode(inode, function, line, 0,
> +						"invalid orphan symlink nlink %d",
> +						inode->i_nlink);
> +					ret = -EFSCORRUPTED;
> +					goto bad_inode;
> +				}
> +			} else {
> +				if (inode->i_size == 0 ||
> +				    inode->i_size >= sizeof(ei->i_data) ||
> +				    strnlen((char *)ei->i_data, inode->i_size + 1) !=
> +						inode->i_size) {
> +					ext4_error_inode(inode, function, line, 0,
> +						"invalid fast symlink length %llu",
> +						(unsigned long long)inode->i_size);
> +					ret = -EFSCORRUPTED;
> +					goto bad_inode;
> +				}
> +				inode_set_cached_link(inode, (char *)ei->i_data,
> +						      inode->i_size);
>  			}
> -			inode_set_cached_link(inode, (char *)ei->i_data,
> -					      inode->i_size);
>  		} else {
>  			inode->i_op = &ext4_symlink_inode_operations;
>  		}
> -- 
> 2.52.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR