fs: fix filesystem_sync vs write race on rw=>ro remount

PATCH In review Dmitry Monakhov 4 participants 8 messages Jan 24, 2010 – Feb 9, 2010 VFS

lore

Share & tools

lore

Dmitry Monakhov

author

2010-01-24 11:41 UTC #0 lore raw

PATCH 1/1 fs: fix filesystem_sync vs write race on rw=>ro remount

Currently on rw=>ro remount we have following race

| mount /mnt -oremount,ro | write-task |
|-------------------------+------------|
|                         | open(RDWR) |
| shrink_dcache_sb(sb);   |            |
| sync_filesystem(sb);    |            |
|                         | write()    |
|                         | close()    |
| fs_may_remount_ro(sb)   |            |
| sb->s_flags = new_flags |            |

Later writeback or sync() will result in error due to MS_RDONLY flag In case of ext4 this result in jbd2_start failure on writeback
ext4_da_writepages: jbd2_start: 1024 pages, ino 1431; err -30 In fact all others are affected by this error but it is not visible because the skip s_flags check on writeback. For example ext3 check (s_flags & MS_RDONLY) only if page has no buffers during journal start.

In order to prevent the race we have to block new writers before fs_may_remount_ro() and sync_filesystem(). Let's introduce new sb->s_flags MS_RO_REMOUNT flag for this purpose. But suddenly we have no available space in MS_XXX bits, let's share this bit with MS_REMOUNT. This is possible because MS_REMOUNT used only for passing arguments from flags to sys_mount() and never used in sb->s_flags.

##TESTCASE_BEGIN:
#! /bin/bash -x
DEV=/dev/sdb5
FSTYPE=ext4
BINDIR=/home/dmon
MNTOPT="data=ordered"
umount /mnt
mkfs.${FSTYPE} ${DEV} || exit 1
mount ${DEV} /mnt -o${MNTOPT} || exit 1 ${BINDIR}/fsstress -p1 -l999999999 -n9999999999 -d /mnt/test & sleep 15
mount /mnt -oremount,ro,${MNTOPT}
sleep 1
killall -9 fsstress
sync

# after this you may get following message in dmesg
# "ext4_da_writepages: jbd2_start: 1024 pages, ino 1431; err -30"
##TESTCASE_END

Signed-off-by Dmitry Monakhov <[email protected]>

fs/namespace.c +1 -1

index c768f73..a216fb3 100644
@@ -194,7 +194,7 @@ int __mnt_is_readonly(struct vfsmount *mnt)
 {
 	if (mnt->mnt_flags & MNT_READONLY)
 		return 1;
-	if (mnt->mnt_sb->s_flags & MS_RDONLY)
+	if (mnt->mnt_sb->s_flags & (MS_RDONLY| MS_RO_REMOUNT))
 		return 1;
 	return 0;
 }

fs/super.c +18 -9

