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

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

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.

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR