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.
