Re: [PATCH 4/6] btrfs: qgroup: Introduce per-root swapped blocks infrastructure

[Qu Wenruo <wqu@suse.de>]

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



On 2018/10/19 下午5:15, David Sterba wrote:
> On Fri, Oct 19, 2018 at 07:29:26AM +0800, Qu Wenruo wrote:
>>
>>
>> On 2018/10/19 上午12:20, David Sterba wrote:
>>> On Thu, Oct 18, 2018 at 07:17:27PM +0800, Qu Wenruo wrote:
>>>> +void btrfs_qgroup_clean_swapped_blocks(struct btrfs_root *root)
>>>> +{
>>>> +	struct btrfs_qgroup_swapped_blocks *swapped_blocks;
>>>> +	struct btrfs_qgroup_swapped_block *cur, *next;
>>>> +	int i;
>>>> +
>>>> +	swapped_blocks = &root->swapped_blocks;
>>>> +
>>>> +	spin_lock(&swapped_blocks->lock);
>>>> +	if (!swapped_blocks->swapped)
>>>> +		goto out;
>>>> +	for (i = 0; i < BTRFS_MAX_LEVEL; i++) {
>>>> +		struct rb_root *cur_root = &swapped_blocks->blocks[i];
>>>> +
>>>> +		rbtree_postorder_for_each_entry_safe(cur, next, cur_root,
>>>> +						     node) {
>>>> +			rb_erase(&cur->node, cur_root);
>>>
>>> https://elixir.bootlin.com/linux/latest/source/include/linux/rbtree.h#L141
>>>
>>> rb_erase must not be used on the same pointer that
>>> rbtree_postorder_for_each_entry_safe iterates, here it's cur.
>>
>> Oh, thanks for pointing this out.
>>
>> So we must go the old while(rb_first()) loop...
> 
> I just realized the postorder iterator can be used here. The forbidden
> pattern is
> 
> rbtree_postorder_for_each_entry_safe() {
> 	rb_erase();
> }

Did you mean some like this is possible?

rbtree_postorder_for_each_entry_safe() {
	kfree(entry);
}

If so, I still don't really believe it's OK.

For the following tree:
                          4
                       /     \
                      2       6
                    /  \     / \
                   1    3   5   7

If current entry is 2, next is 3.
And 2 get freed.
Then we go 3, to reach next we need to go back to access 2, which is
already freed, we will trigger use-after-free.

So the only correct way to free the whole rbtree is still that tried and
true while(rb_first()) loop.

Or did I miss something?

Thanks,
Qu

> 
> But you also do kfree right after the erase, and the iterator is fine
> with that and rb_erase is pointless for an entry that's being freed
> anyway.
> 


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