index aff046b..756fe88 100644
@@ -569,42 +569,51 @@ int do_remount_sb(struct super_block *sb, int flags, void *data, int force)
 {
 	int retval;
 	int remount_rw;
+	int remount_ro;
 
 	if (sb->s_frozen != SB_UNFROZEN)
 		return -EBUSY;
-
+	remount_ro = (flags & MS_RDONLY) && !(sb->s_flags & MS_RDONLY);
 #ifdef CONFIG_BLOCK
 	if (!(flags & MS_RDONLY) && bdev_read_only(sb->s_bdev))
 		return -EACCES;
 #endif
-
 	if (flags & MS_RDONLY)
 		acct_auto_close(sb);
-	shrink_dcache_sb(sb);
-	sync_filesystem(sb);
 
 	/* If we are remounting RDONLY and current sb is read/write,
 	   make sure there are no rw files opened */
-	if ((flags & MS_RDONLY) && !(sb->s_flags & MS_RDONLY)) {
+	retval = -EBUSY;
+	if (remount_ro) {
+		/* Prevent new writers before check */
+		sb->s_flags |= MS_RO_REMOUNT;
 		if (force)
 			mark_files_ro(sb);
 		else if (!fs_may_remount_ro(sb))
-			return -EBUSY;
+			goto out;
+	}
+	shrink_dcache_sb(sb);
+	sync_filesystem(sb);
+
+	if (remount_ro) {
 		retval = vfs_dq_off(sb, 1);
 		if (retval < 0 && retval != -ENOSYS)
-			return -EBUSY;
+			goto out;
 	}
 	remount_rw = !(flags & MS_RDONLY) && (sb->s_flags & MS_RDONLY);
 
 	if (sb->s_op->remount_fs) {
 		retval = sb->s_op->remount_fs(sb, &flags, data);
 		if (retval)
-			return retval;
+			goto out;
 	}
 	sb->s_flags = (sb->s_flags & ~MS_RMT_MASK) | (flags & MS_RMT_MASK);
 	if (remount_rw)
 		vfs_dq_quota_on_remount(sb);
-	return 0;
+out:
+	if (remount_ro)
+		sb->s_flags = (sb->s_flags & ~MS_RO_REMOUNT);
+	return retval;
 }
 
 static void do_emergency_remount(struct work_struct *work)

include/linux/fs.h +3 -0

index b1bcb27..a613875 100644
@@ -208,6 +208,9 @@ struct inodes_stat_t {
 #define MS_STRICTATIME	(1<<24) /* Always perform atime updates */
 #define MS_ACTIVE	(1<<30)
 #define MS_NOUSER	(1<<31)
+#define MS_RO_REMOUNT	MS_REMOUNT /* Alter flags from rw=>ro of mounted FS.
+				      Not conflicting with MS_REMOUNT because
+				      it never stored in sb->s_flags */
 
 /*
  * Superblock flags that can be altered by MS_REMOUNT

Dmitry Monakhov

author

2010-01-24 11:50 UTC #1 lore raw

Dmitry Monakhov <[email protected]> writes:

As soon as i understand all kernel version are affected, at least I'm able to reproduce the bug on 2.6.29..2.6.33-rc4

Quoted 137 lines

Currently on rw=>ro remount we have following race

| mount /mnt -oremount,ro | write-task |
|-------------------------+------------|
|                         | open(RDWR) |
| shrink_dcache_sb(sb);   |            |
| sync_filesystem(sb);    |            |
|                         | write()    |
|                         | close()    |
| fs_may_remount_ro(sb)   |            |
| sb->s_flags = new_flags |            |

# after this you may get following message in dmesg
# "ext4_da_writepages: jbd2_start: 1024 pages, ino 1431; err -30"
##TESTCASE_END

Signed-off-by Dmitry Monakhov <[email protected]>

fs/namespace.c +1 -1

index c768f73..a216fb3 100644
@@ -194,7 +194,7 @@ int __mnt_is_readonly(struct vfsmount *mnt)
 {
 	if (mnt->mnt_flags & MNT_READONLY)
 		return 1;
-	if (mnt->mnt_sb->s_flags & MS_RDONLY)
+	if (mnt->mnt_sb->s_flags & (MS_RDONLY| MS_RO_REMOUNT))
 		return 1;
 	return 0;
 }

fs/super.c +18 -9

index aff046b..756fe88 100644
@@ -569,42 +569,51 @@ int do_remount_sb(struct super_block *sb, int flags, void *data, int force)
 {
 	int retval;
 	int remount_rw;
+	int remount_ro;
 
 	if (sb->s_frozen != SB_UNFROZEN)
 		return -EBUSY;
-
+	remount_ro = (flags & MS_RDONLY) && !(sb->s_flags & MS_RDONLY);
 #ifdef CONFIG_BLOCK
 	if (!(flags & MS_RDONLY) && bdev_read_only(sb->s_bdev))
 		return -EACCES;
 #endif
-
 	if (flags & MS_RDONLY)
 		acct_auto_close(sb);
-	shrink_dcache_sb(sb);
-	sync_filesystem(sb);
 
 	/* If we are remounting RDONLY and current sb is read/write,
 	   make sure there are no rw files opened */
-	if ((flags & MS_RDONLY) && !(sb->s_flags & MS_RDONLY)) {
+	retval = -EBUSY;
+	if (remount_ro) {
+		/* Prevent new writers before check */
+		sb->s_flags |= MS_RO_REMOUNT;
 		if (force)
 			mark_files_ro(sb);
 		else if (!fs_may_remount_ro(sb))
-			return -EBUSY;
+			goto out;
+	}
+	shrink_dcache_sb(sb);
+	sync_filesystem(sb);
+
+	if (remount_ro) {
 		retval = vfs_dq_off(sb, 1);
 		if (retval < 0 && retval != -ENOSYS)
-			return -EBUSY;
+			goto out;
 	}
 	remount_rw = !(flags & MS_RDONLY) && (sb->s_flags & MS_RDONLY);
 
 	if (sb->s_op->remount_fs) {
 		retval = sb->s_op->remount_fs(sb, &flags, data);
 		if (retval)
-			return retval;
+			goto out;
 	}
 	sb->s_flags = (sb->s_flags & ~MS_RMT_MASK) | (flags & MS_RMT_MASK);
 	if (remount_rw)
 		vfs_dq_quota_on_remount(sb);
-	return 0;
+out:
+	if (remount_ro)
+		sb->s_flags = (sb->s_flags & ~MS_RO_REMOUNT);
+	return retval;
 }
 
 static void do_emergency_remount(struct work_struct *work)

include/linux/fs.h +3 -0

index b1bcb27..a613875 100644
@@ -208,6 +208,9 @@ struct inodes_stat_t {
 #define MS_STRICTATIME	(1<<24) /* Always perform atime updates */
 #define MS_ACTIVE	(1<<30)
 #define MS_NOUSER	(1<<31)
+#define MS_RO_REMOUNT	MS_REMOUNT /* Alter flags from rw=>ro of mounted FS.
+				      Not conflicting with MS_REMOUNT because
+				      it never stored in sb->s_flags */
 
 /*
  * Superblock flags that can be altered by MS_REMOUNT

Al Viro

maintainer

2010-01-24 19:53 UTC #2 lore raw

In reply to Dmitry Monakhov

Quoted 17 lines · Dmitry Monakhov

Currently on rw=>ro remount we have following race
| mount /mnt -oremount,ro | write-task |
|-------------------------+------------|
|                         | open(RDWR) |
| shrink_dcache_sb(sb);   |            |
| sync_filesystem(sb);    |            |
|                         | write()    |
|                         | close()    |
| fs_may_remount_ro(sb)   |            |
| sb->s_flags = new_flags |            |
Later writeback or sync() will result in error due to MS_RDONLY flag In case of ext4 this result in jbd2_start failure on writeback
ext4_da_writepages: jbd2_start: 1024 pages, ino 1431; err -30 In fact all others are affected by this error but it is not visible because the skip s_flags check on writeback. For example ext3 check (s_flags & MS_RDONLY) only if page has no buffers during journal start.

Quoted 6 lines

In order to prevent the race we have to block new writers before fs_may_remount_ro() and sync_filesystem(). Let's introduce new sb->s_flags MS_RO_REMOUNT flag for this purpose. But suddenly we have no available space in MS_XXX bits, let's share this bit with MS_REMOUNT. This is possible because MS_REMOUNT used only for passing arguments from flags to sys_mount() and never used in sb->s_flags.

It's not a solution. You get an _attempted_ remount ro making writes fail, even if it's going to be unsuccessful. No go...

Dmitry Monakhov

author

2010-01-24 21:15 UTC #3 lore raw

Quoted 25 lines · Al Viro

Quoted 17 lines · Dmitry Monakhov
Currently on rw=>ro remount we have following race
| mount /mnt -oremount,ro | write-task |
|-------------------------+------------|
|                         | open(RDWR) |
| shrink_dcache_sb(sb);   |            |
| sync_filesystem(sb);    |            |
|                         | write()    |
|                         | close()    |
| fs_may_remount_ro(sb)   |            |
| sb->s_flags = new_flags |            |
Later writeback or sync() will result in error due to MS_RDONLY flag In case of ext4 this result in jbd2_start failure on writeback
ext4_da_writepages: jbd2_start: 1024 pages, ino 1431; err -30 In fact all others are affected by this error but it is not visible because the skip s_flags check on writeback. For example ext3 check (s_flags & MS_RDONLY) only if page has no buffers during journal start.
Quoted 6 lines
In order to prevent the race we have to block new writers before fs_may_remount_ro() and sync_filesystem(). Let's introduce new sb->s_flags MS_RO_REMOUNT flag for this purpose. But suddenly we have no available space in MS_XXX bits, let's share this bit with MS_REMOUNT. This is possible because MS_REMOUNT used only for passing arguments from flags to sys_mount() and never used in sb->s_flags.

Quoted 2 lines

It's not a solution. You get an _attempted_ remount ro making writes fail, even if it's going to be unsuccessful. No go...

We have two options for new writers:
1) Fail it via -EROFS
Yes, remount may fail, but it is really unlikely.
2) Defer(block) new writers on until we complete or fail remount for example like follows. Do you like second solution ?

fs/namespace.c +9 -0

index c768f73..daf3c5a 100644
@@ -196,6 +196,15 @@ int __mnt_is_readonly(struct vfsmount *mnt)
 		return 1;
 	if (mnt->mnt_sb->s_flags & MS_RDONLY)
 		return 1;
+	if (mnt->mnt_sb->s_flags & MS_RO_REMOUNT) {
+		int ret = 0;
+		/* Serialize against remount */
+		down_read(&mnt->mnt_sb->s_umount);
+		if (mnt->mnt_sb->s_flags & MS_RDONLY)
+			ret = 1;
+		up_read(&mnt->mnt_sb->s_umount);
+		return ret;
+	}
 	return 0;
 }
 EXPORT_SYMBOL_GPL(__mnt_is_readonly);

Al Viro

maintainer

2010-01-24 21:37 UTC #4 lore raw

Quoted 7 lines · Dmitry Monakhov

Quoted 2 lines
It's not a solution. You get an _attempted_ remount ro making writes fail, even if it's going to be unsuccessful. No go...
We have two options for new writers:
1) Fail it via -EROFS
Yes, remount may fail, but it is really unlikely.
2) Defer(block) new writers on until we complete or fail remount for example like follows. Do you like second solution ?

