[PATCH] btrfs: check for missing device in btrfs_trim_fs

[PATCH] btrfs: check for missing device in btrfs_trim_fs

[Anand Jain]

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++;
-- 
2.31.1

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

[Qu Wenruo]

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

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

[Anand Jain]

On 05/07/2021 08:12, Qu Wenruo wrote:
> 
> 
> 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()?

  Fail early is better. Also there is only one caller for 
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.

  I can add NULL device->bdev check for now. It is a pending cleanup as 
a  whole to use one of it, and I am sure DEV_STATE_MISSING only will 
suffice.

  I will wait and see if there are any more comments before sending v2.

Thanks, Anand

> Thanks,
> Qu

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

[Qu Wenruo]

On 2021/7/5 上午11:21, Anand Jain wrote:
> 
> 
> On 05/07/2021 08:12, Qu Wenruo wrote:
>>
>>
>> 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()?
> 
>   Fail early is better. Also there is only one caller for 
> btrfs_trim_free_extents().

I mentioned this because there are already some other checks in 
btrfs_trim_free_extents(), thus it may be a good idea to put them there.
> 
>> 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.
> 
>   I can add NULL device->bdev check for now. It is a pending cleanup as 
> a  whole to use one of it, and I am sure DEV_STATE_MISSING only will 
> suffice.
> 
>   I will wait and see if there are any more comments before sending v2

Yeah, despite that, the patch looks pretty fine to me.

Thanks,
Qu
.
> 
> Thanks, Anand
> 
>> Thanks,
>> Qu
> 

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

[Filipe Manana]

On Sun, Jul 4, 2021 at 12:17 PM Anand Jain <anand.jain@oracle.com> 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

This step isn't needed, it's confusing to suggest the filesystem needs
to have some data in order to trigger the bug.

>
>   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>

Other than that, it looks good, thanks.

Reviewed-by: Filipe Manana <fdmanana@suse.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++;
> --
> 2.31.1
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

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

[Anand Jain]

On 5/7/21 4:43 pm, Filipe Manana wrote:
> On Sun, Jul 4, 2021 at 12:17 PM Anand Jain <anand.jain@oracle.com> 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
> 


> This step isn't needed, it's confusing to suggest the filesystem needs
> to have some data in order to trigger the bug.

That's right. I wonder if David could remove the above two lines while 
integrating? I am ok to send a reroll with this change if that's better.

The test case btrfs/242 contains a non-empty filesystem to test for some 
regression in the future.


> 
>>
>>    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>
> 
> Other than that, it looks good, thanks.
> 
> Reviewed-by: Filipe Manana <fdmanana@suse.com>


Thanks, Anand


> 
>> ---
>>   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++;
>> --
>> 2.31.1
>>
> 
> 

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

[David Sterba]

On Thu, Jul 08, 2021 at 08:11:30AM +0800, Anand Jain wrote:
> On 5/7/21 4:43 pm, Filipe Manana wrote:
> > On Sun, Jul 4, 2021 at 12:17 PM Anand Jain <anand.jain@oracle.com> 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
> > 
> 
> 
> > This step isn't needed, it's confusing to suggest the filesystem needs
> > to have some data in order to trigger the bug.
> 
> That's right. I wonder if David could remove the above two lines while 
> integrating? I am ok to send a reroll with this change if that's better.

Yeah such fixups are fine, no need to resend. I've applied thisd patch
before the cleanup of the devices variable so this one can be
backported, thanks.