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:   Wed, 11 May 2022 10:09:16 -0400
From:   "Theodore Ts'o" <tytso@mit.edu>
To:     yebin <yebin10@huawei.com>
Cc:     Jan Kara <jack@suse.cz>, adilger.kernel@dilger.ca,
        linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org,
        lczerner@redhat.com
Subject: Re: [PATCH -next] ext4: fix warning in ext4_handle_inode_extension
Message-ID: <YnvDjHFAeZWEDne1@mit.edu>
References: <20220326065351.761952-1-yebin10@huawei.com>
 <20220329092810.j5ngxckygut6mxo2@quack3.lan>
 <6244482D.4090603@huawei.com>
 <20220330133015.yxfnnw564wgehjc3@quack3.lan>
 <62578B0C.9000803@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <62578B0C.9000803@huawei.com>
Precedence: bulk

On Thu, Apr 14, 2022 at 10:46:36AM +0800, yebin wrote:
> To be honest, I don't know syzkaller how to inject the NOMEM
> fault. If syzkaller rely on the memory fault injection mode provided
> by the kernel, should report null pointer access. Anyway, If inject
> a single point of IO fault, we still have to face the same
> situation.

Was this patch in response to a syzkaller report?  There wasn't a
Reported-by tag indicating that this came from syzkaller.  If it did,
and it came from syzkaller run by the syzkaller team (e.g., at
https://syzkaller.appspot.com/upstream) could you include a reference
to the syzkaller report?

> On 2022/3/30 21:30, Jan Kara wrote:
> > > Do you mean call jbd2_abort in ext4_reserve_inode_write() ?
> > Yes.
> > 
> > > If we abort journal when metadata is not guaranteed to be consistent. The
> > > mode of ‘errors=continue’ is unnecessary.
> > Well, firstly, errors=continue was always the best effort. There are no
> > guarantees which failures we are able to withstand and which not.

That's true; however, in general, if we can back out and recover from
the error, we should, so that errors=continue can work.  If we think
that continuing will result in far more file system corruption and/or
the error is from the journalling infrastructure itself, then aborting
the journal makes sense.

> > There are
> > almost 80 callsites of ext4_mark_inode_dirty() and honestly I suspect that
> > e.g. inconsistent states resulting from extent tree manipulations being
> > aborted in the middle due to ext4_ext_dirty() failing due to ENOMEM will
> > also trigger all sorts of "interesting" behavior. So that's why I'd rather
> > abort the journal than try to continue when we almost certainly now we
> > cannot.

It would not be a bad thing for us to audit all of the callers of
ext4_mark_inode_dirty() and ext4_reserve_inode_write().  We are
getting things right in at least some of the callers (for example:
ext4_mkdir).

In any case, I'll take this patch, but if this was in response to a
syzkaller report, please let me know with the syzkaller ID is, so I
can update the commit before I send a pull request to Linus.

Thanks!

							- Ted