Re: [PATCH] XFS: Check for valid transaction headers in recovery

[Eric Sandeen <sandeen@sandeen.net>]

Dave Chinner wrote:
> When we are about to add a new item to a transaction in recovery,
> we need to check that it is valid first. Current we just assert
> that header magic number matches, but in production systems
> that is not done add a corrupted transaction to the list to be
> processed. This results in a kernel oops later when processing the
> corrupted transaction.
> 
> Instead, if we detect a corrupted transaction, abort recovery and
> leave the user to clean up the mess that has occurred.
> 
> Signed-off-by: Dave Chinner <david@fromorbit.com>

Seems fine to me (I guess you tried the provided corrupt image?) but the
commit message could be made a bit more ... English ;)

-Eric

> ---
>  fs/xfs/xfs_log_recover.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 37c2bf9..1ccc80d 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -1420,7 +1420,13 @@ xlog_recover_add_to_trans(
>  		return 0;
>  	item = trans->r_itemq;
>  	if (item == NULL) {
> -		ASSERT(*(uint *)dp == XFS_TRANS_HEADER_MAGIC);
> +		/* we need to catch log corruptions here */
> +		if (*(uint *)dp != XFS_TRANS_HEADER_MAGIC) {
> +			xlog_warn("XFS: xlog_recover_add_to_trans: "
> +				  "bad header magic number");
> +			ASSERT(0);
> +			return XFS_ERROR(EIO);
> +		}
>  		if (len == sizeof(xfs_trans_header_t))
>  			xlog_recover_add_item(&trans->r_itemq);
>  		memcpy(&trans->r_theader, dp, len); /* d, s, l */