linux-kernel - Re: [PATCH -next] ext4: fix warning in ext4_handle_inode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YnvDjHFAeZWEDne1@mit.edu>
Date:   Wed, 11 May 2022 10:09:16 -0400
From:   "Theodore Ts'o" <tytso@....edu>
To:     yebin <yebin10@...wei.com>
Cc:     Jan Kara <jack@...e.cz>, adilger.kernel@...ger.ca,
        linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org,
        lczerner@...hat.com
Subject: Re: [PATCH -next] ext4: fix warning in ext4_handle_inode_extension

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