Re: [PATCH] btrfs: relocation: Fix KASAN reports caused by extended reloc tree lifespan

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5612 bytes --]



On 2020/1/7 上午10:30, Qu Wenruo wrote:
> 
> 
> On 2020/1/7 上午2:15, David Sterba wrote:
>> On Sat, Jan 04, 2020 at 09:56:02PM +0800, Qu Wenruo wrote:
>>>  }
>>>  
>>> +/*
>>> + * Check if this subvolume tree has valid reloc(*) tree.
>>> + *
>>> + * *: Reloc tree after swap is considered dead, thus not considered as valid.
>>> + *    This is enough for most callers, as they don't distinguish dead reloc
>>> + *    root from no reloc root.
>>> + *    But should_ignore_root() below is a special case.
>>> + */
>>> +static bool have_reloc_root(struct btrfs_root *root)
>>> +{
>>> +	smp_mb__before_atomic();
>>
>> That one should be the easiest, to get an up to date value of the bit,
>> sync before reading it. Similar to smp_rmb.
> 
> Yep, if we just go plain rmb/wmb everything would be much easier to
> understand.
> 
> But since full rmb/wmb is expensive and in this case we're only address
> certain arches which doesn't follower Total Store Order, we still need
> to use that variant.
> 
> 
> One more question.
> 
> Why not use before_atomic() and after_atomic() to surround the
> set_bit()/test_bit()?

Typo, it's set_bit()/clear_bit().

> 
> If before and after acts as rmb/wmb, then we don't really need this
> before_atomic call aroudn test_bit()
> 
>>
>>> +	if (test_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state))
>>> +		return false;
>>> +	if (!root->reloc_root)
>>> +		return false;
>>> +	return true;
>>> +}
>>>  
>>>  static int should_ignore_root(struct btrfs_root *root)
>>>  {
>>> @@ -525,6 +542,11 @@ static int should_ignore_root(struct btrfs_root *root)
>>>  	if (!test_bit(BTRFS_ROOT_REF_COWS, &root->state))
>>>  		return 0;
>>>  
>>> +	/* This root has been merged with its reloc tree, we can ignore it */
>>> +	smp_mb__before_atomic();
>>
>> This could be replaced by have_reloc_root but the reloc_root has to be
>> check twice in that function. Here it was slightly optimized as it
>> partially opencodes have_reloc_root. For clarity and fewer standalone
>> barriers using the helper might be better.
> 
> The problem is, have_reloc_root() returns false if either:
> - DEAD_RELOC_TREE is set
> - no reloc_root
> 
> What we really want is, if bit set, return 1, but if no reloc root,
> return 0 instead.
> 
> So we can't use that helper at all.
> 
>>
>>> +	if (test_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state))
>>> +		return 1;
>>> +
>>>  	reloc_root = root->reloc_root;
>>>  	if (!reloc_root)
>>>  		return 0;
>>> @@ -1439,6 +1461,7 @@ int btrfs_init_reloc_root(struct btrfs_trans_handle *trans,
>>>  	 * The subvolume has reloc tree but the swap is finished, no need to
>>>  	 * create/update the dead reloc tree
>>>  	 */
>>> +	smp_mb__before_atomic();
>>
>> Another partial have_reloc_root, could be used here as well with
>> additional reloc_tree check.
> 
> Same problem as should_ignore_root().
> Or we need to do extra reloc_root out of the helper.
> 
>>
>>>  	if (test_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state))
>>>  		return 0;
>>>  
>>> @@ -1478,8 +1501,7 @@ int btrfs_update_reloc_root(struct btrfs_trans_handle *trans,
>>>  	struct btrfs_root_item *root_item;
>>>  	int ret;
>>>  
>>> -	if (test_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state) ||
>>> -	    !root->reloc_root)
>>> +	if (!have_reloc_root(root))
>>>  		goto out;
>>>  
>>>  	reloc_root = root->reloc_root;
>>> @@ -1489,6 +1511,7 @@ int btrfs_update_reloc_root(struct btrfs_trans_handle *trans,
>>>  	if (fs_info->reloc_ctl->merge_reloc_tree &&
>>>  	    btrfs_root_refs(root_item) == 0) {
>>>  		set_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state);
>>
>> First set the bit, so anybody who properly uses barriers before checking
>> the bit will see it set
> 
> Still the same question, why not use before and after version around
> set_bit()/clear_bit() so test_bit() doesn't need extra before_atomic call?
>>
>>> +		smp_mb__after_atomic();
>>
>> since the reloc_root pointer is not safe to be accessed since this point
>>
>>>  		__del_reloc_root(reloc_root);
>>>  	}
>>>  
>>> @@ -2202,6 +2225,7 @@ static int clean_dirty_subvols(struct reloc_control *rc)
>>>  					ret = ret2;
>>>  			}
>>>  			clear_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state);
>>
>> This one looks misplaced and reverse, root->reloc_root is set to NULL a
>> few lines before and the barrier must be between this and clear_bit.
> 
> Got the point.
> 
>> This was not in my proposed version, why did you change that?
> 
> I thought the clear_bit() must be visible for all later operations, thus
> the after_atomic() is needed.
> But forgot the reloc_root is set to NULL.
> 
> So in that case, I guess we need both barriers.
> 
> Thanks,
> Qu
> 
>>
>>
>>
>>> +			smp_mb__after_atomic();
>>>  			btrfs_put_fs_root(root);
>>>  		} else {
>>>  			/* Orphan reloc tree, just clean it up */
>>> @@ -4717,7 +4741,7 @@ void btrfs_reloc_pre_snapshot(struct btrfs_pending_snapshot *pending,
>>>  	struct btrfs_root *root = pending->root;
>>>  	struct reloc_control *rc = root->fs_info->reloc_ctl;
>>>  
>>> -	if (!root->reloc_root || !rc)
>>> +	if (!rc || !have_reloc_root(root))
>>>  		return;
>>>  
>>>  	if (!rc->merge_reloc_tree)
>>> @@ -4751,7 +4775,7 @@ int btrfs_reloc_post_snapshot(struct btrfs_trans_handle *trans,
>>>  	struct reloc_control *rc = root->fs_info->reloc_ctl;
>>>  	int ret;
>>>  
>>> -	if (!root->reloc_root || !rc)
>>> +	if (!rc || !have_reloc_root(root))
>>>  		return 0;
>>>  
>>>  	rc = root->fs_info->reloc_ctl;
>>> -- 
>>> 2.24.1
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]