Re: [PATCH 1/3] btrfs: init devices always

[Anand Jain <anand.jain@oracle.com>]

On 12/3/21 1:52 pm, Anand Jain wrote:
> On 12/3/21 12:23 am, Josef Bacik wrote:
>> Neal reported a panic trying to use -o rescue=all
>>
>> BUG: kernel NULL pointer dereference, address: 0000000000000030
>> PGD 0 P4D 0
>> Oops: 0000 [#1] SMP NOPTI
>> CPU: 0 PID: 696 Comm: mount Tainted: G        W         5.12.0-rc2+ #296
>> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-2.fc32 
>> 04/01/2014
>> RIP: 0010:btrfs_device_init_dev_stats+0x1d/0x200
>> RSP: 0018:ffffafaec1483bb8 EFLAGS: 00010286
>> RAX: 0000000000000000 RBX: ffff9a5715bcb298 RCX: 0000000000000070
>> RDX: ffff9a5703248000 RSI: ffff9a57052ea150 RDI: ffff9a5715bca400
>> RBP: ffff9a57052ea150 R08: 0000000000000070 R09: ffff9a57052ea150
>> R10: 000130faf0741c10 R11: 0000000000000000 R12: ffff9a5703700000
>> R13: 0000000000000000 R14: ffff9a5715bcb278 R15: ffff9a57052ea150
>> FS:  00007f600d122c40(0000) GS:ffff9a577bc00000(0000) 
>> knlGS:0000000000000000
>> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 0000000000000030 CR3: 0000000112a46005 CR4: 0000000000370ef0
>> Call Trace:
>>   ? btrfs_init_dev_stats+0x1f/0xf0
>>   ? kmem_cache_alloc+0xef/0x1f0
>>   btrfs_init_dev_stats+0x5f/0xf0
>>   open_ctree+0x10cb/0x1720
>>   btrfs_mount_root.cold+0x12/0xea
>>   legacy_get_tree+0x27/0x40
>>   vfs_get_tree+0x25/0xb0
>>   vfs_kern_mount.part.0+0x71/0xb0
>>   btrfs_mount+0x10d/0x380
>>   legacy_get_tree+0x27/0x40
>>   vfs_get_tree+0x25/0xb0
>>   path_mount+0x433/0xa00
>>   __x64_sys_mount+0xe3/0x120
>>   do_syscall_64+0x33/0x40
>>   entry_SYSCALL_64_after_hwframe+0x44/0xae
>>
>> This happens because when we call btrfs_init_dev_stats we do
>> device->fs_info->dev_root.  However device->fs_info isn't init'ed
>> because we were only calling btrfs_init_devices_late() if we properly
>> read the device root. 
> 
> 
>> However we don't actually need the device root to
>> init the devices, this function simply assigns the devices their
>> ->fs_info pointer properly, so this needs to be done unconditionally
>> always so that we can properly deref device->fs_info in rescue cases.


>   btrfs_device_init_dev_stats() calls btrfs_search_slot() leading
>   to btrfs_search_slot_get_root(), and does de-reference root (dev_root)
>   to get fs_info.

  Never mind. patch 2/3 handles it. Spoke too early.
  Maybe can reorder the patches during integration.
-Anand


> -------------
>   static int btrfs_device_init_dev_stats(struct btrfs_device *device,
>                                         struct btrfs_path *path)
> ::
>          ret = btrfs_search_slot(NULL, device->fs_info->dev_root, &key, 
> path, 0, 0);
> 
> 
> int btrfs_search_slot(struct btrfs_trans_handle *trans, struct 
> btrfs_root *root, ...)
> ::
>          b = btrfs_search_slot_get_root(root, p, write_lock_level);
> 
> 
> static struct extent_buffer *btrfs_search_slot_get_root(struct 
> btrfs_root *root, ...)
> {
>          struct btrfs_fs_info *fs_info = root->fs_info;
> --------------
> 
>   Can we allocate a dummy dev_root and set its dev_root::fs_info?



> Thanks, Anand
> 
> 
>> Reported-by: Neal Gompa <ngompa13@gmail.com>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> ---
>>   fs/btrfs/disk-io.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 41b718cfea40..63656bf23ff2 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -2387,8 +2387,8 @@ static int btrfs_read_roots(struct btrfs_fs_info 
>> *fs_info)
>>       } else {
>>           set_bit(BTRFS_ROOT_TRACK_DIRTY, &root->state);
>>           fs_info->dev_root = root;
>> -        btrfs_init_devices_late(fs_info);
>>       }
>> +    btrfs_init_devices_late(fs_info);
>>       /* If IGNOREDATACSUMS is set don't bother reading the csum root. */
>>       if (!btrfs_test_opt(fs_info, IGNOREDATACSUMS)) {
>>
>