Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 23 Aug 2022 15:07:32 +0200
From:   Jan Kara <jack@suse.cz>
To:     yebin <yebin10@huawei.com>
Cc:     Jan Kara <jack@suse.cz>, tytso@mit.edu, adilger.kernel@dilger.ca,
        linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFC] jbd2: detect old record when do journal scan
Message-ID: <20220823130732.73ifn3qwvnj7xh4b@quack3>
References: <20220810013442.3474533-1-yebin10@huawei.com>
 <20220819095445.yq4d2qhrhb73p3zk@quack3>
 <63049B47.2000408@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <63049B47.2000408@huawei.com>
Precedence: bulk

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