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 <[email protected]>
---
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];
+
+ 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, 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 <[email protected]>
> ---
> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > > ---
> > > 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
> > >