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

2021-03-17 Thread David Sterba 
On Fri, Mar 12, 2021 at 01:57:32PM +0800, Anand Jain wrote:
> 
> 
> 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: 0030
> >> PGD 0 P4D 0
> >> Oops:  [#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:afaec1483bb8 EFLAGS: 00010286
> >> RAX:  RBX: 9a5715bcb298 RCX: 0070
> >> RDX: 9a5703248000 RSI: 9a57052ea150 RDI: 9a5715bca400
> >> RBP: 9a57052ea150 R08: 0070 R09: 9a57052ea150
> >> R10: 000130faf0741c10 R11:  R12: 9a570370
> >> R13:  R14: 9a5715bcb278 R15: 9a57052ea150
> >> FS:  7f600d122c40() GS:9a577bc0() 
> >> knlGS:
> >> CS:  0010 DS:  ES:  CR0: 80050033
> >> CR2: 0030 CR3: 000112a46005 CR4: 00370ef0
> >> 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.

I think swapping the order makes sense, though all the patches fix some
sort of crash, the number should be decreasing.

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

2021-03-11 Thread Anand Jain 

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: 0030
PGD 0 P4D 0
Oops:  [#1] SMP NOPTI
CPU: 0 PID: 696 Comm: mount Tainted: GW 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:afaec1483bb8 EFLAGS: 00010286
RAX:  RBX: 9a5715bcb298 RCX: 0070
RDX: 9a5703248000 RSI: 9a57052ea150 RDI: 9a5715bca400
RBP: 9a57052ea150 R08: 0070 R09: 9a57052ea150
R10: 000130faf0741c10 R11:  R12: 9a570370
R13:  R14: 9a5715bcb278 R15: 9a57052ea150
FS:  7f600d122c40() GS:9a577bc0() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: 0030 CR3: 000112a46005 CR4: 00370ef0
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.

Reported-by: Neal Gompa 
Signed-off-by: Josef Bacik 


Reviewed-by: Anand Jain

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

2021-03-11 Thread Anand Jain 




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: 0030
PGD 0 P4D 0
Oops:  [#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:afaec1483bb8 EFLAGS: 00010286
RAX:  RBX: 9a5715bcb298 RCX: 0070
RDX: 9a5703248000 RSI: 9a57052ea150 RDI: 9a5715bca400
RBP: 9a57052ea150 R08: 0070 R09: 9a57052ea150
R10: 000130faf0741c10 R11:  R12: 9a570370
R13:  R14: 9a5715bcb278 R15: 9a57052ea150
FS:  7f600d122c40() GS:9a577bc0() 
knlGS:

CS:  0010 DS:  ES:  CR0: 80050033
CR2: 0030 CR3: 000112a46005 CR4: 00370ef0
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 
Signed-off-by: Josef Bacik 
---
  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)) {

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

2021-03-11 Thread Anand Jain 

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: 0030
PGD 0 P4D 0
Oops:  [#1] SMP NOPTI
CPU: 0 PID: 696 Comm: mount Tainted: GW 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:afaec1483bb8 EFLAGS: 00010286
RAX:  RBX: 9a5715bcb298 RCX: 0070
RDX: 9a5703248000 RSI: 9a57052ea150 RDI: 9a5715bca400
RBP: 9a57052ea150 R08: 0070 R09: 9a57052ea150
R10: 000130faf0741c10 R11:  R12: 9a570370
R13:  R14: 9a5715bcb278 R15: 9a57052ea150
FS:  7f600d122c40() GS:9a577bc0() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: 0030 CR3: 000112a46005 CR4: 00370ef0
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.

-
 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 
Signed-off-by: Josef Bacik 
---
  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)) {