From: Arthur Jones <[email protected]>
In ext3_sync_fs, we only wait for a commit to finish if we started it, but
there may be one already in progress which will not be synced.
In the case of a data=ordered umount with pending long symlinks which are
delayed due to a long list of other I/O on the backing block device, this
causes the buffer associated with the long symlinks to not be moved to the
inode dirty list in the second phase of fsync_super. Then, before they
can be dirtied again, kjournald exits, seeing the UMOUNT flag and the
dirty pages are never written to the backing block device, causing long
symlink corruption and exposing new or previously freed block data to
userspace.
This can be reproduced with a script created
by Eric Sandeen <[email protected]>:
#!/bin/bash
umount /mnt/test2
mount /dev/sdb4 /mnt/test2
rm -f /mnt/test2/*
dd if=/dev/zero of=/mnt/test2/bigfile bs=1M count=512
touch
/mnt/test2/thisisveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryverylongfilename
ln -s
/mnt/test2/thisisveryveryveryveryveryveryveryveryveryveryveryveryveryveryveryverylongfilename
/mnt/test2/link
umount /mnt/test2
mount /dev/sdb4 /mnt/test2
ls /mnt/test2/
umount /mnt/test2
To ensure all commits are synced, we flush all journal commits now when
sync_fs'ing ext3.
Signed-off-by: Arthur Jones <[email protected]>
Cc: Eric Sandeen <[email protected]>
Cc: Theodore Ts'o <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]> [2.6.everything]
Signed-off-by: Andrew Morton <[email protected]>
---
fs/ext3/super.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff -puN fs/ext3/super.c~ext3-wait-on-all-pending-commits-in-ext3_sync_fs fs/ext3/super.c
--- a/fs/ext3/super.c~ext3-wait-on-all-pending-commits-in-ext3_sync_fs
+++ a/fs/ext3/super.c
@@ -2390,13 +2390,12 @@ static void ext3_write_super (struct sup
static int ext3_sync_fs(struct super_block *sb, int wait)
{
- tid_t target;
My version of this patch also cleaned up the following comment, which
has been wrong since 2.5.70 or thereabouts...
This isn't urgent, so could you just queue this up for the next merge
window in the -mm tree?
- Ted
ext3: Clean up outdated and incorrect comment for ext3_write_super()
Signed-off-by: "Theodore Ts'o" <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: <[email protected]>
---
diff --git a/fs/ext3/super.c b/fs/ext3/super.c
index e5717a4..296c044 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -2375,12 +2375,9 @@ int ext3_force_commit(struct super_block *sb)
/*
* Ext3 always journals updates to the superblock itself, so we don't
* have to propagate any other updates to the superblock on disk at this
- * point. Just start an async writeback to get the buffers on their way
- * to the disk.
- *
- * This implicitly triggers the writebehind on sync().
+ * point. (We can probably nuke this function altogether, and remove
+ * any mention to sb->s_dirt in all of fs/ext3; eventual cleanup...)
*/
On Thu, 6 Nov 2008 16:44:31 -0500
Theodore Tso <[email protected]> wrote:
> My version of this patch also cleaned up the following comment, which
> has been wrong since 2.5.70 or thereabouts...
oh, I didn't spot that.
I looked at the version in linux-next and saw that it was propagating
the error value back to the VFS as well. Or maybe that was done in a
separate patch, dunno. But while that's a good change, I felt that we
should separate it from this bugfix. I meant to mention it but I
forgot, sorry.
> This isn't urgent, so could you just queue this up for the next merge
> window in the -mm tree?
>
> - Ted
>
> ext3: Clean up outdated and incorrect comment for ext3_write_super()
>
> Signed-off-by: "Theodore Ts'o" <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: <[email protected]>
> ---
> diff --git a/fs/ext3/super.c b/fs/ext3/super.c
> index e5717a4..296c044 100644
> --- a/fs/ext3/super.c
> +++ b/fs/ext3/super.c
> @@ -2375,12 +2375,9 @@ int ext3_force_commit(struct super_block *sb)
> /*
> * Ext3 always journals updates to the superblock itself, so we don't
> * have to propagate any other updates to the superblock on disk at this
> - * point. Just start an async writeback to get the buffers on their way
> - * to the disk.
> - *
> - * This implicitly triggers the writebehind on sync().
> + * point. (We can probably nuke this function altogether, and remove
> + * any mention to sb->s_dirt in all of fs/ext3; eventual cleanup...)
> */
> -
> static void ext3_write_super (struct super_block * sb)
> {
> if (mutex_trylock(&sb->s_lock) != 0)
Sure.
On Thu, Nov 06, 2008 at 02:20:20PM -0800, Andrew Morton wrote:
> On Thu, 6 Nov 2008 16:44:31 -0500
> Theodore Tso <[email protected]> wrote:
>
> > My version of this patch also cleaned up the following comment, which
> > has been wrong since 2.5.70 or thereabouts...
>
> oh, I didn't spot that.
>
> I looked at the version in linux-next and saw that it was propagating
> the error value back to the VFS as well. Or maybe that was done in a
> separate patch, dunno. But while that's a good change, I felt that we
> should separate it from this bugfix. I meant to mention it but I
> forgot, sorry.
Fair enough. Propagating the return value to the VFS is also a
cleanup, although given that the VFS drops return value on the floor,
I considered it a risk-free change and included it in the ext4 version
of the patch. But yeah, I can see why separating could be argued to
be the right thing.
So that means we'll need two cleanup patches for ext3; one to fix the
comment, and another to propagate the error code back to the VFS.
- Ted
Hi Ted, ...
On Thu, Nov 06, 2008 at 02:42:02PM -0800, Theodore Tso wrote:
> [...]
> So that means we'll need two cleanup patches for ext3; one to fix the
> comment, and another to propagate the error code back to the VFS.
A trivial patch for this is in the linux-ext4
mailing list:
http://marc.info/?l=linux-kernel&m=122574905024321&w=2
Arthur