Umm... I wonder what the locking implications would be... Frankly, I suspect that what we really want is this:
* per-superblock write count of some kind, bumped when we decide
that writeback is inevitable and dropped when we are done with it (the same thing goes for async part of unlink(), etc.)
* fs_may_remount_ro() checking that write count
So basically we try to push those short-term writers to completion and if new ones had come while we'd been doing that (or some are really stuck) we fail remount with -EBUSY.

As a short-term solution the second patch would do probably (-stable and .33), but in the next cycle I'd rather see something addressing the real problem. fs_may_remount_ro() in its current form is really broken by design - it should not scan any lists (which is where your race comes from, BTW)

Dave Chinner

2010-01-24 22:40 UTC #5 lore raw

Quoted 10 lines · Al Viro

Quoted 7 lines · Dmitry Monakhov
Quoted 2 lines
It's not a solution. You get an _attempted_ remount ro making writes fail, even if it's going to be unsuccessful. No go...
We have two options for new writers:
1) Fail it via -EROFS
Yes, remount may fail, but it is really unlikely.
2) Defer(block) new writers on until we complete or fail remount for example like follows. Do you like second solution ?

Quoted 9 lines

Umm... I wonder what the locking implications would be... Frankly, I suspect that what we really want is this:
* per-superblock write count of some kind, bumped when we decide
that writeback is inevitable and dropped when we are done with it (the same thing goes for async part of unlink(), etc.)
* fs_may_remount_ro() checking that write count
So basically we try to push those short-term writers to completion and if new ones had come while we'd been doing that (or some are really stuck) we fail remount with -EBUSY.

