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.
Signed-off-by: Kalpak Shah <[email protected]>
Index: e2fsprogs-1.39/e2fsck/pass2.c
===================================================================
--- e2fsprogs-1.39.orig/e2fsck/pass2.c
+++ e2fsprogs-1.39/e2fsck/pass2.c
@@ -690,7 +690,10 @@ static void salvage_directory(ext2_filsy
*/
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;
return;
}
/*
Thanks,
Kalpak.
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
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.
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.:
<tytso.root@candygram> {/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
<tytso.root@candygram> {/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
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.
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
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
>
>
>