Re: [e2fsprogs] Bug in salvage_directory
On Mon, 2007-07-09 at 19:02 -0400, Theodore Tso wrote: On Mon, Jul 09, 2007 at 11:22:05PM +0530, Kalpak Shah wrote: Yes even prev-rec_len cannot be beyond fs-blocksize. I do have the corrupt filesystem image but it is a large one. This patch certainly works well and corrects the problem in a single run of e2fsck. When you say this patch, I assume you meant the patch I wrote as opposed to the one you submitted, right? Yes, I meant the patch you wrote. Thanks, Kalpak. In any case, I've created a test case (attached) which is fixed in a single run of e2fsck, but which your patch requires two runs to fix. So I will be committing my patch into the tree. Regards, - Ted - 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
Re: [e2fsprogs] Bug in salvage_directory
On Mon, Jul 09, 2007 at 03:02:02PM +0530, Kalpak Shah wrote:
Hi Ted,
Recently, one of our customers found this message in pass2 of e2fsck while
doing some regression testing:
Entry '4, 0x695a, 0x81ff, 0x0040, 0x8320, 0xa192, 0x0021' in ??? (136554) has
rec_len of 14200, should be 26908.
Both the displayed rec_len and the should be value are bogus. The
reason is that salvage_directory sets a offset beyond blocksize
leading to bogus messages.
Do you have a test case where this happens? I don't think your patch
is right, because if dirent-rec_len is too big, this yes, your patch
will make sure offset doesn't get set beyond fs-blocksize, but it
ends up leaving prev-rec_len also pointing beyond fs-blocksize ---
which means a 2nd e2fsck should result in a complaint about that.
if (prev dirent-rec_len (dirent-rec_len % 4) == 0) {
prev-rec_len += dirent-rec_len;
^^^
- *offset += dirent-rec_len;
+ if (*offset + dirent-rec_len = fs-blocksize)
+ *offset += dirent-rec_len;
+ else
+ *offset = fs-blocksize;
I think this is a better fix for the problem:
diff --git a/e2fsck/pass2.c b/e2fsck/pass2.c
index e235348..5e088e2 100644
--- a/e2fsck/pass2.c
+++ b/e2fsck/pass2.c
@@ -675,11 +675,12 @@ static void salvage_directory(ext2_filsys fs,
return;
}
/*
-* If the directory entry is a multiple of four, so it is
-* valid, let the previous directory entry absorb the invalid
-* one.
+* If the record length of the directory entry is a multiple
+* of four, and not too big, such that it is valid, let the
+* previous directory entry absorb the invalid one.
*/
- if (prev dirent-rec_len (dirent-rec_len % 4) == 0) {
+ if (prev dirent-rec_len (dirent-rec_len % 4) == 0
+ (*offset + dirent-rec_len = fs-blocksize)) {
prev-rec_len += dirent-rec_len;
*offset += dirent-rec_len;
return;
If the dirent-rec_len is too big, then the default salvage method
which follows will do the right thing.
I'd like to have a test case to make sure this works, though, so if
you have a quick test case whipped up, that would be great. Otherwise
I'll have to cons one up when I have a moment.
Thanks, regards,
- Ted
-
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
Re: [e2fsprogs] Bug in salvage_directory
On Mon, 2007-07-09 at 12:50 -0400, Theodore Tso wrote: On Mon, Jul 09, 2007 at 03:02:02PM +0530, Kalpak Shah wrote: Hi Ted, Recently, one of our customers found this message in pass2 of e2fsck while doing some regression testing: Entry '4, 0x695a, 0x81ff, 0x0040, 0x8320, 0xa192, 0x0021' in ??? (136554) has rec_len of 14200, should be 26908. Both the displayed rec_len and the should be value are bogus. The reason is that salvage_directory sets a offset beyond blocksize leading to bogus messages. Do you have a test case where this happens? I don't think your patch is right, because if dirent-rec_len is too big, this yes, your patch will make sure offset doesn't get set beyond fs-blocksize, but it ends up leaving prev-rec_len also pointing beyond fs-blocksize --- which means a 2nd e2fsck should result in a complaint about that. Yes even prev-rec_len cannot be beyond fs-blocksize. I do have the corrupt filesystem image but it is a large one. This patch certainly works well and corrects the problem in a single run of e2fsck. Thanks, Kalpak. - 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
Re: [e2fsprogs] Bug in salvage_directory
On Mon, Jul 09, 2007 at 11:22:05PM +0530, Kalpak Shah wrote:
On Mon, 2007-07-09 at 12:50 -0400, Theodore Tso wrote:
On Mon, Jul 09, 2007 at 03:02:02PM +0530, Kalpak Shah wrote:
Hi Ted,
Recently, one of our customers found this message in pass2 of e2fsck
while doing some regression testing:
Entry '4, 0x695a, 0x81ff, 0x0040, 0x8320, 0xa192, 0x0021' in ???
(136554) has
rec_len of 14200, should be 26908.
Both the displayed rec_len and the should be value are bogus. The
reason is that salvage_directory sets a offset beyond blocksize
leading to bogus messages.
Do you have a test case where this happens? I don't think your patch
is right, because if dirent-rec_len is too big, this yes, your patch
will make sure offset doesn't get set beyond fs-blocksize, but it
ends up leaving prev-rec_len also pointing beyond fs-blocksize ---
which means a 2nd e2fsck should result in a complaint about that.
Yes even prev-rec_len cannot be beyond fs-blocksize.
Really? Even after this:
prev-rec_len += dirent-rec_len;
^^^
... when *offset + dirent-rec_len fs-blocksize? If the else part
of your conditional triggers, then dirent-rec_len is too big; it
could potentially be huge. So just blindly adding that invalid value
to prev-rec_len can't be right.
I do have the corrupt filesystem image but it is a large one.
Can you use debugfs's dump command to dump out the contents of the
directory in question? i.e.:
[EMAIL PROTECTED] {/usr/projects/ext4-patch-queue}, level 2 [master]
504# debugfs /dev/sda2
debugfs 1.40.1 (08-Jul-2007)
debugfs: dump /home/tytso/isync/mit/new /tmp/new-dir.img
debugfs: q
[EMAIL PROTECTED] {/usr/projects/ext4-patch-queue}, level 2 [master]
505# ls -l /tmp/new-dir.img
408 -rw-r--r-- 1 root root 409600 2007-07-09 14:28 /tmp/new-dir.img
- Ted
-
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
Re: [e2fsprogs] Bug in salvage_directory
On Jul 09, 2007 14:29 -0400, Theodore Tso wrote: On Mon, Jul 09, 2007 at 11:22:05PM +0530, Kalpak Shah wrote: Yes even prev-rec_len cannot be beyond fs-blocksize. Really? Even after this: prev-rec_len += dirent-rec_len; ^^^ I think Kalpak was agreeing with you... Cheers, Andreas -- Andreas Dilger Principal Software Engineer Cluster File Systems, Inc. - 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
Re: [e2fsprogs] Bug in salvage_directory
On Mon, Jul 09, 2007 at 01:17:33PM -0600, Andreas Dilger wrote: On Jul 09, 2007 14:29 -0400, Theodore Tso wrote: On Mon, Jul 09, 2007 at 11:22:05PM +0530, Kalpak Shah wrote: Yes even prev-rec_len cannot be beyond fs-blocksize. Really? Even after this: prev-rec_len += dirent-rec_len; ^^^ I think Kalpak was agreeing with you... Sorry, I misread his note. - Ted - 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