Perhaps we could utilise the filesystem freeze infrastructure - it already has hooks for intercepting new writers and modifcations, and filesystems have to flush any current modifications before the freeze completes. It sounds very similar to the requirements needed here...

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Jan Kara

reviewer

2010-02-09 15:28 UTC #6 lore raw

Quoted 21 lines · Al Viro

Quoted 10 lines · Dmitry Monakhov
Quoted 8 lines
Quoted 3 lines
It's not a solution. You get an _attempted_ remount ro making writes fail, even if it's going to be unsuccessful. No go...
We have two options for new writers:
1) Fail it via -EROFS
Yes, remount may fail, but it is really unlikely.
2) Defer(block) new writers on until we complete or fail remount for example like follows. Do you like second solution ?
Quoted 9 lines
Umm... I wonder what the locking implications would be... Frankly, I suspect that what we really want is this:
* per-superblock write count of some kind, bumped when we decide
that writeback is inevitable and dropped when we are done with it (the same thing goes for async part of unlink(), etc.)
* fs_may_remount_ro() checking that write count
So basically we try to push those short-term writers to completion and if new ones had come while we'd been doing that (or some are really stuck) we fail remount with -EBUSY.

Quoted 4 lines

Perhaps we could utilise the filesystem freeze infrastructure - it already has hooks for intercepting new writers and modifcations, and filesystems have to flush any current modifications before the freeze completes. It sounds very similar to the requirements needed here...

