Re: [PATCH 2/4] metadump: be careful zeroing corrupt inode forks

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Apr 27, 2022 at 09:44:51AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> When a corrupt inode fork is encountered, we can zero beyond the end
> of the inode if the fork pointers are sufficiently trashed. We
> should not trust the fork pointers when corruption is detected and
> skip the zeroing in this case. We want metadump to capture the
> corruption and so skipping the zeroing will give us the best chance
> of preserving the corruption in a meaningful state for diagnosis.
> 
> Reported-by: Sean Caron <scaron@umich.edu>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Hmm.  I /think/ the only real change here is the addition of the
DFORK_DSIZE > LITINO warning, right?  The rest is just reindenting the
loop body?

If so, LGTM.
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  db/metadump.c | 49 +++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 39 insertions(+), 10 deletions(-)
> 
> diff --git a/db/metadump.c b/db/metadump.c
> index a21baa2070d9..3948d36e4d5c 100644
> --- a/db/metadump.c
> +++ b/db/metadump.c
> @@ -2308,18 +2308,34 @@ process_inode_data(
>  {
>  	switch (dip->di_format) {
>  		case XFS_DINODE_FMT_LOCAL:
> -			if (obfuscate || zero_stale_data)
> -				switch (itype) {
> -					case TYP_DIR2:
> -						process_sf_dir(dip);
> -						break;
> +			if (!(obfuscate || zero_stale_data))
> +				break;
> +
> +			/*
> +			 * If the fork size is invalid, we can't safely do
> +			 * anything with this fork. Leave it alone to preserve
> +			 * the information for diagnostic purposes.
> +			 */
> +			if (XFS_DFORK_DSIZE(dip, mp) > XFS_LITINO(mp)) {
> +				print_warning(
> +"Invalid data fork size (%d) in inode %llu, preserving contents!",
> +						XFS_DFORK_DSIZE(dip, mp),
> +						(long long)cur_ino);
> +				break;
> +			}
>  
> -					case TYP_SYMLINK:
> -						process_sf_symlink(dip);
> -						break;
> +			switch (itype) {
> +				case TYP_DIR2:
> +					process_sf_dir(dip);
> +					break;
>  
> -					default: ;
> -				}
> +				case TYP_SYMLINK:
> +					process_sf_symlink(dip);
> +					break;
> +
> +				default:
> +					break;
> +			}
>  			break;
>  
>  		case XFS_DINODE_FMT_EXTENTS:
> @@ -2341,6 +2357,19 @@ process_dev_inode(
>  				      (unsigned long long)cur_ino);
>  		return;
>  	}
> +
> +	/*
> +	 * If the fork size is invalid, we can't safely do anything with
> +	 * this fork. Leave it alone to preserve the information for diagnostic
> +	 * purposes.
> +	 */
> +	if (XFS_DFORK_DSIZE(dip, mp) > XFS_LITINO(mp)) {
> +		print_warning(
> +"Invalid data fork size (%d) in inode %llu, preserving contents!",
> +				XFS_DFORK_DSIZE(dip, mp), (long long)cur_ino);
> +		return;
> +	}
> +
>  	if (zero_stale_data) {
>  		unsigned int	size = sizeof(xfs_dev_t);
>  
> -- 
> 2.35.1
>