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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]> 写道：
>
> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
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 <[email protected]>
SUSE Labs, CR
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 <[email protected]>
> 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 <[email protected]>
> > 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 <[email protected]>
SUSE Labs, CR