There are filesystems (e.g. ext2 or UDF) which don't support freezing so it's not
an option at least short term...

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SuSE CR Labs

Dmitry Monakhov

author

2010-01-24 23:01 UTC #7 lore raw

In reply to Al Viro

Quoted 20 lines · Al Viro

Quoted 7 lines · Dmitry Monakhov
Quoted 2 lines
It's not a solution. You get an _attempted_ remount ro making writes fail, even if it's going to be unsuccessful. No go...
We have two options for new writers:
1) Fail it via -EROFS
Yes, remount may fail, but it is really unlikely.
2) Defer(block) new writers on until we complete or fail remount for example like follows. Do you like second solution ?
Umm... I wonder what the locking implications would be... Frankly, I suspect that what we really want is this:
* per-superblock write count of some kind, bumped when we decide
that writeback is inevitable and dropped when we are done with it (the same thing goes for async part of unlink(), etc.)
* fs_may_remount_ro() checking that write count
So basically we try to push those short-term writers to completion and if new ones had come while we'd been doing that (or some are really stuck) we fail remount with -EBUSY.

Quoted 4 lines

As a short-term solution the second patch would do probably (-stable and .33), but in the next cycle I'd rather see something addressing the real problem. fs_may_remount_ro() in its current form is really broken by design - it should not scan any lists (which is where your race comes from, BTW)

This is not actually true. The race happens not only because fs_may_remount_ro() is not atomic, but because we have two stages
1) fs_may_remount_ro()
2) sync_filesystem()
Even when we make first stage atomic, we still have race between second stage and new writers.
BTW: Your idea about per-sb counter may be useful here but it must be not reference count, but it may be used like i_version For example:
mnt_want_write()
{
mnt->mnt_sb->s_wr_count++;
}
mnt_drop_write()
{
mnt->mnt_sb->s_wr_count++;
}

do_remount_sb {
    cur = mnt->mnt_sb->s_wr_count;
    if (fs_may_remount_ro())
         return -EBUSY;
    sync_filesystem()
    if (cur != mnt->mnt_sb->s_wr_count)
         return -EBUSY;
}