Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 1/4] fs: btrfs: fix a data race in
 btrfs_block_group_done()
To:     Nikolay Borisov <nborisov@suse.com>, clm@fb.com,
        josef@toxicpanda.com, dsterba@suse.com
Cc:     linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20200509052001.2298-1-baijiaju1990@gmail.com>
 <21982360-ee20-edfd-bee9-cbea04b3893f@suse.com>
From:   Jia-Ju Bai <baijiaju1990@gmail.com>
Message-ID: <d991d28c-986a-0efc-e10e-62fe516dd7c6@gmail.com>
Date:   Sat, 9 May 2020 19:20:58 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.4.1
MIME-Version: 1.0
In-Reply-To: <21982360-ee20-edfd-bee9-cbea04b3893f@suse.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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