Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 23 Jan 2021 16:57:23 +0800
From:   Murphy Zhou <jencce.kernel@gmail.com>
To:     Jan Kara <jack@suse.cz>
Cc:     Theodore Ts'o <tytso@mit.edu>,
        Murphy Zhou <jencce.kernel@gmail.com>,
        linux-ext4@vger.kernel.org
Subject: Re: ext4 regression panic
Message-ID: <20210123085723.zobwxf6aik6jgfbn@xzhoux.usersys.redhat.com>
References: <20210121101547.fwh35hov3hshogbz@xzhoux.usersys.redhat.com>
 <YAm8qH/0oo2ofSMR@mit.edu>
 <20210121210949.GH24063@quack2.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210121210949.GH24063@quack2.suse.cz>
Precedence: bulk

On Thu, Jan 21, 2021 at 10:09:49PM +0100, Jan Kara wrote:
> On Thu 21-01-21 12:40:56, Theodore Ts'o wrote:
> > On Thu, Jan 21, 2021 at 06:15:47PM +0800, Murphy Zhou wrote:
> > > Hi Jack,
> > > 
> > > A panic was introduced by this commit. It's easy and reliable to
> > > reproduce.
> > > 
> > > commit 2d01ddc86606564fb08c56e3bc93a0693895f710
> > > Author: Jan Kara <jack@suse.cz>
> > > Date:   Wed Dec 16 11:18:40 2020 +0100
> > > 
> > >     ext4: save error info to sb through journal if available
> > 
> > Hi Murphy,
> > 
> > Thanks for the bug report.  What's happening is that we haven't yet
> > initialized mballoc yet --- that happens in line 4943 of
> > fs/ext4/super.c, in ext4_fill_super().
> > 
> > But in line 4903 (in the case of the BZ #199275 reproducer), we
> > attempt to fetch the root inode, which is fails because it is
> > unallocated.  That then triggers a call to ext4_error(), which now
> > results in a journalled change, since the journal is initialized
> > starting in line 4793, and in line 4838, we set up the
> > j_commit_callback, which is what ends up calling
> > ext4_process_freed_data(), but since the multiblock allocator hasn't
> > been set up yet, that causes the NULL pointer dereference.
> > 
> > So what we need to do is to *not* set up the callback until after the
> > call to ext4_mb_init().
> > 
> > We should probably create an ext4-specific test in xfstests which
> > tries mounting a small, deliberately corrupted file system, to make
> > sure we handle this case correctly in the future.
> > 
> > 						- Ted
> 
> Thanks for looking into this. You beat me to my fix (which was slightly
> different - I moved ext4_mb_init() somewhat earlier during mount). But this
> should work fine as well. So feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> 								Honza

Hi Jack and Ted,

This patch fixed it. Thanks for the quick fix!

Murphy

> 
> > 
> > commit 6c2f9a8247273cf1108ff71c99680b7457f48318
> > Author: Theodore Ts'o <tytso@mit.edu>
> > Date:   Thu Jan 21 12:33:20 2021 -0500
> > 
> >     ext4: don't try to processed freed blocks until mballoc is initialized
> >     
> >     If we try to make any changes via the journal between when the journal
> >     is initialized, but before the multi-block allocated is initialized,
> >     we will end up deferencing a NULL pointer when the journal commit
> >     callback function calls ext4_process_freed_data().
> >     
> >     The proximate cause of this failure was commit 2d01ddc86606 ("ext4:
> >     save error info to sb through journal if available") since file system
> >     corruption problems detected before the call to ext4_mb_init() would
> >     result in a journal commit before we aborted the mount of the file
> >     system.... and we would then trigger the NULL pointer deref.
> >     
> >     Cc: Jan Kara <jack@suse.cz>
> >     Reported by: Murphy Zhou <jencce.kernel@gmail.com>
> >     Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > 
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index 0f0db49031dc..802ef55f0a55 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -4876,7 +4876,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> >  
> >  	set_task_ioprio(sbi->s_journal->j_task, journal_ioprio);
> >  
> > -	sbi->s_journal->j_commit_callback = ext4_journal_commit_callback;
> >  	sbi->s_journal->j_submit_inode_data_buffers =
> >  		ext4_journal_submit_inode_data_buffers;
> >  	sbi->s_journal->j_finish_inode_data_buffers =
> > @@ -4993,6 +4992,14 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> >  		goto failed_mount5;
> >  	}
> >  
> > +	/*
> > +	 * We can only set up the journal commit callback once
> > +	 * mballoc is initialized
> > +	 */
> > +	if (sbi->s_journal)
> > +		sbi->s_journal->j_commit_callback =
> > +			ext4_journal_commit_callback;
> > +
> >  	block = ext4_count_free_clusters(sb);
> >  	ext4_free_blocks_count_set(sbi->s_es, 
> >  				   EXT4_C2B(sbi, block));
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

-- 
Murphy