Re: [PATCH 1/4] fs: btrfs: fix a data race in btrfs_block_group_done()

Re: [PATCH 1/4] fs: btrfs: fix a data race in btrfs_block_group_done()

[Markus Elfring]

> This data race was found and actually reproduced by our concurrency fuzzer.

I have got the impression that this patch series has got a questionable
mail threading.
Will it be helpful to resend it with a cover letter together with a few
adjustments for the corresponding change descriptions?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=d5eeab8d7e269e8cfc53b915bccd7bd30485bcbf#n785

Regards,
Markus

Re: [PATCH 1/4] fs: btrfs: fix a data race in btrfs_block_group_done()

[Jia-Ju Bai]

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

Re: [PATCH 1/4] fs: btrfs: fix a data race in btrfs_block_group_done()

[Nikolay Borisov]

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
> 

Re: [PATCH 1/4] fs: btrfs: fix a data race in btrfs_block_group_done()

[Markus Elfring]

> To fix this race, the spinlock cache->lock is used to protect the
> access to cache->cached in btrfs_block_group_done().

How do you think about to replace this wording by a variant like the following?

   Thus use the spin lock “cache->lock” to protect the access to
   the data structure member “cached” in the implementation of
   the function “btrfs_block_group_done”.


Would you like to add the tag “Fixes” to the change description?

Regards,
Markus

Re: [PATCH 1/4] fs: btrfs: fix a data race in btrfs_block_group_done()

[Markus Elfring]

> To fix this race, the spinlock cache->lock is used to protect the
> access to cache->cached in btrfs_block_group_done().

How do you think about to replace this wording by a variant like the following?

   Thus use the spin lock “cache->lock” to protect the access to
   the data structure member “cached” in the implementation of
   the function “btrfs_block_group_done”.


Would you like to add the tag “Fixes” to the change description?

Regards,
Markus

[PATCH 1/4] fs: btrfs: fix a data race in btrfs_block_group_done()

[Jia-Ju Bai]

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;
 }
 
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
-- 
2.17.1