| Message ID | 20200509052001.2298-1-baijiaju1990@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] fs: btrfs: fix a data race in btrfs_block_group_done() | expand |
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. > } > > #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS >
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
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; } #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
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(-)