ext2fs_link() corrupting a directory

ext2fs_link() corrupting a directory

[Jan Kara]

Hello,

I was tracking down a filesystem corruption issue with one proposed
xfstests testcase. After some debugging I've found out that the problem
actually is not in the kernel but rather in e2fsck (libext2fs
respectively). The testcase deletes lost+found, e2fsck recreates it. But
after the testcase / is h-tree directory. So ext2fs_link() creates
lost+found in / and clears EXT4_INODE_INDEX flag. Now because the
filesystem has metadata checksums, clearing the index flag needs to also
rewrite all directory blocks with h-tree index blocks because the layout
now needs to be different.  ext2fs_link() actually tries to do this in its
link_proc() but if the space for new directory entry is found before we
walk all the h-tree index blocks, we terminate the iteration and some index
blocks remain unconverted resulting in checksum errors and other weirdness
later on.

The question is how to best fix this. The easiest fix is to just make
link_proc() iterate through all directory blocks when it needs to do the
index blocks conversion. But this seems somewhat stupid. Also there's
another problem with clearing EXT4_INODE_INDEX in ext2fs_link() - if the
directory has more than 65000 subdirectories, clearing EXT4_INODE_INDEX is
not allowed because large directory link count handling is supported only
for EXT4_INODE_INDEX directories.

So what do we do with this? For e2fsck, we could just link the new entry
into the directory and force rehashing later. But ext2fs_link() can be
called also from other tools and it should be a self-contained API... Any
ideas? Should we just bite the bullet and implement ext2fs_link() for
h-tree dirs properly?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: ext2fs_link() corrupting a directory

[Theodore Y. Ts'o]

On Fri, Jan 17, 2020 at 01:24:20PM +0100, Jan Kara wrote:
> 
> I was tracking down a filesystem corruption issue with one proposed
> xfstests testcase. After some debugging I've found out that the problem
> actually is not in the kernel but rather in e2fsck (libext2fs
> respectively). The testcase deletes lost+found, e2fsck recreates it. But
> after the testcase / is h-tree directory. So ext2fs_link() creates
> lost+found in / and clears EXT4_INODE_INDEX flag. Now because the
> filesystem has metadata checksums, clearing the index flag needs to also
> rewrite all directory blocks with h-tree index blocks because the layout
> now needs to be different.  ext2fs_link() actually tries to do this in its
> link_proc() but if the space for new directory entry is found before we
> walk all the h-tree index blocks, we terminate the iteration and some index
> blocks remain unconverted resulting in checksum errors and other weirdness
> later on.

Ouch.  Nice catch.

I suspect we have a similar problem when the kernel discovers that the
htree is corrupted; it will clear EXT4_INODE_INDEX flag, since before
how we added metadata checksum, it was safe to do that.  Given our
choice of where the stash the checksum, in order to not decrease the
fanout of htree directories, this is no no longer safe to do.

I think what we will need to do is to set an in-memory "fallback"
flag, which simply ignores the index blocks, but doesn't actually clear
the EXT4_INODE_INDEX flag in the case where metadata_csum is enabled.

> The question is how to best fix this. The easiest fix is to just make
> link_proc() iterate through all directory blocks when it needs to do the
> index blocks conversion. But this seems somewhat stupid. Also there's
> another problem with clearing EXT4_INODE_INDEX in ext2fs_link() - if the
> directory has more than 65000 subdirectories, clearing EXT4_INODE_INDEX is
> not allowed because large directory link count handling is supported only
> for EXT4_INODE_INDEX directories.
> 
> So what do we do with this? For e2fsck, we could just link the new entry
> into the directory and force rehashing later. But ext2fs_link() can be
> called also from other tools and it should be a self-contained API... Any
> ideas? Should we just bite the bullet and implement ext2fs_link() for
> h-tree dirs properly?

Yes, I think that's what we are going to need to do; we had cheated
and not bothered to implement htree support in ext2fs library, but if
we're going to implement it for link_proc, we might as well also
implement it for lookups as well.

					- Ted

Re: ext2fs_link() corrupting a directory

[Jan Kara]

On Fri 17-01-20 11:29:27, Theodore Y. Ts'o wrote:
> On Fri, Jan 17, 2020 at 01:24:20PM +0100, Jan Kara wrote:
> > 
> > I was tracking down a filesystem corruption issue with one proposed
> > xfstests testcase. After some debugging I've found out that the problem
> > actually is not in the kernel but rather in e2fsck (libext2fs
> > respectively). The testcase deletes lost+found, e2fsck recreates it. But
> > after the testcase / is h-tree directory. So ext2fs_link() creates
> > lost+found in / and clears EXT4_INODE_INDEX flag. Now because the
> > filesystem has metadata checksums, clearing the index flag needs to also
> > rewrite all directory blocks with h-tree index blocks because the layout
> > now needs to be different.  ext2fs_link() actually tries to do this in its
> > link_proc() but if the space for new directory entry is found before we
> > walk all the h-tree index blocks, we terminate the iteration and some index
> > blocks remain unconverted resulting in checksum errors and other weirdness
> > later on.
> 
> Ouch.  Nice catch.
> 
> I suspect we have a similar problem when the kernel discovers that the
> htree is corrupted; it will clear EXT4_INODE_INDEX flag, since before
> how we added metadata checksum, it was safe to do that.  Given our
> choice of where the stash the checksum, in order to not decrease the
> fanout of htree directories, this is no no longer safe to do.
> 
> I think what we will need to do is to set an in-memory "fallback"
> flag, which simply ignores the index blocks, but doesn't actually clear
> the EXT4_INODE_INDEX flag in the case where metadata_csum is enabled.

Yeah, that might be a good fix for the kernel.

> > The question is how to best fix this. The easiest fix is to just make
> > link_proc() iterate through all directory blocks when it needs to do the
> > index blocks conversion. But this seems somewhat stupid. Also there's
> > another problem with clearing EXT4_INODE_INDEX in ext2fs_link() - if the
> > directory has more than 65000 subdirectories, clearing EXT4_INODE_INDEX is
> > not allowed because large directory link count handling is supported only
> > for EXT4_INODE_INDEX directories.
> > 
> > So what do we do with this? For e2fsck, we could just link the new entry
> > into the directory and force rehashing later. But ext2fs_link() can be
> > called also from other tools and it should be a self-contained API... Any
> > ideas? Should we just bite the bullet and implement ext2fs_link() for
> > h-tree dirs properly?
> 
> Yes, I think that's what we are going to need to do; we had cheated
> and not bothered to implement htree support in ext2fs library, but if
> we're going to implement it for link_proc, we might as well also
> implement it for lookups as well.

JFYI (since I won't make today's ext4 call) I have full-featured insertion
into indexed dirs implemented, just need to write some decent testing
because it's quite a bit of new code and not exactly trivial...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR