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

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

[Dave Chinner]

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>
---
 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 */
-- 
1.5.6

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

[Eric Sandeen]

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 */

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

[Dave Chinner]

On Tue, Sep 23, 2008 at 10:07:56PM -0500, Eric Sandeen wrote:
> 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?)

Yes, I tried to mount it.

> but the
> commit message could be made a bit more ... English ;)

Right. Updated patch (description) below ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

XFS: Check for valid transaction headers in recovery

When we are about to add a new item to a transaction in recovery, we
need to check that it is valid first. Currently we just assert that
header magic number matches, but in production systems that is not
present and we 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.
---
 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 */

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

[Lachlan McIlroy]

Looks fine now Dave.

Dave Chinner wrote:
> On Tue, Sep 23, 2008 at 10:07:56PM -0500, Eric Sandeen wrote:
>> 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?)
> 
> Yes, I tried to mount it.
> 
>> but the
>> commit message could be made a bit more ... English ;)
> 
> Right. Updated patch (description) below ;)
> 
> Cheers,
> 
> Dave.

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

[Dave Chinner]

When we are about to add a new item to a transaction in recovery, we
need to check that it is valid first. Currently we just assert that
header magic number matches, but in production systems that is
present and we 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>
---
 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 0280a25..6d961fc 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1417,7 +1417,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 */
-- 
1.5.6.5