Messages in this thread | | | Subject | Re: [PATCH 1/4] fs: btrfs: fix a data race in btrfs_block_group_done() | From | Jia-Ju Bai <> | Date | Sat, 9 May 2020 19:20:58 +0800 |
| |
On 2020/5/9 18:53, Nikolay Borisov wrote: > > On 9.05.20 г. 8:20 ч., Jia-Ju Bai wrote: >> The functions btrfs_block_group_done() and caching_thread() are >> concurrently executed at runtime in the following call contexts: >> >> Thread 1: >> btrfs_sync_file() >> start_ordered_ops() >> btrfs_fdatawrite_range() >> btrfs_writepages() [via function pointer] >> extent_writepages() >> extent_write_cache_pages() >> __extent_writepage() >> writepage_delalloc() >> btrfs_run_delalloc_range() >> cow_file_range() >> btrfs_reserve_extent() >> find_free_extent() >> btrfs_block_group_done() >> >> Thread 2: >> caching_thread() >> >> In btrfs_block_group_done(): >> smp_mb(); >> return cache->cached == BTRFS_CACHE_FINISHED || >> cache->cached == BTRFS_CACHE_ERROR; >> >> In caching_thread(): >> spin_lock(&block_group->lock); >> block_group->caching_ctl = NULL; >> block_group->cached = ret ? BTRFS_CACHE_ERROR : BTRFS_CACHE_FINISHED; >> spin_unlock(&block_group->lock); >> >> The values cache->cached and block_group->cached access the same memory, >> and thus a data race can occur. >> This data race was found and actually reproduced by our concurrency >> fuzzer. >> >> To fix this race, the spinlock cache->lock is used to protect the >> access to cache->cached in btrfs_block_group_done(). >> >> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> >> --- >> fs/btrfs/block-group.h | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/fs/btrfs/block-group.h b/fs/btrfs/block-group.h >> index 107bb557ca8d..fb5f12acea40 100644 >> --- a/fs/btrfs/block-group.h >> +++ b/fs/btrfs/block-group.h >> @@ -278,9 +278,13 @@ static inline u64 btrfs_system_alloc_profile(struct btrfs_fs_info *fs_info) >> >> static inline int btrfs_block_group_done(struct btrfs_block_group *cache) >> { >> + int flag; >> smp_mb(); >> - return cache->cached == BTRFS_CACHE_FINISHED || >> - cache->cached == BTRFS_CACHE_ERROR; >> + spin_lock(&cache->lock); >> + flag = (cache->cached == BTRFS_CACHE_FINISHED || >> + cache->cached == BTRFS_CACHE_ERROR); >> + spin_unlock(&cache->lock); >> + return flag; > Using the lock also obviates the need for the memory barrier. > Furthermore this race is benign because even if it occurs and we call > into btrfs_cache_block_group we do check cache->cached under > btrfs_block_group::lock and do the right thing, though we would have > done a bit more unnecessary wor if that's the case. So have you actually > measured the effect of adding the the spinlock? This is likely done so > as to make the fastpath lock-free. Perhaps a better approach would be to > annotate the access of cached with READ/WRITE once so that it's fetched > from memory and not optimized out by the compiler and leave the more > heavy work in the unlikely path. > > Please exercise some critical thinking when looking into such issues as > there might be a good reason why something has been coded in a > particular way and it might look wrong on a first look but in fact is not.
Okay, thanks a lot for your explanation and advice :)
Best wishes, Jia-Ju Bai
| |