Re: [PATCH 4/5] btrfs: add helper to get fs_info from struct inode pointer

[David Sterba <dsterba@suse.cz>]

On Wed, Jan 31, 2024 at 07:13:43PM +1030, Qu Wenruo wrote:
> 
> 
> On 2024/1/31 17:53, David Sterba wrote:
> > On Mon, Jan 29, 2024 at 07:33:18PM +0100, David Sterba wrote:
> >> @@ -5211,7 +5211,7 @@ static struct btrfs_trans_handle *evict_refill_and_join(struct btrfs_root *root,
> >>
> >>   void btrfs_evict_inode(struct inode *inode)
> >>   {
> >> -	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> >> +	struct btrfs_fs_info *fs_info = inode_to_fs_info(inode);
> >
> > This leads to a crash in btrfs/232, happened twice:
> >
> >    BUG: KASAN: null-ptr-deref in btrfs_evict_inode+0xac/0x6b0 [btrfs]
> >    BUG: kernel NULL pointer dereference, address: 0000000000000208
> >    Read of size 8 at addr 0000000000000208 by task fsstress/21264
> >    #PF: supervisor read access in kernel mode
> >
> >    CPU: 3 PID: 21264 Comm: fsstress Not tainted 6.8.0-rc2-default+ #2288
> >    #PF: error_code(0x0000) - not-present page
> >    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.2-3-gd478f380-rebuilt.opensuse.org 04/01/2014
> >    PGD 683f8067
> >    Call Trace:
> >    P4D 683f8067
> >     <TASK>
> >     dump_stack_lvl+0x46/0x70
> >     kasan_report+0x123/0x150
> >     ? btrfs_evict_inode+0xac/0x6b0 [btrfs]
> >     ? btrfs_evict_inode+0xac/0x6b0 [btrfs]
> >     btrfs_evict_inode+0xac/0x6b0 [btrfs]
> >     ? local_clock_noinstr+0x11/0xc0
> >     ? btrfs_rmdir+0x380/0x380 [btrfs]
> >     ? reacquire_held_locks+0x280/0x280
> >     ? wake_up_var+0x120/0x120
> >     evict+0x17f/0x2d0
> >
> >     btrfs_create_common+0xe4/0x1c0 [btrfs]
> >     ? btrfs_tmpfile+0x2b0/0x2b0 [btrfs]
> >     ? init_special_inode+0xb9/0xe0
> >     vfs_mknod+0x25c/0x320
> >     do_mknodat+0x2fd/0x360
> >     ? kern_path_create+0x50/0x50
> >     ? getname_flags+0xb5/0x220
> >     __x64_sys_mknodat+0x5d/0x70
> >     do_syscall_64+0x6f/0x140
> >     entry_SYSCALL_64_after_hwframe+0x46/0x4e
> >
> > The new macro does BTRFS_I(inode)->root->fs_info while the old one uses
> > fs_info in the super block. From the context I don't see why a root
> > pointer would be NULL or how would anyone see that right away and not
> > introduce such crashes by using the helpers.
> 
> The function btrfs_evict_inode() only utilize BTRFS_I(inode)->root when
> the inode's i_nlink is not 0, and there are even explicit check on root.
> 
> So I guess BTRFS_I(inode)->root can be NULL, and the old code is already
> handing it.

Of course, now it's obvious.

> If you need, I can definitely dig deeper to give a more comprehensive
> call trace and analyze.

Not needed, thanks, I should have read the code.

> But it looks like if you want to grab fs_info, i_sb is way safer.

I'd like to minimize reading the fs_info from super block if the root is
available, for consistency and because sb::s_fs_info is void *.