linux-ext4 - Re: [PATCH] ext4: fix race between writepages and enabling EXT4_EXTENTS

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] [day] [month] [year] [list]

Date:   Tue, 18 Feb 2020 20:56:51 -0800
From:   Eric Biggers <ebiggers@...nel.org>
To:     Jan Kara <jack@...e.cz>
Cc:     linux-ext4@...r.kernel.org, Theodore Ts'o <tytso@....edu>
Subject: Re: [PATCH] ext4: fix race between writepages and enabling
 EXT4_EXTENTS_FL

On Tue, Feb 18, 2020 at 08:49:14AM +0100, Jan Kara wrote:
> On Mon 17-02-20 16:21:51, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@...gle.com>
> > 
> > If EXT4_EXTENTS_FL is set on an inode while ext4_writepages() is running
> > on it, the following warning in ext4_add_complete_io() can be hit:
> > 
> > WARNING: CPU: 1 PID: 0 at fs/ext4/page-io.c:234 ext4_put_io_end_defer+0xf0/0x120
> > 
> > Here's a minimal reproducer (not 100% reliable) (root isn't required):
> > 
> > 	while true; do
> > 		sync
> > 	done &
> > 	while true; do
> > 		rm -f file
> > 		touch file
> > 		chattr -e file
> > 		echo X >> file
> > 		chattr +e file
> > 	done
> > 
> > The problem is that in ext4_writepages(), ext4_should_dioread_nolock()
> > (which only returns true on extent-based files) is checked once to set
> > the number of reserved journal credits, and also again later to select
> > the flags for ext4_map_blocks() and copy the reserved journal handle to
> > ext4_io_end::handle.  But if EXT4_EXTENTS_FL is being concurrently set,
> > the first check can see dioread_nolock disabled while the later one can
> > see it enabled, causing the reserved handle to unexpectedly be NULL.
> > 
> > Fix this by checking ext4_should_dioread_nolock() only once and storing
> > the result in struct mpage_da_data.  This way, each ext4_writepages()
> > call uses a consistent dioread_nolock setting.
> > 
> > This was originally reported by syzbot without a reproducer at
> > https://syzkaller.appspot.com/bug?extid=2202a584a00fffd19fbf,
> > but now that dioread_nolock is the default I also started seeing this
> > when running syzkaller locally.
> > 
> > Reported-by: syzbot+2202a584a00fffd19fbf@...kaller.appspotmail.com
> > Fixes: 6b523df4fb5a ("ext4: use transaction reservation for extent conversion in ext4_end_io")
> > Cc: stable@...nel.org
> > Signed-off-by: Eric Biggers <ebiggers@...gle.com>
> 
> What you propose is probably enough to stop this particular race but I
> think there are other races that can get triggered by inode conversion
> to/from extent format. So I think we rather need to make inode
> format conversion much more careful (or we could just remove that
> functionality because I'm not sure if anybody actually uses it).
> 
> WRT making inode format conversion more careful you can have a look at how
> ext4_change_inode_journal_flag() works. I uses EXT4_I(inode)->i_mmap_sem to
> block page faults, it also uses sbi->s_journal_flag_rwsem to avoid races
> with writepages and I belive the migration code should do the same.

I was looking at that earlier, but I was a bit concerned that people could
complain about a performance regression due to EXTENTS_FL no longer being
settable/clearable on different files concurrently.

But if we think this functionality is rarely used and that no one would care,
then sure, we should go with that solution instead.  I'll probably rename
s_journal_flag_rwsem to s_writepages_rwsem since it will become for both the
EXTENTS and JOURNAL_DATA flags.

- Eric