2011-12-22 08:31:41

by Akira Fujita

[permalink] [raw]
Subject: [PATCH 1/2] e2fsprogs: fix data lost with mke2fs -S

If we run the mke2fs with the -S option and the uninit_bg feature
simultaneously, the mke2fs marks blockgroups as uninitialized.
The e2fsck which run immediately after the mke2fs
removes all of the files.

To avoid this, the patch prohibits user from
setting the -S option and the uninit_bg feature simultaneously.

Usage example of the mke2fsk -S is as follows:

1. mke2fs -t ext4 -S -O ^uninit_bg DEV
2. tune2fs -O uninit_bg DEV
3. e2fsck DEV

#2 is not necessary only if the uninit_bg feature is not set
to ext4 originally.

Signed-off-by: Akira Fujita <[email protected]>
---
misc/mke2fs.c | 10 ++++++++++
1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 0ef2531..19f3684 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -1676,6 +1676,16 @@ profile_error:
if (tmp)
free(tmp);

+ if (super_only == 1 &&
+ EXT2_HAS_RO_COMPAT_FEATURE((&fs_param),
+ EXT4_FEATURE_RO_COMPAT_GDT_CSUM)) {
+ fprintf(stderr, _("The -S option and the uninit_bg feature "
+ "are not compatible.\n"
+ "They can not be set both "
+ "simultaneously.\n"));
+ exit(1);
+ }
+
/*
* We now need to do a sanity check of fs_blocks_count for
* 32-bit vs 64-bit block number support.


2012-02-17 04:20:41

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH 1/2] e2fsprogs: fix data lost with mke2fs -S

On Thu, Dec 22, 2011 at 05:30:22PM +0900, Akira Fujita wrote:
> If we run the mke2fs with the -S option and the uninit_bg feature
> simultaneously, the mke2fs marks blockgroups as uninitialized.
> The e2fsck which run immediately after the mke2fs
> removes all of the files.
>
> To avoid this, the patch prohibits user from
> setting the -S option and the uninit_bg feature simultaneously.

This is not the best fix. The best fix is to clear the itable_unused
fields in the block group descriptors if mke2fs -S is set. See below
for what I have in my tree.

- Ted

commit 9b6a158524fe82202bef6d0d8a101b47e6c02b64
Author: Theodore Ts'o <[email protected]>
Date: Thu Feb 16 23:16:34 2012 -0500

mke2fs: allow file systems w/ uninit_bg to be recovered with mke2fs -S

The command mke2fs -S is used as a last ditch recovery command to
write new superblock and block group descriptors, but _not_ to destroy
the inode table in hopes of recovering from a badly corrupted file
system. If the uninit_bg feature is enabled, we need to make sure to
clear the unused inodes count field in the block group descriptors or
else e2fsck -fy will leave the file system completely empty.

Thanks to Akira Fujita for reporting this problem.

Signed-off-by: "Theodore Ts'o" <[email protected]>

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 08789c4..c70c1b4 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -2434,6 +2434,17 @@ int main (int argc, char *argv[])
if (super_only) {
fs->super->s_state |= EXT2_ERROR_FS;
fs->flags &= ~(EXT2_FLAG_IB_DIRTY|EXT2_FLAG_BB_DIRTY);
+ /*
+ * The command "mke2fs -S" is used to recover
+ * corrupted file systems, so do not mark any of the
+ * inodes as unused; we want e2fsck to consider all
+ * inodes as potentially containing recoverable data.
+ */
+ if (fs->super->s_feature_ro_compat &
+ EXT4_FEATURE_RO_COMPAT_GDT_CSUM) {
+ for (i = 1; i < fs->group_desc_count; i++)
+ ext2fs_bg_itable_unused_set(fs, i, 0);
+ }
} else {
/* rsv must be a power of two (64kB is MD RAID sb alignment) */
blk64_t rsv = 65536 / fs->blocksize;

2012-02-21 07:09:50

by Akira Fujita

[permalink] [raw]
Subject: Re: [PATCH 1/2] e2fsprogs: fix data lost with mke2fs -S

Hi Ted,

> mke2fs: allow file systems w/ uninit_bg to be recovered with mke2fs -S
>
> The command mke2fs -S is used as a last ditch recovery command to
> write new superblock and block group descriptors, but _not_ to destroy
> the inode table in hopes of recovering from a badly corrupted file
> system. If the uninit_bg feature is enabled, we need to make sure to
> clear the unused inodes count field in the block group descriptors or
> else e2fsck -fy will leave the file system completely empty.
>
> Thanks to Akira Fujita for reporting this problem.

I tried to mkfs -S and e2fsck to salvage file
with e2fsprogs 1.42.1 (commit: 5ab348723247).
But it seemed that the file was removed after e2fsck.
Could you check my method (below script)?

Am I missing something?

---
#!/bin/bash

DEV="" #device
MP="" #mount point

mke2fs -t ext4 $DEV
mount -t ext4 $DEV $MP

dd if=/dev/urandom of=$MP/FILE bs=4K count=100 #create file on ext4
ls -l $MP
umount $DEV

dd if=/dev/zero of=$DEV bs=4K count=1 #clear primary SB

mke2fs -t ext4 -S -b 4096 $DEV #write out SB
mount -t ext4 $DEV $MP #can't mount because of the gdp checksum difference

e2fsck $DEV #can salvage?
mount -t ext4 $DEV $MP
ls -l $MP # It seems that "FILE" is removed
umount $DEV
---

Regards,
Akira Fujita

(2012/02/17 13:20), Ted Ts'o wrote:
> On Thu, Dec 22, 2011 at 05:30:22PM +0900, Akira Fujita wrote:
>> If we run the mke2fs with the -S option and the uninit_bg feature
>> simultaneously, the mke2fs marks blockgroups as uninitialized.
>> The e2fsck which run immediately after the mke2fs
>> removes all of the files.
>>
>> To avoid this, the patch prohibits user from
>> setting the -S option and the uninit_bg feature simultaneously.
>
> This is not the best fix. The best fix is to clear the itable_unused
> fields in the block group descriptors if mke2fs -S is set. See below
> for what I have in my tree.
>
> - Ted
>
> commit 9b6a158524fe82202bef6d0d8a101b47e6c02b64
> Author: Theodore Ts'o<[email protected]>
> Date: Thu Feb 16 23:16:34 2012 -0500
>
> mke2fs: allow file systems w/ uninit_bg to be recovered with mke2fs -S
>
> The command mke2fs -S is used as a last ditch recovery command to
> write new superblock and block group descriptors, but _not_ to destroy
> the inode table in hopes of recovering from a badly corrupted file
> system. If the uninit_bg feature is enabled, we need to make sure to
> clear the unused inodes count field in the block group descriptors or
> else e2fsck -fy will leave the file system completely empty.
>
> Thanks to Akira Fujita for reporting this problem.
>
> Signed-off-by: "Theodore Ts'o"<[email protected]>
>
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index 08789c4..c70c1b4 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -2434,6 +2434,17 @@ int main (int argc, char *argv[])
> if (super_only) {
> fs->super->s_state |= EXT2_ERROR_FS;
> fs->flags&= ~(EXT2_FLAG_IB_DIRTY|EXT2_FLAG_BB_DIRTY);
> + /*
> + * The command "mke2fs -S" is used to recover
> + * corrupted file systems, so do not mark any of the
> + * inodes as unused; we want e2fsck to consider all
> + * inodes as potentially containing recoverable data.
> + */
> + if (fs->super->s_feature_ro_compat&
> + EXT4_FEATURE_RO_COMPAT_GDT_CSUM) {
> + for (i = 1; i< fs->group_desc_count; i++)
> + ext2fs_bg_itable_unused_set(fs, i, 0);
> + }
> } else {
> /* rsv must be a power of two (64kB is MD RAID sb alignment) */
> blk64_t rsv = 65536 / fs->blocksize;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

2012-02-27 05:54:40

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH 1/2] e2fsprogs: fix data lost with mke2fs -S

On Tue, Feb 21, 2012 at 04:09:28PM +0900, Akira Fujita wrote:
>
> I tried to mkfs -S and e2fsck to salvage file
> with e2fsprogs 1.42.1 (commit: 5ab348723247).
> But it seemed that the file was removed after e2fsck.

Oops, sorry. I made a stupid mistake in my mke2fs patch. This should
fix things so that your script works correctly.

- Ted

commit 30ac1ce7df719e40b0c3c612696ada7c9ebbaed2
Author: Theodore Ts'o <[email protected]>
Date: Mon Feb 27 00:51:39 2012 -0500

mke2fs: make sure bg 0's unused inode count field is zero'ed for mke2fs -S

There was a bug/typo in commit ba9e0afc5 which caused the first block
group (bg #0) to not have its unused inode count field to get set to
zero in the case of mke2fs -S. This caused inodes in the first block
group to not be recoverable via mke2fs -S. Oops.

Signed-off-by: "Theodore Ts'o" <[email protected]>

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index c70c1b4..51435d2 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -2442,7 +2442,7 @@ int main (int argc, char *argv[])
*/
if (fs->super->s_feature_ro_compat &
EXT4_FEATURE_RO_COMPAT_GDT_CSUM) {
- for (i = 1; i < fs->group_desc_count; i++)
+ for (i = 0; i < fs->group_desc_count; i++)
ext2fs_bg_itable_unused_set(fs, i, 0);
}
} else {

2012-02-27 06:26:26

by Akira Fujita

[permalink] [raw]
Subject: Re: [PATCH 1/2] e2fsprogs: fix data lost with mke2fs -S

(2012/02/27 14:54), Ted Ts'o wrote:
> On Tue, Feb 21, 2012 at 04:09:28PM +0900, Akira Fujita wrote:
>>
>> I tried to mkfs -S and e2fsck to salvage file
>> with e2fsprogs 1.42.1 (commit: 5ab348723247).
>> But it seemed that the file was removed after e2fsck.
>
> Oops, sorry. I made a stupid mistake in my mke2fs patch. This should
> fix things so that your script works correctly.

Worked fine, thank you for you work. :)

Akira Fujita

> - Ted
>
> commit 30ac1ce7df719e40b0c3c612696ada7c9ebbaed2
> Author: Theodore Ts'o<[email protected]>
> Date: Mon Feb 27 00:51:39 2012 -0500
>
> mke2fs: make sure bg 0's unused inode count field is zero'ed for mke2fs -S
>
> There was a bug/typo in commit ba9e0afc5 which caused the first block
> group (bg #0) to not have its unused inode count field to get set to
> zero in the case of mke2fs -S. This caused inodes in the first block
> group to not be recoverable via mke2fs -S. Oops.
>
> Signed-off-by: "Theodore Ts'o"<[email protected]>
>
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index c70c1b4..51435d2 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -2442,7 +2442,7 @@ int main (int argc, char *argv[])
> */
> if (fs->super->s_feature_ro_compat&
> EXT4_FEATURE_RO_COMPAT_GDT_CSUM) {
> - for (i = 1; i< fs->group_desc_count; i++)
> + for (i = 0; i< fs->group_desc_count; i++)
> ext2fs_bg_itable_unused_set(fs, i, 0);
> }
> } else {
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>