Re: [PATCH 6/6] btrfs-progs: rescue-super: Don't double free fs_devices

[Nikolay Borisov <nborisov@suse.com>]

On  3.08.2018 08:50, Qu Wenruo wrote:
> [BUG]
> During fuzz/007 we hit the following error:
> ------
> ====== RUN MAYFAIL /home/adam/btrfs/btrfs-progs/btrfs rescue super-recover -y -v /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-200409.raw.restored.scratch
> ERROR: tree_root block unaligned: 33554431
> ERROR: superblock checksum matches but it has invalid members
> ERROR: tree_root block unaligned: 33554431
> ERROR: superblock checksum matches but it has invalid members
> ERROR: tree_root block unaligned: 33554431
> ERROR: superblock checksum matches but it has invalid members
> ERROR: failed to add chunk map start=12582912 len=8454144: -17 (File exists)
> Couldn't read chunk tree
> failed (ignored, ret=139): /home/adam/btrfs/btrfs-progs/btrfs rescue super-recover -y -v /home/adam/btrfs/btrfs-progs/tests//fuzz-tests/images/bko-200409.raw.restored.scratch
> mayfail: returned code 139 (SEGFAULT), not ignored
> test failed for case 007-simple-super-recover
> ------
> 
> [CAUSE]
> In __open_ctree_fd(), if we have valid @open_ctree_flags and
> btrfs_scan_fs_devices() successes without problem, no matter what
> happens we will call btrfs_close_devices(), thus free all related
> devices.

Why do you think it's _always_ going to be called? Looking into that
function it seems this can happen if
btrfs_setup_chunk_tree_and_device_map fails.
> 
> In super-recover, before we call open_ctree(), we have called
> btrfs_scan_fs_devices() already, so btrfs_scan_fs_devices() should not
> fail in open_ctree(), fs_devices will always be freed in open_ctree() or
> close_ctree().

Isn't the actual issue just that we call close_ctree. So the actual life
time of fs_devices is :

1. Create in btrfs_scan_fs_devices called from btrfs_recover_superblocks
2. All other references to those fs_devices will just return the same
reference.
3. Calling close_ctree frees fs_devices.

> 
> [FIX]
> So in super-recover.c, we should not call btrfs_close_devices(), or we
> will find fs_devices->list get poisoned, and trigger segfault when
> exiting.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  super-recover.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/super-recover.c b/super-recover.c
> index 880fd7712546..86b3df9867dc 100644
> --- a/super-recover.c
> +++ b/super-recover.c
> @@ -292,9 +292,6 @@ int btrfs_recover_superblocks(const char *dname,
>  no_recover:
>  	recover_err_str(ret);
>  	free_recover_superblock(&recover);
> -	/* check if we have freed fs_devices in close_ctree() */
> -	if (!root)
> -		btrfs_close_devices(recover.fs_devices);
>  	return ret;
>  }
>  
>