* Re: [PATCH 1/4] ext4: Fix fsync error handling after filesysteb abort.
@ 2013-05-17 3:24 Yuan Fu
2013-05-17 7:10 ` Dmitry Monakhov
0 siblings, 1 reply; 4+ messages in thread
From: Yuan Fu @ 2013-05-17 3:24 UTC (permalink / raw)
To: dmonakhov, linux-ext4
Dear Dmitry Monakhov,
I see a race condition,
__ext4_abort()
...
EXT4_SB(sb)->s_mount_flags |= EXT4_MF_FS_ABORTED;
...
smp_wmb()
[if scheduled at this point ]
...
sb->s_flags |= MS_RDONLY;
if schedule occur above point(in red). There comes race condition
the s_mount_flags set to EXT4_MF_FS_ABORTED. On the other hand
sb->s_flags is not set to MS_RDONLY. Now if ext4_fsync_file() is
called from some process, the check s_flags to MS_RDONLY will fail,
and it will flush unwritten io and not return -EORFS.
thanks
----- Original Message -----
From: Dmitry Monakhov
Sent: 05/16/13 05:58 PM
Subject: [PATCH 1/4] ext4: Fix fsync error handling after filesysteb abort.
If filesystem was aborted after inode's write back complete
but before it's metadata was updated we may return success
due to (sb->s_flags & MS_RDONLY) which is incorrect and
result in data loss.
In order to handle fs abort correctly we have to check
fs state once we discover that it is in MS_RDONLY state
Test case: http://patchwork.ozlabs.org/patch/244297/
Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
fs/ext4/fsync.c | 8 ++++++--
fs/ext4/super.c | 13 ++++++++++++-
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
index e0ba8a4..d7df2f1 100644
--- a/fs/ext4/fsync.c
+++ b/fs/ext4/fsync.c
@@ -129,9 +129,13 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
return ret;
mutex_lock(&inode->i_mutex);
- if (inode->i_sb->s_flags & MS_RDONLY)
+ if (inode->i_sb->s_flags & MS_RDONLY) {
+ /* Make shure that we read updated s_mount_flags value */
+ smp_rmb();
+ if (EXT4_SB(inode->i_sb)->s_mount_flags & EXT4_MF_FS_ABORTED)
+ ret = -EROFS;
goto out;
-
+ }
ret = ext4_flush_unwritten_io(inode);
if (ret < 0)
goto out;
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index dbc7c09..6c91c8e 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -398,6 +398,11 @@ static void ext4_handle_error(struct super_block *sb)
}
if (test_opt(sb, ERRORS_RO)) {
ext4_msg(sb, KERN_CRIT, "Remounting filesystem read-only");
+ /*
+ * Make shure updated value of ->s_mount_flags will be visiable
+ * before ->s_flags update
+ */
+ smp_wmb();
sb->s_flags |= MS_RDONLY;
}
if (test_opt(sb, ERRORS_PANIC))
@@ -552,6 +557,7 @@ void __ext4_std_error(struct super_block *sb, const char *function,
*
* We unconditionally force the filesystem into an ABORT|READONLY state,
* unless the error response on the fs has been set to panic in which
+
* case we take the easy way out and panic immediately.
*/
@@ -570,8 +576,13 @@ void __ext4_abort(struct super_block *sb, const char *function,
if ((sb->s_flags & MS_RDONLY) == 0) {
ext4_msg(sb, KERN_CRIT, "Remounting filesystem read-only");
- sb->s_flags |= MS_RDONLY;
EXT4_SB(sb)->s_mount_flags |= EXT4_MF_FS_ABORTED;
+ /*
+ * Make shure updated value of ->s_mount_flags will be visiable
+ * before ->s_flags update
+ */
+ smp_wmb();
+ sb->s_flags |= MS_RDONLY;
if (EXT4_SB(sb)->s_journal)
jbd2_journal_abort(EXT4_SB(sb)->s_journal, -EIO);
save_error_info(sb, function, line);
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH 1/4] ext4: Fix fsync error handling after filesysteb abort.
2013-05-17 3:24 [PATCH 1/4] ext4: Fix fsync error handling after filesysteb abort Yuan Fu
@ 2013-05-17 7:10 ` Dmitry Monakhov
0 siblings, 0 replies; 4+ messages in thread
From: Dmitry Monakhov @ 2013-05-17 7:10 UTC (permalink / raw)
To: Yuan Fu, linux-ext4
On Fri, 17 May 2013 05:24:15 +0200, "Yuan Fu" <yuan.fu@gmx.cn> wrote:
> Dear Dmitry Monakhov,
>
> I see a race condition,
> __ext4_abort()
> ...
> EXT4_SB(sb)->s_mount_flags |= EXT4_MF_FS_ABORTED;
> ...
> smp_wmb()
>
> [if scheduled at this point ]
> ...
> sb->s_flags |= MS_RDONLY;
>
>
> if schedule occur above point(in red). There comes race condition
> the s_mount_flags set to EXT4_MF_FS_ABORTED. On the other hand
> sb->s_flags is not set to MS_RDONLY. Now if ext4_fsync_file() is
> called from some process, the check s_flags to MS_RDONLY will fail,
> and it will flush unwritten io and not return -EORFS.
If flush_unwritten_io() was called after fs was aborted it will return
appropriate code (EROFS or EIO) so fsync(2) will fail as expected.
>
> thanks
>
> ----- Original Message -----
> From: Dmitry Monakhov
> Sent: 05/16/13 05:58 PM
> Subject: [PATCH 1/4] ext4: Fix fsync error handling after filesysteb abort.
> If filesystem was aborted after inode's write back complete
> but before it's metadata was updated we may return success
> due to (sb->s_flags & MS_RDONLY) which is incorrect and
> result in data loss.
> In order to handle fs abort correctly we have to check
> fs state once we discover that it is in MS_RDONLY state
>
> Test case: http://patchwork.ozlabs.org/patch/244297/
>
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
> fs/ext4/fsync.c | 8 ++++++--
> fs/ext4/super.c | 13 ++++++++++++-
> 2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
> index e0ba8a4..d7df2f1 100644
> --- a/fs/ext4/fsync.c
> +++ b/fs/ext4/fsync.c
> @@ -129,9 +129,13 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
> return ret;
> mutex_lock(&inode->i_mutex);
>
> - if (inode->i_sb->s_flags & MS_RDONLY)
> + if (inode->i_sb->s_flags & MS_RDONLY) {
> + /* Make shure that we read updated s_mount_flags value */
> + smp_rmb();
> + if (EXT4_SB(inode->i_sb)->s_mount_flags & EXT4_MF_FS_ABORTED)
> + ret = -EROFS;
> goto out;
> -
> + }
> ret = ext4_flush_unwritten_io(inode);
> if (ret < 0)
> goto out;
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index dbc7c09..6c91c8e 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -398,6 +398,11 @@ static void ext4_handle_error(struct super_block *sb)
> }
> if (test_opt(sb, ERRORS_RO)) {
> ext4_msg(sb, KERN_CRIT, "Remounting filesystem read-only");
> + /*
> + * Make shure updated value of ->s_mount_flags will be visiable
> + * before ->s_flags update
> + */
> + smp_wmb();
> sb->s_flags |= MS_RDONLY;
> }
> if (test_opt(sb, ERRORS_PANIC))
> @@ -552,6 +557,7 @@ void __ext4_std_error(struct super_block *sb, const char *function,
> *
> * We unconditionally force the filesystem into an ABORT|READONLY state,
> * unless the error response on the fs has been set to panic in which
> +
> * case we take the easy way out and panic immediately.
> */
>
> @@ -570,8 +576,13 @@ void __ext4_abort(struct super_block *sb, const char *function,
>
> if ((sb->s_flags & MS_RDONLY) == 0) {
> ext4_msg(sb, KERN_CRIT, "Remounting filesystem read-only");
> - sb->s_flags |= MS_RDONLY;
> EXT4_SB(sb)->s_mount_flags |= EXT4_MF_FS_ABORTED;
> + /*
> + * Make shure updated value of ->s_mount_flags will be visiable
> + * before ->s_flags update
> + */
> + smp_wmb();
> + sb->s_flags |= MS_RDONLY;
> if (EXT4_SB(sb)->s_journal)
> jbd2_journal_abort(EXT4_SB(sb)->s_journal, -EIO);
> save_error_info(sb, function, line);
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/4] ext4: Fix fsync error handling after filesysteb abort.
@ 2013-05-16 12:28 Dmitry Monakhov
2013-05-21 16:18 ` Jan Kara
0 siblings, 1 reply; 4+ messages in thread
From: Dmitry Monakhov @ 2013-05-16 12:28 UTC (permalink / raw)
Cc: linux-ext4, Dmitry Monakhov
If filesystem was aborted after inode's write back complete
but before it's metadata was updated we may return success
due to (sb->s_flags & MS_RDONLY) which is incorrect and
result in data loss.
In order to handle fs abort correctly we have to check
fs state once we discover that it is in MS_RDONLY state
Test case: http://patchwork.ozlabs.org/patch/244297/
Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
fs/ext4/fsync.c | 8 ++++++--
fs/ext4/super.c | 13 ++++++++++++-
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
index e0ba8a4..d7df2f1 100644
--- a/fs/ext4/fsync.c
+++ b/fs/ext4/fsync.c
@@ -129,9 +129,13 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
return ret;
mutex_lock(&inode->i_mutex);
- if (inode->i_sb->s_flags & MS_RDONLY)
+ if (inode->i_sb->s_flags & MS_RDONLY) {
+ /* Make shure that we read updated s_mount_flags value */
+ smp_rmb();
+ if (EXT4_SB(inode->i_sb)->s_mount_flags & EXT4_MF_FS_ABORTED)
+ ret = -EROFS;
goto out;
-
+ }
ret = ext4_flush_unwritten_io(inode);
if (ret < 0)
goto out;
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index dbc7c09..6c91c8e 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -398,6 +398,11 @@ static void ext4_handle_error(struct super_block *sb)
}
if (test_opt(sb, ERRORS_RO)) {
ext4_msg(sb, KERN_CRIT, "Remounting filesystem read-only");
+ /*
+ * Make shure updated value of ->s_mount_flags will be visiable
+ * before ->s_flags update
+ */
+ smp_wmb();
sb->s_flags |= MS_RDONLY;
}
if (test_opt(sb, ERRORS_PANIC))
@@ -552,6 +557,7 @@ void __ext4_std_error(struct super_block *sb, const char *function,
*
* We unconditionally force the filesystem into an ABORT|READONLY state,
* unless the error response on the fs has been set to panic in which
+
* case we take the easy way out and panic immediately.
*/
@@ -570,8 +576,13 @@ void __ext4_abort(struct super_block *sb, const char *function,
if ((sb->s_flags & MS_RDONLY) == 0) {
ext4_msg(sb, KERN_CRIT, "Remounting filesystem read-only");
- sb->s_flags |= MS_RDONLY;
EXT4_SB(sb)->s_mount_flags |= EXT4_MF_FS_ABORTED;
+ /*
+ * Make shure updated value of ->s_mount_flags will be visiable
+ * before ->s_flags update
+ */
+ smp_wmb();
+ sb->s_flags |= MS_RDONLY;
if (EXT4_SB(sb)->s_journal)
jbd2_journal_abort(EXT4_SB(sb)->s_journal, -EIO);
save_error_info(sb, function, line);
--
1.7.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH 1/4] ext4: Fix fsync error handling after filesysteb abort.
2013-05-16 12:28 Dmitry Monakhov
@ 2013-05-21 16:18 ` Jan Kara
0 siblings, 0 replies; 4+ messages in thread
From: Jan Kara @ 2013-05-21 16:18 UTC (permalink / raw)
To: Dmitry Monakhov; +Cc: linux-ext4
On Thu 16-05-13 16:28:07, Dmitry Monakhov wrote:
The patch looks OK but I have some typo fixes below. But you can add
Reviewed-by: Jan Kara <jack@suse.cz>
> If filesystem was aborted after inode's write back complete
^^ is
> but before it's metadata was updated we may return success
^^^ its
> due to (sb->s_flags & MS_RDONLY) which is incorrect and
> result in data loss.
^^^ results
> In order to handle fs abort correctly we have to check
> fs state once we discover that it is in MS_RDONLY state
>
> Test case: http://patchwork.ozlabs.org/patch/244297/
>
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
> fs/ext4/fsync.c | 8 ++++++--
> fs/ext4/super.c | 13 ++++++++++++-
> 2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
> index e0ba8a4..d7df2f1 100644
> --- a/fs/ext4/fsync.c
> +++ b/fs/ext4/fsync.c
> @@ -129,9 +129,13 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
> return ret;
> mutex_lock(&inode->i_mutex);
>
> - if (inode->i_sb->s_flags & MS_RDONLY)
> + if (inode->i_sb->s_flags & MS_RDONLY) {
> + /* Make shure that we read updated s_mount_flags value */
^^^ sure
> + smp_rmb();
> + if (EXT4_SB(inode->i_sb)->s_mount_flags & EXT4_MF_FS_ABORTED)
> + ret = -EROFS;
> goto out;
> -
> + }
> ret = ext4_flush_unwritten_io(inode);
> if (ret < 0)
> goto out;
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index dbc7c09..6c91c8e 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -398,6 +398,11 @@ static void ext4_handle_error(struct super_block *sb)
> }
> if (test_opt(sb, ERRORS_RO)) {
> ext4_msg(sb, KERN_CRIT, "Remounting filesystem read-only");
> + /*
> + * Make shure updated value of ->s_mount_flags will be visiable
^^^ sure visible ^^
> + * before ->s_flags update
> + */
> + smp_wmb();
> sb->s_flags |= MS_RDONLY;
> }
> if (test_opt(sb, ERRORS_PANIC))
> @@ -552,6 +557,7 @@ void __ext4_std_error(struct super_block *sb, const char *function,
> *
> * We unconditionally force the filesystem into an ABORT|READONLY state,
> * unless the error response on the fs has been set to panic in which
> +
^^ Stray empty line.
> * case we take the easy way out and panic immediately.
> */
>
> @@ -570,8 +576,13 @@ void __ext4_abort(struct super_block *sb, const char *function,
>
> if ((sb->s_flags & MS_RDONLY) == 0) {
> ext4_msg(sb, KERN_CRIT, "Remounting filesystem read-only");
> - sb->s_flags |= MS_RDONLY;
> EXT4_SB(sb)->s_mount_flags |= EXT4_MF_FS_ABORTED;
> + /*
> + * Make shure updated value of ->s_mount_flags will be visiable
^^ sure visible ^^
> + * before ->s_flags update
> + */
> + smp_wmb();
> + sb->s_flags |= MS_RDONLY;
> if (EXT4_SB(sb)->s_journal)
> jbd2_journal_abort(EXT4_SB(sb)->s_journal, -EIO);
> save_error_info(sb, function, line);
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-05-21 16:18 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-17 3:24 [PATCH 1/4] ext4: Fix fsync error handling after filesysteb abort Yuan Fu
2013-05-17 7:10 ` Dmitry Monakhov
-- strict thread matches above, loose matches on Subject: below --
2013-05-16 12:28 Dmitry Monakhov
2013-05-21 16:18 ` Jan Kara
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.