| Message ID | 1582641477-4011-1-git-send-email-cai@lca.pw (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfs: fix an undefined behaviour in _da3_path_shift | expand |
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? > > 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@lca.pw> > --- > 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 >
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@lca.pw> > > --- > > 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 > >
I think we code do this a tad more cleaner, something like:
diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
index 875e04f82541..542a4edfcf54 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -1986,7 +1986,8 @@ xfs_da3_path_shift(
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--) {
+ for ( ; level >= 0; level--) {
+ blk = &path->blk[level];
xfs_da3_node_hdr_from_disk(dp->i_mount, &nodehdr,
blk->bp->b_addr);
On Tue, 2020-02-25 at 10:07 -0800, Christoph Hellwig wrote: > I think we code do this a tad more cleaner, something like: > > diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c > index 875e04f82541..542a4edfcf54 100644 > --- a/fs/xfs/libxfs/xfs_da_btree.c > +++ b/fs/xfs/libxfs/xfs_da_btree.c > @@ -1986,7 +1986,8 @@ xfs_da3_path_shift( > 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--) { > + for ( ; level >= 0; level--) { > + blk = &path->blk[level]; > xfs_da3_node_hdr_from_disk(dp->i_mount, &nodehdr, > blk->bp->b_addr); > Yes, indeed. I'll send a v2 until Darrick is still not convinced that "path->active == 1" could reach here?
On Tue, Feb 25, 2020 at 10:46:56AM -0500, Qian Cai wrote: > 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 ? Yes. The commit message ought to state that active == 1 is a valid state when we're trying to add an entry to a single dir leaf block and are trying to shift forward to see if there's a sibling block that would be a better place to put the new entry. This is to build confidence in future readers that we actually understood the circumstances of the UBSAN error and aren't just monkeypatching the code to shut up the automated checks. --D > 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@lca.pw> > > > --- > > > 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 > > >
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]; + + for (; level >= 0; blk--, level--) { xfs_da3_node_hdr_from_disk(dp->i_mount, &nodehdr, blk->bp->b_addr);
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]. 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@lca.pw> --- fs/xfs/libxfs/xfs_da_btree.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)