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:
signature.asc
Description: OpenPGP digital signature
