| Message ID | 20220810013442.3474533-1-yebin10@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] jbd2: detect old record when do journal scan | expand |
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;
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; >
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 >
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 >>
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 > > > >
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;
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(-)