| Message ID | 20240410142948.2817554-3-yi.zhang@huaweicloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ext4: use iomap for regular file's buffered IO path and enable large folio | expand |
On Wed, Apr 10, 2024 at 10:29:16PM +0800, Zhang Yi wrote: > From: Zhang Yi <yi.zhang@huawei.com> > > Now we lookup extent status entry without holding the i_data_sem before > inserting delalloc block, it works fine in buffered write path and > because it holds i_rwsem and folio lock, and the mmap path holds folio > lock, so the found extent locklessly couldn't be modified concurrently. > But it could be raced by fallocate since it allocate block whitout > holding i_rwsem and folio lock. > > ext4_page_mkwrite() ext4_fallocate() > block_page_mkwrite() > ext4_da_map_blocks() > //find hole in extent status tree > ext4_alloc_file_blocks() > ext4_map_blocks() > //allocate block and unwritten extent > ext4_insert_delayed_block() > ext4_da_reserve_space() > //reserve one more block > ext4_es_insert_delayed_block() > //drop unwritten extent and add delayed extent by mistake Shouldn't this be serialised by the file invalidation lock? Hole punching via fallocate must do this to avoid data use-after-free bugs w.r.t racing page faults and all the other fallocate ops need to serialise page faults to avoid page cache level data corruption. Yet here we see a problem resulting from a fallocate operation racing with a page fault.... Ah, I see that the invalidation lock is only picked up deep inside ext4_punch_hole(), ext4_collapse_range(), ext4_insert_range() and ext4_zero_range(). They all do the same flush, lock, and dio wait preamble but each do it just a little bit differently. The allocation path does it just a little bit differently again and does not take the invalidate lock... Perhaps the ext4 fallocate code should be factored so that all the fallocate operations run the same flush, lock and wait code rather than having 5 slightly different copies of the same code? Cheers, Dave.
Dave Chinner <david@fromorbit.com> writes: > On Wed, Apr 10, 2024 at 10:29:16PM +0800, Zhang Yi wrote: >> From: Zhang Yi <yi.zhang@huawei.com> >> >> Now we lookup extent status entry without holding the i_data_sem before >> inserting delalloc block, it works fine in buffered write path and >> because it holds i_rwsem and folio lock, and the mmap path holds folio >> lock, so the found extent locklessly couldn't be modified concurrently. >> But it could be raced by fallocate since it allocate block whitout >> holding i_rwsem and folio lock. >> >> ext4_page_mkwrite() ext4_fallocate() >> block_page_mkwrite() >> ext4_da_map_blocks() >> //find hole in extent status tree >> ext4_alloc_file_blocks() >> ext4_map_blocks() >> //allocate block and unwritten extent >> ext4_insert_delayed_block() >> ext4_da_reserve_space() >> //reserve one more block >> ext4_es_insert_delayed_block() >> //drop unwritten extent and add delayed extent by mistake > > Shouldn't this be serialised by the file invalidation lock? Hole > punching via fallocate must do this to avoid data use-after-free > bugs w.r.t racing page faults and all the other fallocate ops need > to serialise page faults to avoid page cache level data corruption. > Yet here we see a problem resulting from a fallocate operation > racing with a page fault.... IIUC, fallocate operations which invalidates the page cache contents needs to take th invalidate_lock in exclusive mode to prevent page fault operations from loading pages for stale mappings (blocks which were marked free might get reused). This can cause stale data exposure. Here the fallocate operation require allocation of unwritten extents and does not require truncate of pagecache range. So I guess, it is not strictly necessary to hold the invalidate lock here. But I see XFS does take IOLOCK_EXCL AND MMAPLOCK_EXCL even for this operation. I guess we could use the invalidate lock for fallocate operation in ext4 too. However, I think we still require the current patch. The reason is ext4_da_map_blocks() call here first tries to lookup the extent status cache w/o any i_data_sem lock in the fastpath. If it finds a hole, it takes the i_data_sem in write mode and just inserts an entry into extent status cache w/o re-checking for the same under the exclusive lock. ...So I believe we still should have this patch which re-verify under the write lock if whether any other operation has inserted any entry already or not. > > Ah, I see that the invalidation lock is only picked up deep inside > ext4_punch_hole(), ext4_collapse_range(), ext4_insert_range() and > ext4_zero_range(). They all do the same flush, lock, and dio wait > preamble but each do it just a little bit differently. The allocation path does > it just a little bit differently again and does not take the > invalidate lock... Yes, I think it is not stricly required to take invalidate lock in the allocation path of fallocate. Hence it could expose such a problem which existed in ext4_da_map_blocks(), right? > > Perhaps the ext4 fallocate code should be factored so that all the > fallocate operations run the same flush, lock and wait code rather > than having 5 slightly different copies of the same code? Yes. I agree. These paths can be refactored and if we are doing so, we may as well just use the invalidate lock as you suggested. -ritesh
On Wed, May 01, 2024 at 05:49:50PM +0530, Ritesh Harjani wrote: > Dave Chinner <david@fromorbit.com> writes: > > > On Wed, Apr 10, 2024 at 10:29:16PM +0800, Zhang Yi wrote: > >> From: Zhang Yi <yi.zhang@huawei.com> > >> > >> Now we lookup extent status entry without holding the i_data_sem before > >> inserting delalloc block, it works fine in buffered write path and > >> because it holds i_rwsem and folio lock, and the mmap path holds folio > >> lock, so the found extent locklessly couldn't be modified concurrently. > >> But it could be raced by fallocate since it allocate block whitout > >> holding i_rwsem and folio lock. > >> > >> ext4_page_mkwrite() ext4_fallocate() > >> block_page_mkwrite() > >> ext4_da_map_blocks() > >> //find hole in extent status tree > >> ext4_alloc_file_blocks() > >> ext4_map_blocks() > >> //allocate block and unwritten extent > >> ext4_insert_delayed_block() > >> ext4_da_reserve_space() > >> //reserve one more block > >> ext4_es_insert_delayed_block() > >> //drop unwritten extent and add delayed extent by mistake > > > > Shouldn't this be serialised by the file invalidation lock? Hole > > punching via fallocate must do this to avoid data use-after-free > > bugs w.r.t racing page faults and all the other fallocate ops need > > to serialise page faults to avoid page cache level data corruption. > > Yet here we see a problem resulting from a fallocate operation > > racing with a page fault.... > > IIUC, fallocate operations which invalidates the page cache contents needs > to take th invalidate_lock in exclusive mode to prevent page fault > operations from loading pages for stale mappings (blocks which were > marked free might get reused). This can cause stale data exposure. > > Here the fallocate operation require allocation of unwritten extents and > does not require truncate of pagecache range. So I guess, it is not > strictly necessary to hold the invalidate lock here. True, but you can make exactly the same argument for write() vs fallocate(). Yet this path in ext4_fallocate() locks out concurrent write()s and waits for DIOs in flight to drain. What makes buffered writes triggered by page faults special? i.e. if you are going to say "we don't need serialisation between writes and fallocate() allocating unwritten extents", then why is it still explicitly serialising against both buffered and direct IO and not just truncate and other fallocate() operations? > But I see XFS does take IOLOCK_EXCL AND MMAPLOCK_EXCL even for this operation. Yes, that's the behaviour preallocation has had in XFS since we introduced the MMAPLOCK almost a decade ago. This was long before the file_invalidation_lock() was even a glimmer in Jan's eye. btrfs does the same thing, for the same reasons. COW support makes extent tree manipulations excitingly complex at times... > I guess we could use the invalidate lock for fallocate operation in ext4 > too. However, I think we still require the current patch. The reason is > ext4_da_map_blocks() call here first tries to lookup the extent status > cache w/o any i_data_sem lock in the fastpath. If it finds a hole, it > takes the i_data_sem in write mode and just inserts an entry into extent > status cache w/o re-checking for the same under the exclusive lock. > ...So I believe we still should have this patch which re-verify under > the write lock if whether any other operation has inserted any entry > already or not. Yup, I never said the code in the patch is wrong or unnecessary; I'm commenting on the high level race condition that lead to the bug beting triggered. i.e. that racing data modification operations with low level extent manipulations is often dangerous and a potential source of very subtle, hard to trigger, reproduce and debug issues like the one reported... -Dave.
Dave Chinner <david@fromorbit.com> writes: > On Wed, May 01, 2024 at 05:49:50PM +0530, Ritesh Harjani wrote: >> Dave Chinner <david@fromorbit.com> writes: >> >> > On Wed, Apr 10, 2024 at 10:29:16PM +0800, Zhang Yi wrote: >> >> From: Zhang Yi <yi.zhang@huawei.com> >> >> >> >> Now we lookup extent status entry without holding the i_data_sem before >> >> inserting delalloc block, it works fine in buffered write path and >> >> because it holds i_rwsem and folio lock, and the mmap path holds folio >> >> lock, so the found extent locklessly couldn't be modified concurrently. >> >> But it could be raced by fallocate since it allocate block whitout >> >> holding i_rwsem and folio lock. >> >> >> >> ext4_page_mkwrite() ext4_fallocate() >> >> block_page_mkwrite() >> >> ext4_da_map_blocks() >> >> //find hole in extent status tree >> >> ext4_alloc_file_blocks() >> >> ext4_map_blocks() >> >> //allocate block and unwritten extent >> >> ext4_insert_delayed_block() >> >> ext4_da_reserve_space() >> >> //reserve one more block >> >> ext4_es_insert_delayed_block() >> >> //drop unwritten extent and add delayed extent by mistake >> > >> > Shouldn't this be serialised by the file invalidation lock? Hole >> > punching via fallocate must do this to avoid data use-after-free >> > bugs w.r.t racing page faults and all the other fallocate ops need >> > to serialise page faults to avoid page cache level data corruption. >> > Yet here we see a problem resulting from a fallocate operation >> > racing with a page fault.... >> >> IIUC, fallocate operations which invalidates the page cache contents needs >> to take th invalidate_lock in exclusive mode to prevent page fault >> operations from loading pages for stale mappings (blocks which were >> marked free might get reused). This can cause stale data exposure. >> >> Here the fallocate operation require allocation of unwritten extents and >> does not require truncate of pagecache range. So I guess, it is not >> strictly necessary to hold the invalidate lock here. > > True, but you can make exactly the same argument for write() vs > fallocate(). Yet this path in ext4_fallocate() locks out > concurrent write()s and waits for DIOs in flight to drain. What > makes buffered writes triggered by page faults special? > > i.e. if you are going to say "we don't need serialisation between > writes and fallocate() allocating unwritten extents", then why is it > still explicitly serialising against both buffered and direct IO and > not just truncate and other fallocate() operations? > >> But I see XFS does take IOLOCK_EXCL AND MMAPLOCK_EXCL even for this operation. > > Yes, that's the behaviour preallocation has had in XFS since we > introduced the MMAPLOCK almost a decade ago. This was long before > the file_invalidation_lock() was even a glimmer in Jan's eye. > > btrfs does the same thing, for the same reasons. COW support makes > extent tree manipulations excitingly complex at times... > >> I guess we could use the invalidate lock for fallocate operation in ext4 >> too. However, I think we still require the current patch. The reason is >> ext4_da_map_blocks() call here first tries to lookup the extent status >> cache w/o any i_data_sem lock in the fastpath. If it finds a hole, it >> takes the i_data_sem in write mode and just inserts an entry into extent >> status cache w/o re-checking for the same under the exclusive lock. >> ...So I believe we still should have this patch which re-verify under >> the write lock if whether any other operation has inserted any entry >> already or not. > > Yup, I never said the code in the patch is wrong or unnecessary; I'm > commenting on the high level race condition that lead to the bug > beting triggered. i.e. that racing data modification operations with > low level extent manipulations is often dangerous and a potential > source of very subtle, hard to trigger, reproduce and debug issues > like the one reported... > Yes, thanks for explaining and commenting on the high level design. It was indeed helpful. And I agree with your comment on, we can refactor out the common operations from fallocate path and use invalidate lock to protect against data modification (page fault) and extent manipulation path (fallocate operations). -ritesh
On 2024/5/2 12:11, Ritesh Harjani (IBM) wrote: > Dave Chinner <david@fromorbit.com> writes: > >> On Wed, May 01, 2024 at 05:49:50PM +0530, Ritesh Harjani wrote: >>> Dave Chinner <david@fromorbit.com> writes: >>> >>>> On Wed, Apr 10, 2024 at 10:29:16PM +0800, Zhang Yi wrote: >>>>> From: Zhang Yi <yi.zhang@huawei.com> >>>>> >>>>> Now we lookup extent status entry without holding the i_data_sem before >>>>> inserting delalloc block, it works fine in buffered write path and >>>>> because it holds i_rwsem and folio lock, and the mmap path holds folio >>>>> lock, so the found extent locklessly couldn't be modified concurrently. >>>>> But it could be raced by fallocate since it allocate block whitout >>>>> holding i_rwsem and folio lock. >>>>> >>>>> ext4_page_mkwrite() ext4_fallocate() >>>>> block_page_mkwrite() >>>>> ext4_da_map_blocks() >>>>> //find hole in extent status tree >>>>> ext4_alloc_file_blocks() >>>>> ext4_map_blocks() >>>>> //allocate block and unwritten extent >>>>> ext4_insert_delayed_block() >>>>> ext4_da_reserve_space() >>>>> //reserve one more block >>>>> ext4_es_insert_delayed_block() >>>>> //drop unwritten extent and add delayed extent by mistake >>>> >>>> Shouldn't this be serialised by the file invalidation lock? Hole >>>> punching via fallocate must do this to avoid data use-after-free >>>> bugs w.r.t racing page faults and all the other fallocate ops need >>>> to serialise page faults to avoid page cache level data corruption. >>>> Yet here we see a problem resulting from a fallocate operation >>>> racing with a page fault.... >>> >>> IIUC, fallocate operations which invalidates the page cache contents needs >>> to take th invalidate_lock in exclusive mode to prevent page fault >>> operations from loading pages for stale mappings (blocks which were >>> marked free might get reused). This can cause stale data exposure. >>> >>> Here the fallocate operation require allocation of unwritten extents and >>> does not require truncate of pagecache range. So I guess, it is not >>> strictly necessary to hold the invalidate lock here. >> >> True, but you can make exactly the same argument for write() vs >> fallocate(). Yet this path in ext4_fallocate() locks out >> concurrent write()s and waits for DIOs in flight to drain. What >> makes buffered writes triggered by page faults special? >> >> i.e. if you are going to say "we don't need serialisation between >> writes and fallocate() allocating unwritten extents", then why is it >> still explicitly serialising against both buffered and direct IO and >> not just truncate and other fallocate() operations? >> >>> But I see XFS does take IOLOCK_EXCL AND MMAPLOCK_EXCL even for this operation. >> >> Yes, that's the behaviour preallocation has had in XFS since we >> introduced the MMAPLOCK almost a decade ago. This was long before >> the file_invalidation_lock() was even a glimmer in Jan's eye. >> >> btrfs does the same thing, for the same reasons. COW support makes >> extent tree manipulations excitingly complex at times... >> >>> I guess we could use the invalidate lock for fallocate operation in ext4 >>> too. However, I think we still require the current patch. The reason is >>> ext4_da_map_blocks() call here first tries to lookup the extent status >>> cache w/o any i_data_sem lock in the fastpath. If it finds a hole, it >>> takes the i_data_sem in write mode and just inserts an entry into extent >>> status cache w/o re-checking for the same under the exclusive lock. >>> ...So I believe we still should have this patch which re-verify under >>> the write lock if whether any other operation has inserted any entry >>> already or not. >> >> Yup, I never said the code in the patch is wrong or unnecessary; I'm >> commenting on the high level race condition that lead to the bug >> beting triggered. i.e. that racing data modification operations with >> low level extent manipulations is often dangerous and a potential >> source of very subtle, hard to trigger, reproduce and debug issues >> like the one reported... >> > > Yes, thanks for explaining and commenting on the high level design. > It was indeed helpful. And I agree with your comment on, we can refactor > out the common operations from fallocate path and use invalidate lock to > protect against data modification (page fault) and extent manipulation > path (fallocate operations). > Yeah, thanks for explanation and suggestion, too. After looking at your discussion, I also suppose we could refactor a common helper and use the file invalidation lock for the whole ext4 fallocate path, current code is too scattered. Thanks, Yi.
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 6a41172c06e1..118b0497a954 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -1737,6 +1737,7 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock, if (ext4_es_is_hole(&es)) goto add_delayed; +found: /* * Delayed extent could be allocated by fallocate. * So we need to check it. @@ -1781,6 +1782,24 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock, add_delayed: down_write(&EXT4_I(inode)->i_data_sem); + /* + * Lookup extents tree again under i_data_sem, make sure this + * inserting delalloc range haven't been delayed or allocated + * whitout holding i_rwsem and folio lock. + */ + if (ext4_es_lookup_extent(inode, iblock, NULL, &es)) { + if (!ext4_es_is_hole(&es)) { + up_write(&EXT4_I(inode)->i_data_sem); + goto found; + } + } else if (!ext4_has_inline_data(inode)) { + retval = ext4_map_query_blocks(NULL, inode, map); + if (retval) { + up_write(&EXT4_I(inode)->i_data_sem); + return retval; + } + } + retval = ext4_insert_delayed_block(inode, map->m_lblk); up_write(&EXT4_I(inode)->i_data_sem); if (retval)