* [PATCH RFC] jbd2: detect old record when do journal scan
@ 2022-08-10 1:34 Ye Bin
2022-08-19 8:00 ` yebin
2022-08-19 9:54 ` Jan Kara
0 siblings, 2 replies; 6+ messages in thread
From: Ye Bin @ 2022-08-10 1:34 UTC (permalink / raw)
To: tytso, adilger.kernel, linux-ext4; +Cc: linux-kernel, jack, Ye Bin
As https://github.com/tytso/e2fsprogs/issues/120 describe tune2fs do not update
j_tail_sequence when do journal recovery. This maybe recover old journal record,
then will lead to file system corruption.
To avoid file system corruption in this case, if detect current transaction's
commit time earlier than previous transaction's commit time when do journal
scan, just return error.
Signed-off-by: Ye Bin <yebin10@huawei.com>
---
fs/jbd2/recovery.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
index f548479615c6..f3def21a96a5 100644
--- a/fs/jbd2/recovery.c
+++ b/fs/jbd2/recovery.c
@@ -812,8 +812,17 @@ static int do_one_pass(journal_t *journal,
break;
}
}
- if (pass == PASS_SCAN)
+ if (pass == PASS_SCAN) {
+ if (commit_time < last_trans_commit_time) {
+ pr_err("JBD2: old journal record found "
+ "in transaction %u\n",
+ next_commit_ID);
+ err = -EFSBADCRC;
+ brelse(bh);
+ goto failed;
+ }
last_trans_commit_time = commit_time;
+ }
brelse(bh);
next_commit_ID++;
continue;
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH RFC] jbd2: detect old record when do journal scan
2022-08-10 1:34 [PATCH RFC] jbd2: detect old record when do journal scan Ye Bin
@ 2022-08-19 8:00 ` yebin
2022-08-19 8:34 ` fengnan chang
2022-08-19 9:54 ` Jan Kara
1 sibling, 1 reply; 6+ messages in thread
From: yebin @ 2022-08-19 8:00 UTC (permalink / raw)
To: tytso, adilger.kernel, linux-ext4; +Cc: linux-kernel, jack
ping...
On 2022/8/10 9:34, Ye Bin wrote:
> As https://github.com/tytso/e2fsprogs/issues/120 describe tune2fs do not update
> j_tail_sequence when do journal recovery. This maybe recover old journal record,
> then will lead to file system corruption.
> To avoid file system corruption in this case, if detect current transaction's
> commit time earlier than previous transaction's commit time when do journal
> scan, just return error.
>
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
> fs/jbd2/recovery.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> index f548479615c6..f3def21a96a5 100644
> --- a/fs/jbd2/recovery.c
> +++ b/fs/jbd2/recovery.c
> @@ -812,8 +812,17 @@ static int do_one_pass(journal_t *journal,
> break;
> }
> }
> - if (pass == PASS_SCAN)
> + if (pass == PASS_SCAN) {
> + if (commit_time < last_trans_commit_time) {
> + pr_err("JBD2: old journal record found "
> + "in transaction %u\n",
> + next_commit_ID);
> + err = -EFSBADCRC;
> + brelse(bh);
> + goto failed;
> + }
> last_trans_commit_time = commit_time;
> + }
> brelse(bh);
> next_commit_ID++;
> continue;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RFC] jbd2: detect old record when do journal scan
2022-08-19 8:00 ` yebin
@ 2022-08-19 8:34 ` fengnan chang
0 siblings, 0 replies; 6+ messages in thread
From: fengnan chang @ 2022-08-19 8:34 UTC (permalink / raw)
To: yebin; +Cc: tytso@mit.edu, adilger.kernel, linux-ext4, linux-kernel, jack
If there has system time calibration, system time has become smaller, what will be happen?
the journal maybe thought as corrupt?
> 2022年8月19日 16:00，yebin <yebin10@huawei.com> 写道：
>
> ping...
>
> On 2022/8/10 9:34, Ye Bin wrote:
>> As https://github.com/tytso/e2fsprogs/issues/120 describe tune2fs do not update
>> j_tail_sequence when do journal recovery. This maybe recover old journal record,
>> then will lead to file system corruption.
>> To avoid file system corruption in this case, if detect current transaction's
>> commit time earlier than previous transaction's commit time when do journal
>> scan, just return error.
>>
>> Signed-off-by: Ye Bin <yebin10@huawei.com>
>> ---
>> fs/jbd2/recovery.c | 11 ++++++++++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
>> index f548479615c6..f3def21a96a5 100644
>> --- a/fs/jbd2/recovery.c
>> +++ b/fs/jbd2/recovery.c
>> @@ -812,8 +812,17 @@ static int do_one_pass(journal_t *journal,
>> break;
>> }
>> }
>> - if (pass == PASS_SCAN)
>> + if (pass == PASS_SCAN) {
>> + if (commit_time < last_trans_commit_time) {
>> + pr_err("JBD2: old journal record found "
>> + "in transaction %u\n",
>> + next_commit_ID);
>> + err = -EFSBADCRC;
>> + brelse(bh);
>> + goto failed;
>> + }
>> last_trans_commit_time = commit_time;
>> + }
>> brelse(bh);
>> next_commit_ID++;
>> continue;
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] jbd2: detect old record when do journal scan
2022-08-10 1:34 [PATCH RFC] jbd2: detect old record when do journal scan Ye Bin
2022-08-19 8:00 ` yebin
@ 2022-08-19 9:54 ` Jan Kara
2022-08-23 9:17 ` yebin
1 sibling, 1 reply; 6+ messages in thread
From: Jan Kara @ 2022-08-19 9:54 UTC (permalink / raw)
To: Ye Bin; +Cc: tytso, adilger.kernel, linux-ext4, linux-kernel, jack
On Wed 10-08-22 09:34:42, Ye Bin wrote:
> As https://github.com/tytso/e2fsprogs/issues/120 describe tune2fs do not update
> j_tail_sequence when do journal recovery. This maybe recover old journal record,
> then will lead to file system corruption.
> To avoid file system corruption in this case, if detect current transaction's
> commit time earlier than previous transaction's commit time when do journal
> scan, just return error.
>
> Signed-off-by: Ye Bin <yebin10@huawei.com>
Thanks for the patch! Let me see if I understand your concern right. You
are concerned about the following scenario:
1) Kernel uses the filesystem, there's a crash.
2) E2fsprogs replays the journal but fails to update sb->s_sequence in the
journal superblock.
3) Kernel mounts the fs again - however note that even if kernel skips
recovery, it does scan the journal jbd2_journal_skip_recovery() and
journal->j_transaction_sequence is set based on the last transaction found
in the journal.
So I don't think there is really possibility we will quickly reuse some
transaction IDs and thus possibility of corruption on replay? Am I missing
something?
Honza
> ---
> fs/jbd2/recovery.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> index f548479615c6..f3def21a96a5 100644
> --- a/fs/jbd2/recovery.c
> +++ b/fs/jbd2/recovery.c
> @@ -812,8 +812,17 @@ static int do_one_pass(journal_t *journal,
> break;
> }
> }
> - if (pass == PASS_SCAN)
> + if (pass == PASS_SCAN) {
> + if (commit_time < last_trans_commit_time) {
> + pr_err("JBD2: old journal record found "
> + "in transaction %u\n",
> + next_commit_ID);
> + err = -EFSBADCRC;
> + brelse(bh);
> + goto failed;
> + }
> last_trans_commit_time = commit_time;
> + }
> brelse(bh);
> next_commit_ID++;
> continue;
> --
> 2.31.1
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RFC] jbd2: detect old record when do journal scan
2022-08-19 9:54 ` Jan Kara
@ 2022-08-23 9:17 ` yebin
2022-08-23 13:07 ` Jan Kara
0 siblings, 1 reply; 6+ messages in thread
From: yebin @ 2022-08-23 9:17 UTC (permalink / raw)
To: Jan Kara; +Cc: tytso, adilger.kernel, linux-ext4, linux-kernel
On 2022/8/19 17:54, Jan Kara wrote:
> On Wed 10-08-22 09:34:42, Ye Bin wrote:
>> As https://github.com/tytso/e2fsprogs/issues/120 describe tune2fs do not update
>> j_tail_sequence when do journal recovery. This maybe recover old journal record,
>> then will lead to file system corruption.
>> To avoid file system corruption in this case, if detect current transaction's
>> commit time earlier than previous transaction's commit time when do journal
>> scan, just return error.
>>
>> Signed-off-by: Ye Bin <yebin10@huawei.com>
> Thanks for the patch! Let me see if I understand your concern right. You
> are concerned about the following scenario:
>
> 1) Kernel uses the filesystem, there's a crash.
> 2) E2fsprogs replays the journal but fails to update sb->s_sequence in the
> journal superblock.
> 3) Kernel mounts the fs again - however note that even if kernel skips
> recovery, it does scan the journal jbd2_journal_skip_recovery() and
> journal->j_transaction_sequence is set based on the last transaction found
> in the journal.
>
> So I don't think there is really possibility we will quickly reuse some
> transaction IDs and thus possibility of corruption on replay? Am I missing
> something?
>
> Honza
The file system corruption I encountered was indeed because e2fsprogs
did not update
journal - > J_ transaction_ Sequence leads to replay the old transaction.
So I wonder whether the kernel should detect this kind of exception, at
least when there
is a file system corruption, there are clues to trace.
>
>> ---
>> fs/jbd2/recovery.c | 11 ++++++++++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
>> index f548479615c6..f3def21a96a5 100644
>> --- a/fs/jbd2/recovery.c
>> +++ b/fs/jbd2/recovery.c
>> @@ -812,8 +812,17 @@ static int do_one_pass(journal_t *journal,
>> break;
>> }
>> }
>> - if (pass == PASS_SCAN)
>> + if (pass == PASS_SCAN) {
>> + if (commit_time < last_trans_commit_time) {
>> + pr_err("JBD2: old journal record found "
>> + "in transaction %u\n",
>> + next_commit_ID);
>> + err = -EFSBADCRC;
>> + brelse(bh);
>> + goto failed;
>> + }
>> last_trans_commit_time = commit_time;
>> + }
>> brelse(bh);
>> next_commit_ID++;
>> continue;
>> --
>> 2.31.1
>>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RFC] jbd2: detect old record when do journal scan
2022-08-23 9:17 ` yebin
@ 2022-08-23 13:07 ` Jan Kara
0 siblings, 0 replies; 6+ messages in thread
From: Jan Kara @ 2022-08-23 13:07 UTC (permalink / raw)
To: yebin; +Cc: Jan Kara, tytso, adilger.kernel, linux-ext4, linux-kernel
On Tue 23-08-22 17:17:59, yebin wrote:
> On 2022/8/19 17:54, Jan Kara wrote:
> > On Wed 10-08-22 09:34:42, Ye Bin wrote:
> > > As https://github.com/tytso/e2fsprogs/issues/120 describe tune2fs do not update
> > > j_tail_sequence when do journal recovery. This maybe recover old journal record,
> > > then will lead to file system corruption.
> > > To avoid file system corruption in this case, if detect current transaction's
> > > commit time earlier than previous transaction's commit time when do journal
> > > scan, just return error.
> > >
> > > Signed-off-by: Ye Bin <yebin10@huawei.com>
> > Thanks for the patch! Let me see if I understand your concern right. You
> > are concerned about the following scenario:
> >
> > 1) Kernel uses the filesystem, there's a crash.
> > 2) E2fsprogs replays the journal but fails to update sb->s_sequence in the
> > journal superblock.
> > 3) Kernel mounts the fs again - however note that even if kernel skips
> > recovery, it does scan the journal jbd2_journal_skip_recovery() and
> > journal->j_transaction_sequence is set based on the last transaction found
> > in the journal.
> >
> > So I don't think there is really possibility we will quickly reuse some
> > transaction IDs and thus possibility of corruption on replay? Am I missing
> > something?
> >
> The file system corruption I encountered was indeed because e2fsprogs did
> not update
> journal - > J_ transaction_ Sequence leads to replay the old transaction.
> So I wonder whether the kernel should detect this kind of exception, at
> least when there
> is a file system corruption, there are clues to trace.
OK, but what is not quite clear to me is why the kernel started to replay
the journal in the first place. Didn't e2fsprogs mark the filesystem as
clean after replaying the journal for some reason?
I'm asking because checking transaction validity based on wall clock time
has its own issues as well (clock gets skewed, updated via ntpd, BIOS can
reset it to some random value etc.) so adding the check you propose can
also create new issues...
Honza
> > > diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> > > index f548479615c6..f3def21a96a5 100644
> > > --- a/fs/jbd2/recovery.c
> > > +++ b/fs/jbd2/recovery.c
> > > @@ -812,8 +812,17 @@ static int do_one_pass(journal_t *journal,
> > > break;
> > > }
> > > }
> > > - if (pass == PASS_SCAN)
> > > + if (pass == PASS_SCAN) {
> > > + if (commit_time < last_trans_commit_time) {
> > > + pr_err("JBD2: old journal record found "
> > > + "in transaction %u\n",
> > > + next_commit_ID);
> > > + err = -EFSBADCRC;
> > > + brelse(bh);
> > > + goto failed;
> > > + }
> > > last_trans_commit_time = commit_time;
> > > + }
> > > brelse(bh);
> > > next_commit_ID++;
> > > continue;
> > > --
> > > 2.31.1
> > >
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-08-23 16:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-10 1:34 [PATCH RFC] jbd2: detect old record when do journal scan Ye Bin
2022-08-19 8:00 ` yebin
2022-08-19 8:34 ` fengnan chang
2022-08-19 9:54 ` Jan Kara
2022-08-23 9:17 ` yebin
2022-08-23 13:07 ` Jan Kara
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox