Re: [PATCH] btrfs: check for missing device in btrfs_trim_fs

From: Qu Wenruo <quwenruo.btrfs@gmx.com>
To: Anand Jain <anand.jain@oracle.com>, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH] btrfs: check for missing device in btrfs_trim_fs
Date: Mon, 5 Jul 2021 08:12:34 +0800	[thread overview]
Message-ID: <9786b027-e380-6286-4ec9-e28d7c6d46f1@gmx.com> (raw)
In-Reply-To: <fce2724eaa78b9666c2ac4f0a1b806ae14c55cd0.1625236214.git.anand.jain@oracle.com>

On 2021/7/4 下午7:14, Anand Jain wrote:
> A fstrim on a degraded raid1 can trigger the following null pointer
> dereference:
>
> BTRFS info (device loop0): allowing degraded mounts
> BTRFS info (device loop0): disk space caching is enabled
> BTRFS info (device loop0): has skinny extents
> BTRFS warning (device loop0): devid 2 uuid 97ac16f7-e14d-4db1-95bc-3d489b424adb is missing
> BTRFS warning (device loop0): devid 2 uuid 97ac16f7-e14d-4db1-95bc-3d489b424adb is missing
> BTRFS info (device loop0): enabling ssd optimizations
> BUG: kernel NULL pointer dereference, address: 0000000000000620
> PGD 0 P4D 0
> Oops: 0000 [#1] SMP NOPTI
> CPU: 0 PID: 4574 Comm: fstrim Not tainted 5.13.0-rc7+ #31
> Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
> RIP: 0010:btrfs_trim_fs+0x199/0x4a0 [btrfs]
> Code: 24 10 65 4c 8b 34 25 00 70 01 00 48 c7 44 24 38 00 00 10 00 48 8b 45 50 48 c7 44 24 40 00 00 00 00 48 c7 44 24 30 00 00 00 00 <48> 8b 80 20 06 00 00 48 8b 80 90 00 00 00 48 8b 40 68 f6 c4 01 0f
> RSP: 0018:ffff959541797d28 EFLAGS: 00010293
> RAX: 0000000000000000 RBX: ffff946f84eca508 RCX: a7a67937adff8608
> RDX: ffff946e8122d000 RSI: 0000000000000000 RDI: ffffffffc02fdbf0
> RBP: ffff946ea4615000 R08: 0000000000000001 R09: 0000000000000000
> R10: 0000000000000000 R11: ffff946e8122d960 R12: 0000000000000000
> R13: ffff959541797db8 R14: ffff946e8122d000 R15: ffff959541797db8
> FS:  00007f55917a5080(0000) GS:ffff946f9bc00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000620 CR3: 000000002d2c8001 CR4: 00000000000706f0
> Call Trace:
> btrfs_ioctl_fitrim+0x167/0x260 [btrfs]
> btrfs_ioctl+0x1c00/0x2fe0 [btrfs]
> ? selinux_file_ioctl+0x140/0x240
> ? syscall_trace_enter.constprop.0+0x188/0x240
> ? __x64_sys_ioctl+0x83/0xb0
> __x64_sys_ioctl+0x83/0xb0
> do_syscall_64+0x40/0x80
> entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> Reproducer:
>
>    Create raid1 btrfs:
> 	mkfs.btrfs -fq -draid1 -mraid1 /dev/loop0 /dev/loop1
> 	mount /dev/loop0 /btrfs
>
>    Create some data:
> 	_sysbench prepare /btrfs 10 2g 6
>
>    Mount with one the device missing:
> 	umount /btrfs
> 	btrfs dev scan --forget
> 	mount -o degraded /dev/loop0 /btrfs
>
>    Run fstrim:
> 	fstrim /btrfs
>
> The reason is we call btrfs_trim_free_extents() for the missing device,
> which uses device->bdev (NULL for missing device) to find if the device
> supports discard.
>
> Fix is to check if the device is missing before calling
> btrfs_trim_free_extents().
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>   fs/btrfs/extent-tree.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index d296483d148f..268ce58d4569 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -6019,6 +6019,9 @@ int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range)
>   	mutex_lock(&fs_info->fs_devices->device_list_mutex);
>   	devices = &fs_info->fs_devices->devices;
>   	list_for_each_entry(device, devices, dev_list) {
> +		if (test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state))
> +			continue;
> +
>   		ret = btrfs_trim_free_extents(device, &group_trimmed);
>   		if (ret) {
>   			dev_failed++;
>
Won't it be better to put the check in to btrfs_trim_free_extents()?

And maybe it's better to also check device->bdev, just in case we got
some strange de-sync between DEV_STATE_MISSING and NULL device->bdev
pointer.

Thanks,
Qu