On Tue, 2020-02-25 at 07:28 -0800, Darrick J. Wong wrote:
> On Tue, Feb 25, 2020 at 09:37:57AM -0500, Qian Cai wrote:
> > state->path.active could be 1 in xfs_da3_node_lookup_int() and then in
> > xfs_da3_path_shift() could see state->path.blk[-1].
>
> Under what circumstancs can it be 1? Is this a longstanding bug in XFS?
> A corrupted filesystem? A deliberately corrupted filesystem?
in xfs_da3_node_lookup_int(),
for (blk = &state->path.blk[0], state->path.active = 1;
state->path.active <= XFS_DA_NODE_MAXDEPTH;
blk++, state->path.active++) {
<snip>
if (magic == XFS_ATTR_LEAF_MAGIC ||
magic == XFS_ATTR3_LEAF_MAGIC) {
blk->magic = XFS_ATTR_LEAF_MAGIC;
blk->hashval = xfs_attr_leaf_lasthash(blk->bp, NULL);
break;
}
if (magic == XFS_DIR2_LEAFN_MAGIC ||
magic == XFS_DIR3_LEAFN_MAGIC) {
blk->magic = XFS_DIR2_LEAFN_MAGIC;
blk->hashval = xfs_dir2_leaf_lasthash(args->dp,
blk->bp, NULL);
break;
Isn't that if the first iteration in the loop calls any of those "break", it
will have state->path.active = 1 ?
I suppose this is a long-standing bug that need UBSAN (no obvious harm could be
done later because it will bail out immediately in xfs_da3_path_shift()) and a
set of specific conditions to met to trigger.
>
> >
> > UBSAN: Undefined behaviour in fs/xfs/libxfs/xfs_da_btree.c:1989:14
> > index -1 is out of range for type 'xfs_da_state_blk_t [5]'
> > Call trace:
> > dump_backtrace+0x0/0x2c8
> > show_stack+0x20/0x2c
> > dump_stack+0xe8/0x150
> > __ubsan_handle_out_of_bounds+0xe4/0xfc
> > xfs_da3_path_shift+0x860/0x86c [xfs]
> > xfs_da3_node_lookup_int+0x7c8/0x934 [xfs]
> > xfs_dir2_node_addname+0x2c8/0xcd0 [xfs]
> > xfs_dir_createname+0x348/0x38c [xfs]
> > xfs_create+0x6b0/0x8b4 [xfs]
> > xfs_generic_create+0x12c/0x1f8 [xfs]
> > xfs_vn_mknod+0x3c/0x4c [xfs]
> > xfs_vn_create+0x34/0x44 [xfs]
> > do_last+0xd4c/0x10c8
> > path_openat+0xbc/0x2f4
> > do_filp_open+0x74/0xf4
> > do_sys_openat2+0x98/0x180
> > __arm64_sys_openat+0xf8/0x170
> > do_el0_svc+0x170/0x240
> > el0_sync_handler+0x150/0x250
> > el0_sync+0x164/0x180
> >
> > Signed-off-by: Qian Cai <cai@xxxxxx>
> > ---
> > fs/xfs/libxfs/xfs_da_btree.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
> > index 875e04f82541..0906b7748a3f 100644
> > --- a/fs/xfs/libxfs/xfs_da_btree.c
> > +++ b/fs/xfs/libxfs/xfs_da_btree.c
> > @@ -1986,7 +1986,11 @@ static inline int xfs_dabuf_nfsb(struct xfs_mount *mp, int whichfork)
> > ASSERT(path != NULL);
> > ASSERT((path->active > 0) && (path->active < XFS_DA_NODE_MAXDEPTH));
> > level = (path->active-1) - 1; /* skip bottom layer in path */
> > - for (blk = &path->blk[level]; level >= 0; blk--, level--) {
> > +
> > + if (level >= 0)
> > + blk = &path->blk[level];
>
> ...because if the reason is "corrupt metadata" then perhaps this should
> return -EFSCORRUPTED? But I don't know enough about the context to know
> the answer to that question.
>
> --D
>
> > +
> > + for (; level >= 0; blk--, level--) {
> > xfs_da3_node_hdr_from_disk(dp->i_mount, &nodehdr,
> > blk->bp->b_addr);
> >
> > --
> > 1.8.3.1
> >
