lkml.org 
[lkml]   [2020]   [May]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/4] fs: btrfs: fix a data race in btrfs_block_group_done()
From
Date


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

\
 
 \ /
  Last update: 2020-05-09 13:22    [W:0.076 / U:0.212 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site