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

[Qu WenRuo <wqu@suse.com>]

On 2020/1/8 下午8:28, Nikolay Borisov wrote:
> 
> 
> On 8.01.20 г. 7:12 ч., Qu Wenruo wrote:
>> [BUG]
>> There are several different KASAN reports for balance + snapshot
>> workloads.
>> Involved call paths include:
>>
>>    should_ignore_root+0x54/0xb0 [btrfs]
>>    build_backref_tree+0x11af/0x2280 [btrfs]
>>    relocate_tree_blocks+0x391/0xb80 [btrfs]
>>    relocate_block_group+0x3e5/0xa00 [btrfs]
>>    btrfs_relocate_block_group+0x240/0x4d0 [btrfs]
>>    btrfs_relocate_chunk+0x53/0xf0 [btrfs]
>>    btrfs_balance+0xc91/0x1840 [btrfs]
>>    btrfs_ioctl_balance+0x416/0x4e0 [btrfs]
>>    btrfs_ioctl+0x8af/0x3e60 [btrfs]
>>    do_vfs_ioctl+0x831/0xb10
>>    ksys_ioctl+0x67/0x90
>>    __x64_sys_ioctl+0x43/0x50
>>    do_syscall_64+0x79/0xe0
>>    entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>
>>    create_reloc_root+0x9f/0x460 [btrfs]
>>    btrfs_reloc_post_snapshot+0xff/0x6c0 [btrfs]
>>    create_pending_snapshot+0xa9b/0x15f0 [btrfs]
>>    create_pending_snapshots+0x111/0x140 [btrfs]
>>    btrfs_commit_transaction+0x7a6/0x1360 [btrfs]
>>    btrfs_mksubvol+0x915/0x960 [btrfs]
>>    btrfs_ioctl_snap_create_transid+0x1d5/0x1e0 [btrfs]
>>    btrfs_ioctl_snap_create_v2+0x1d3/0x270 [btrfs]
>>    btrfs_ioctl+0x241b/0x3e60 [btrfs]
>>    do_vfs_ioctl+0x831/0xb10
>>    ksys_ioctl+0x67/0x90
>>    __x64_sys_ioctl+0x43/0x50
>>    do_syscall_64+0x79/0xe0
>>    entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>
>>    btrfs_reloc_pre_snapshot+0x85/0xc0 [btrfs]
>>    create_pending_snapshot+0x209/0x15f0 [btrfs]
>>    create_pending_snapshots+0x111/0x140 [btrfs]
>>    btrfs_commit_transaction+0x7a6/0x1360 [btrfs]
>>    btrfs_mksubvol+0x915/0x960 [btrfs]
>>    btrfs_ioctl_snap_create_transid+0x1d5/0x1e0 [btrfs]
>>    btrfs_ioctl_snap_create_v2+0x1d3/0x270 [btrfs]
>>    btrfs_ioctl+0x241b/0x3e60 [btrfs]
>>    do_vfs_ioctl+0x831/0xb10
>>    ksys_ioctl+0x67/0x90
>>    __x64_sys_ioctl+0x43/0x50
>>    do_syscall_64+0x79/0xe0
>>    entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>
>> [CAUSE]
>> All these call sites are only relying on root->reloc_root, which can
>> undergo btrfs_drop_snapshot(), and since we don't have real refcount
> 
> what do you mean by "root->reloc_root can undergo btrfs_drop_snapshot" ?

I mean some caller got root->reloc_root and use it, while
root->reloc_root soon get dropped by btrfs_drop_snapshot().

> 
>> based protection to reloc roots, we can reach already dropped reloc
>> root, triggering KASAN.
> what's the relationship between not having a refcount protection and
> reaching reloc roots, perhaps you could expand the explanation?

If we had a proper refcount protection, we could wait until we're the
last holder of reloc_root before calling btrfs_drop_snapshot().

And to me, that should be the correct solution, while this patch is just
a quick and maybe dirty fix mostly for backport.

> 
>>
>> [FIX]
>> To avoid such access to unstable root->reloc_root, we should check
>> BTRFS_ROOT_DEAD_RELOC_TREE bit first.
>>
>> This patch introduces a new wrapper, have_reloc_root(), to do the proper
>> check for most callers who don't distinguish merged reloc tree and no
>> reloc tree.
>>
>> The only exception is should_ignore_root(), as merged reloc tree can be
>> ignored, while no reloc tree shouldn't.
>>
>> [CRITICAL SECTION ANALYSE]
>> Although test_bit()/set_bit()/clear_bit() doesn't imply a barrier, the
>> DEAD_RELOC_TREE bit has extra help from transaction as a higher level
>> barrier, the lifespan of root::reloc_root and DEAD_RELOC_TREE bit are:
>>
>> 	NULL: reloc_root is NULL	PTR: reloc_root is not NULL
>> 	0: DEAD_RELOC_ROOT bit not set	DEAD: DEAD_RELOC_ROOT bit set
>>
>> 	(NULL, 0)    Initial state		 __
>> 	  |					 /\ Section A
>>         btrfs_init_reloc_root()			 \/
>> 	  |				 	 __
>> 	(PTR, 0)     reloc_root initialized      /\
>>           |					 |
>> 	btrfs_update_reloc_root()		 |  Section B
>>           |					 |
>> 	(PTR, DEAD)  reloc_root has been merged  \/
>>           |					 __
>> 	=== btrfs_commit_transaction() ====================
>> 	  |					 /\
>> 	clean_dirty_subvols()			 |
>> 	  |					 |  Section C
>> 	(NULL, DEAD) reloc_root cleanup starts   \/
>>           |					 __
>> 	btrfs_drop_snapshot()			 /\
>> 	  |					 |  Section D
>> 	(NULL, 0)    Back to initial state	 \/
>>
>> Very have_reloc_root() or test_bit(DEAD_RELOC_ROOT) caller has hold a
> 
>  ^^ Perhaps you meant: Every caller of have_reloc_root or
> test_bit(DED_RELOC_ROOT) holds a transaction handle which ensures
> modifications in those function are limited to a single transaction?

Yep, I mean *E*very.
It looks I need to replace the switch of my 'e' key...

Thanks,
Qu