Re: [PATCH v1 01/17] xfs: Add larp state XFS_DAS_CREATE_FORK

[Alli <allison.henderson@oracle.com>]

On Wed, 2022-06-15 at 11:09 +1000, Dave Chinner wrote:
> On Sat, Jun 11, 2022 at 02:41:44AM -0700, Allison Henderson wrote:
> > Recent parent pointer testing has exposed a bug in the underlying
> > larp state machine.  A replace operation may remove an old attr
> > before adding the new one, but if it is the only attr in the fork,
> > then the fork is removed.  This later causes a null pointer in
> > xfs_attr_try_sf_addname which expects the fork present.  This
> > patch adds an extra state to create the fork.
> 
> Hmmmm.
> 
> I thought I fixed those problems - in xfs_attr_sf_removename() there
> is this code:
> 
>         if (totsize == sizeof(xfs_attr_sf_hdr_t) && xfs_has_attr2(mp)
> &&
>             (dp->i_df.if_format != XFS_DINODE_FMT_BTREE) &&
>             !(args->op_flags & (XFS_DA_OP_ADDNAME |
> XFS_DA_OP_REPLACE))) {
>                 xfs_attr_fork_remove(dp, args->trans);
Hmm, ok, let me shuffle in some traces around there to see where things
fall off the rails

> 
> A replace operation will have XFS_DA_OP_REPLACE set, and so the
> final remove from a sf directory will not remove the attr fork in
> this case. There is equivalent checks in the leaf/node remove name
> paths to avoid removing the attr fork if the last attr is removed
> while the attr fork is in those formats.
> 
> How do you reproduce this issue?
> 

Sure, you can apply this kernel set or download it here:
https://github.com/allisonhenderson/xfs/tree/xfs_new_pptrs

Next you'll need this xfsprogs that has the neccassary updates to run
parent pointers
https://github.com/allisonhenderson/xfsprogs/tree/xfsprogs_new_pptrs


To reproduce the bug, you'll need to apply a quick patch on the kernel
side:
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index b86188b63897..f279afd43462 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -741,8 +741,8 @@ xfs_attr_set_iter(
 		fallthrough;
 	case XFS_DAS_SF_ADD:
 		if (!args->dp->i_afp) {
-			attr->xattri_dela_state = XFS_DAS_CREATE_FORK;
-			goto next_state;
+//			attr->xattri_dela_state = XFS_DAS_CREATE_FORK;
+//			goto next_state;
 		}
 		return xfs_attr_sf_addname(attr);
 	case XFS_DAS_LEAF_ADD:



Lastly, you'll need Catherines parent pointer tests that she sent out a
day or so ago.  Once you have that, just run the parent pointers test:
echo 1 > /sys/fs/xfs/debug/larp; ./check xfs/549

Dmesg below:


==================================================================
[  365.288788] BUG: KASAN: null-ptr-deref in
xfs_attr_try_sf_addname+0x2a/0xd0 [xfs]
[  365.289170] Read of size 1 at addr 000000000000002a by task
mount/23669

[  365.289182] CPU: 10 PID: 23669 Comm: mount Tainted:
G            E     5.18.0-rc2 #84
[  365.289196] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS
VirtualBox 12/01/2006
[  365.289202] Call Trace:
[  365.289206]  <TASK>
[  365.289211]  dump_stack_lvl+0x49/0x5f
[  365.289232]  print_report.cold+0x494/0x6b4
[  365.289242]  ? path_mount+0x641/0xfd0
[  365.289258]  ? __x64_sys_mount+0xca/0x110
[  365.289265]  ? do_syscall_64+0x3b/0x90
[  365.289274]  ? xfs_attr_try_sf_addname+0x2a/0xd0 [xfs]
[  365.289649]  kasan_report+0xa7/0x120
[  365.289660]  ? xfs_attr_try_sf_addname+0x2a/0xd0 [xfs]
[  365.290034]  __asan_load1+0x6a/0x70
[  365.290048]  xfs_attr_try_sf_addname+0x2a/0xd0 [xfs]
[  365.290423]  xfs_attr_set_iter+0x2f9/0x1510 [xfs]
[  365.290801]  ? xfs_init_attr_trans+0x130/0x130 [xfs]
[  365.291178]  ? kasan_poison+0x3c/0x50
[  365.291187]  ? kasan_unpoison+0x28/0x50
[  365.291197]  ? xfs_errortag_test+0x57/0x120 [xfs]
[  365.291592]  xfs_xattri_finish_update+0x66/0xd0 [xfs]
[  365.292008]  xfs_attr_finish_item+0x43/0x120 [xfs]
[  365.292410]  xfs_defer_finish_noroll+0x3c2/0xcc0 [xfs]
[  365.292804]  ? xfs_defer_cancel+0xc0/0xc0 [xfs]
[  365.293184]  ? kasan_quarantine_put+0x57/0x180
[  365.293196]  __xfs_trans_commit+0x333/0x610 [xfs]
[  365.293599]  ? xfs_trans_free_items+0x150/0x150 [xfs]
[  365.293995]  ? kvfree+0x28/0x30
[  365.294004]  ? kvfree+0x28/0x30
[  365.294017]  ? xfs_defer_ops_continue+0x1c5/0x280 [xfs]
[  365.294401]  xfs_trans_commit+0x10/0x20 [xfs]
[  365.294797]  xlog_finish_defer_ops+0x133/0x270 [xfs]
[  365.295203]  ? xlog_recover_free_trans+0x1c0/0x1c0 [xfs]
[  365.295609]  ? xfs_attr_finish_item+0x120/0x120 [xfs]
[  365.296036]  ? _raw_spin_lock+0x88/0xd7
[  365.296044]  ? _raw_spin_lock_irqsave+0xf0/0xf0
[  365.296054]  xlog_recover_process_intents+0x1f7/0x3e0 [xfs]
[  365.296469]  ? xlog_do_recover+0x290/0x290 [xfs]
[  365.296757]  ? __queue_delayed_work+0xdc/0x140
[  365.296766]  xlog_recover_finish+0x18/0x150 [xfs]
[  365.296949]  xfs_log_mount_finish+0x194/0x310 [xfs]
[  365.297132]  xfs_mountfs+0x957/0xeb0 [xfs]
[  365.297313]  ? xfs_mount_reset_sbqflags+0xa0/0xa0 [xfs]
[  365.297494]  ? xfs_filestream_put_ag+0x40/0x40 [xfs]
[  365.297674]  ? xfs_mru_cache_create+0x226/0x280 [xfs]
[  365.297855]  xfs_fs_fill_super+0x7f0/0xd20 [xfs]
[  365.298034]  get_tree_bdev+0x22e/0x360
[  365.298041]  ? xfs_fs_sync_fs+0x150/0x150 [xfs]
[  365.298223]  xfs_fs_get_tree+0x15/0x20 [xfs]
[  365.298401]  vfs_get_tree+0x4c/0x120
[  365.298408]  path_mount+0x641/0xfd0
[  365.298411]  ? putname+0x7c/0x90
[  365.298416]  ? finish_automount+0x2e0/0x2e0
[  365.298419]  ? kmem_cache_free+0x104/0x4d0
[  365.298422]  ? putname+0x7c/0x90
[  365.298426]  ? putname+0x7c/0x90
[  365.298430]  do_mount+0xd2/0xf0
[  365.298433]  ? path_mount+0xfd0/0xfd0
[  365.298436]  ? memdup_user+0x52/0x90
[  365.298440]  __x64_sys_mount+0xca/0x110
[  365.298444]  do_syscall_64+0x3b/0x90
[  365.298448]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[  365.298451] RIP: 0033:0x7f7e4e213cae
[  365.298455] Code: 48 8b 0d e5 c1 0c 00 f7 d8 64 89 01 48 83 c8 ff c3
66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 a5 00 00 00 0f
05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d b2 c1 0c 00 f7 d8 64 89 01 48
[  365.298459] RSP: 002b:00007ffee6811418 EFLAGS: 00000246 ORIG_RAX:
00000000000000a5
[  365.298467] RAX: ffffffffffffffda RBX: 00007f7e4e345204 RCX:
00007f7e4e213cae
[  365.298470] RDX: 000056006007db70 RSI: 000056006007dbb0 RDI:
000056006007db90
[  365.298472] RBP: 000056006007d960 R08: 0000000000000000 R09:
00007ffee6810190
[  365.298475] R10: 0000000000000000 R11: 0000000000000246 R12:
0000000000000000
[  365.298477] R13: 000056006007db90 R14: 000056006007db70 R15:
000056006007d960




> > Additionally the new state will be used by parent pointers which
> > need to add attributes to newly created inodes that do not yet
> > have a fork.
> 
> We already have the capability of doing that in xfs_init_new_inode()
> by passing in init_xattrs == true. So when we are creating a new
> inode with parent pointers enabled, we know that we are going to be
> creating an xattr on the inode and so we should always set
> init_xattrs in that case.
Hmm, ok.  I'll add some tracing around in there too, if I back out the
entire first patch, we crash out earlier in recovery path because no
state is set.  If we enter xfs_attri_item_recover with no fork, we end
up in the following switch:


        case
XFS_ATTRI_OP_FLAGS_REPLACE:                                        
                args->value = nv-
>value.i_addr;                                 
                args->valuelen = nv-
>value.i_len;                               
                args->total = xfs_attr_calc_size(args,
&local);                 
                if (xfs_inode_hasattr(args-
>dp))                                
                        attr->xattri_dela_state =
xfs_attr_init_replace_state(args);
                else                                                   
         
                        attr->xattri_dela_state =
xfs_attr_init_add_state(args);
                break;     

Which will leave the state unset if the fork is absent.
> 
> This should avoid the need for parent pointers to ever need to run
> an extra transaction to create the attr fork. Hence, AFAICT, this
> new state to handle attr fork creation shouldn't ever be needed for
> parent pointers....
> 
> What am I missing?
> 
I hope the description helped?  I'll do some more poking around too and
post back if I find anything else.

Allison

> Cheers,
> 
> Dave.