Re: [PATCH] btrfs: reloc: Fix NULL pointer dereference due to expanded reloc_root lifespan

[David Sterba <dsterba@suse.cz>]

On Mon, Mar 18, 2019 at 10:48:19AM +0800, Qu Wenruo wrote:
> Commit 514e70150b60 ("btrfs: relocation: Delay reloc tree deletion after
> merge_reloc_roots()") expands the life span of root->reloc_root.

Commit 514e70150b60 does not exist in linus.git, by the subject it's
d2311e69857815a .

> This breaks certain check on fs_info->reloc_ctl.
> Before that commit, if we have a root with valid reloc_root, then it's
> ensured to have fs_info->reloc_ctl.
> 
> But now since reloc_root doesn't always mean a valid fs_info->reloc_ctl,
> such check is unreliable and can cause the following NULL pointer
> dereference:
> BUG: unable to handle kernel NULL pointer dereference at 00000000000005c1
> IP: btrfs_reloc_pre_snapshot+0x20/0x50 [btrfs]
> PGD 0 P4D 0
> Oops: 0000 [#1] SMP PTI
> CPU: 0 PID: 10379 Comm: snapperd Not tainted 4.12.14-lp150.12.53-default #1 openSUSE Leap 15.0

This is probably a backport to 4.12, the patch that caused that has been
merged to master recently. I wonder if this can also be triggered on
unpatched mainline, ie. if we're missing a test for that.

I'll fix the commit and add it to misc-next, thanks.

Reviewed-by: David Sterba <dsterba@suse.com>

> Call Trace:
>  create_pending_snapshot+0xd7/0xfc0 [btrfs]
>  create_pending_snapshots+0x8e/0xb0 [btrfs]
>  btrfs_commit_transaction+0x2ac/0x8f0 [btrfs]
>  btrfs_mksubvol+0x561/0x570 [btrfs]
>  btrfs_ioctl_snap_create_transid+0x189/0x190 [btrfs]
>  btrfs_ioctl_snap_create_v2+0x102/0x150 [btrfs]
>  btrfs_ioctl+0x5c9/0x1e60 [btrfs]
>  do_vfs_ioctl+0x90/0x5f0
>  SyS_ioctl+0x74/0x80
>  do_syscall_64+0x7b/0x150
>  entry_SYSCALL_64_after_hwframe+0x3d/0xa2
> RIP: 0033:0x7fd7cdab8467
> 
> Fix it by explicitly checking fs_info->reloc_ctl other than using the
> implied root->reloc_root.
> 
> Fixes: 514e70150b60 ("btrfs: relocation: Delay reloc tree deletion after merge_reloc_roots()")
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/relocation.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index ddf028509931..351fa506dc9b 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -4667,14 +4667,12 @@ int btrfs_reloc_cow_block(struct btrfs_trans_handle *trans,
>  void btrfs_reloc_pre_snapshot(struct btrfs_pending_snapshot *pending,
>  			      u64 *bytes_to_reserve)
>  {
> -	struct btrfs_root *root;
> -	struct reloc_control *rc;
> +	struct btrfs_root *root = pending->root;
> +	struct reloc_control *rc = root->fs_info->reloc_ctl;
>  
> -	root = pending->root;
> -	if (!root->reloc_root)
> +	if (!root->reloc_root || !rc)
>  		return;
>  
> -	rc = root->fs_info->reloc_ctl;
>  	if (!rc->merge_reloc_tree)
>  		return;
>  
> @@ -4703,10 +4701,10 @@ int btrfs_reloc_post_snapshot(struct btrfs_trans_handle *trans,
>  	struct btrfs_root *root = pending->root;
>  	struct btrfs_root *reloc_root;
>  	struct btrfs_root *new_root;
> -	struct reloc_control *rc;
> +	struct reloc_control *rc = root->fs_info->reloc_ctl;
>  	int ret;
>  
> -	if (!root->reloc_root)
> +	if (!root->reloc_root || !rc)
>  		return 0;
>  
>  	rc = root->fs_info->reloc_ctl;
> -- 
> 2.21.0