XFS: Check for valid transaction headers in recovery
PATCH 1/1 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. 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.
--- fs/xfs/xfs_log_recover.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-)
fs/xfs/xfs_log_recover.c +7 -1
@@ -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 */
Signature · 1 line
1.5.6
Quoted 11 lines · 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 <[email protected]>
Seems fine to me (I guess you tried the provided corrupt image?) but the commit message could be made a bit more ... English ;)
-Eric
Quoted 23 lines
--- fs/xfs/xfs_log_recover.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-)fs/xfs/xfs_log_recover.c +7
-1@@ -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 */
Quoted 8 lines · Eric Sandeen
Quoted 7 lines · 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.
Quoted 6 lines
Quoted 4 lines
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 <[email protected]>Seems fine to me (I guess you tried the provided corrupt image?)
Yes, I tried to mount it.
Quoted 2 lines
but the
commit message could be made a bit more ... English ;)
Right. Updated patch (description) below ;)
Cheers,
Dave.
Signature · 17 lines
Dave Chinner [email protected] 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(-)
fs/xfs/xfs_log_recover.c +7 -1
@@ -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 */
Looks fine now Dave.
Quoted 25 lines · Dave Chinner
Quoted 8 lines · Eric Sandeen
Quoted 7 lines · 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.
Quoted 5 lines
Quoted 4 lines
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 <[email protected]>Seems fine to me (I guess you tried the provided corrupt image?)
Yes, I tried to mount it.
Quoted 2 lines
but the
commit message could be made a bit more ... English ;)Right. Updated patch (description) below ;)
Cheers,
Dave.