On Mon, May 26, 2025 at 03:43:31PM +0200, Jan Kara wrote:
> On Wed 21-05-25 18:52:03, Thadeu Lima de Souza Cascardo wrote:
> > On Tue, May 20, 2025 at 10:57:08AM -0400, Theodore Ts'o wrote:
> > > On Mon, May 19, 2025 at 07:42:46AM -0300, Thadeu Lima de Souza Cascardo wrote:
> > > > inline data handling has a race between writing and writing to a memory
> > > > map.
> > > >
> > > > When ext4_page_mkwrite is called, it calls ext4_convert_inline_data, which
> > > > destroys the inline data, but if block allocation fails, restores the
> > > > inline data. In that process, we could have:
> > > >
> > > > CPU1 CPU2
> > > > destroy_inline_data
> > > > write_begin (does not see inline data)
> > > > restory_inline_data
> > > > write_end (sees inline data)
> > > >
> > > > The conversion inside ext4_page_mkwrite was introduced at commit
> > > > 7b4cc9787fe3 ("ext4: evict inline data when writing to memory map"). This
> > > > fixes a documented bug in the commit message, which suggests some
> > > > alternatives fixes.
> > >
> > > Your fix just reverts commit 7b4cc9787fe3, and removes the BUG_ON.
> > > While this is great for shutting up the syzbot report, but it causes
> > > file writes to an inline data file via a mmap to never get written
> > > back to the storage device. So you are replacing BUG_ON that can get
> > > triggered on a race condition in case of a failed block allocation,
> > > with silent data corruption. This is not an improvement.
> > >
> > > Thanks for trying to address this, but I'm not going to accept your
> > > proposed fix.
> > >
> > > - Ted
> >
> > Hi, Ted.
> >
> > I am trying to understand better the circumstances where the data loss
> > might occur with the fix, but might not occur without the fix. Or, even if
> > they occur either way, such that I can work on a better/proper fix.
> >
> > Right now, if ext4_convert_inline_data (called from ext4_page_mkwrite)
> > fails with ENOSPC, the memory access will lead to a SIGBUS. The same will
> > happen without the fix, if there are no blocks available.
> >
> > Now, without ext4_convert_inline_data, blocks will be allocated by
> > ext4_page_mkwrite and written by ext4_do_writepages. Are you concerned
> > about a failure between the clearing of the inode data and the writing of
> > the block in ext4_do_writepages?
> >
> > Or are you concerned about a potential race condition when allocating
> > blocks?
> >
> > Which of these cannot happen today with the code as is? If I understand
> > correctly, the inline conversion code also calls ext4_destroy_inline_data
> > before allocating and writing to blocks.
> >
> > Thanks a lot for the review and guidance.
>
> So I'm not sure what Ted was exactly worried about because writeback code
> should normally allocate underlying blocks for writeout of the mmaped page
> AFAICT. But the problem I can see is that clearing
> EXT4_STATE_MAY_INLINE_DATA requires i_rwsem held as otherwise we may be
> racing with e.g. write(2) and switching EXT4_STATE_MAY_INLINE_DATA in the
> middle of the write will cause bad things (inconsistency between how
> write_begin() and write_end() callbacks behave).
>
> Honza
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR
Thanks, Jan.
I later noticed as well that writepages is not holding the inode lock
either, so there would be a potential for race condition there as well.
I have sent a v2 that I find would not have this problem. But we should
probably cleanup the handling of inline data in writepages as a followup.
Cascardo.
