On Thu, Apr 19, 2018 at 09:18:38AM +0800, Shan Hai wrote: > Hi Darrick, > > > On 2018年04月18日 10:41, Shan Hai wrote: > > > > > > On 2018年04月18日 10:24, Darrick J. Wong wrote: > > > On Wed, Apr 18, 2018 at 10:13:22AM +0800, Shan Hai wrote: > > > > > > > > On 2018年04月18日 10:09, Shan Hai wrote: > > > > > Fuzzing tool reports a write to null pointer error in the > > > > > xfs_bmap_extents_to_btree, fix it by bailing out on encountering > > > > > a null pointer. > > > > > > > > > > Signed-off-by: Shan Hai <shan.hai@xxxxxxxxxx> > > > > This one supposed to be applied on top of below: > > > > > > > > https://www.spinics.net/lists/linux-xfs/msg17254.html > > > > [PATCH] xfs: set format back to extents if > > > > xfs_bmap_extents_to_btree fails > > > > > > > > Thanks > > > > Shan Hai > > > > > --- > > > > > fs/xfs/libxfs/xfs_bmap.c | 24 ++++++++++++++++-------- > > > > > 1 file changed, 16 insertions(+), 8 deletions(-) > > > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > > > > > index 040eeda..90b743d 100644 > > > > > --- a/fs/xfs/libxfs/xfs_bmap.c > > > > > +++ b/fs/xfs/libxfs/xfs_bmap.c > > > > > @@ -724,19 +724,14 @@ xfs_bmap_extents_to_btree( > > > > > args.wasdel = wasdel; > > > > > *logflagsp = 0; > > > > > if ((error = xfs_alloc_vextent(&args))) { > > > > > - xfs_iroot_realloc(ip, -1, whichfork); > > > > > ASSERT(ifp->if_broot == NULL); > > > > > - XFS_IFORK_FMT_SET(ip, whichfork, XFS_DINODE_FMT_EXTENTS); > > > > > - xfs_btree_del_cursor(cur, XFS_BTREE_ERROR); > > > > > - return error; > > > > > + goto err1; > > > > > } > > > > > if (WARN_ON_ONCE(args.fsbno == NULLFSBLOCK)) { > > > > > - xfs_iroot_realloc(ip, -1, whichfork); > > > > > ASSERT(ifp->if_broot == NULL); > > > > > - XFS_IFORK_FMT_SET(ip, whichfork, XFS_DINODE_FMT_EXTENTS); > > > > > - xfs_btree_del_cursor(cur, XFS_BTREE_ERROR); > > > > > - return -ENOSPC; > > > > > + error = -ENOSPC; > > > > > + goto err1; > > > > > } > > > > > /* > > > > > * Allocation can't fail, the space was reserved. > > > > > @@ -748,6 +743,10 @@ xfs_bmap_extents_to_btree( > > > > > ip->i_d.di_nblocks++; > > > > > xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT, 1L); > > > > > abp = xfs_btree_get_bufl(mp, tp, args.fsbno, 0); > > > > > + if (!abp) { > > > When does this happen? We got args.fsbno from the allocator, so we're > > > not out of space. Or are you saying that something fuzzed the free > > > space btree, therefore the allocator gave back a nonsense block number > > > (say pointing past the end of the fs), and therefore the _get_bufl call > > > returned NULL? > > > > Seems the memory page allocation fails and the xfs_btree_get_bufl > > returns NULL, but I have no reliable reproducer, sorry. > > > > I have got another report of a possible NULL pointer deference in the code > as below, > this time from a static code analysis tool: > xfs_bmap_extents_to_btree > ->abp = xfs_btree_get_bufl > ->return xfs_trans_get_buf > ->return xfs_trans_get_buf_map > ->bp = xfs_buf_get_map > if (bp == NULL) { > return NULL; > } > > As you know that the corrupted block numbers from block allocator is checked > in the _xfs_buf_find, which returns NULL as well after a WARN_ON if the the > bno is > invalid. > > And the maximum allocation size in the xfs_buf_get_map is a single page, so > because of the PAGE_ALLOC_COSTLY_ORDER logic it's highly possible that the > process is killed the OOM killer instead of returning NULL from both > kmalloc/alloc_page. > > Even though this NULL pointer problem is rarely seen in the real workload > but it's > logically correct to check the pointer validity as this patch does in my > opinion. Er... sorry for the four month delay. Looks reasonable, will test... Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx> --D > Thanks > Shan Hai > > Thanks > > Shan Hai > > > If so, maybe we need to change that WARN_ON_ONCE thing above to: > > > > > > if (WARN_ON_ONCE(...) || !xfs_verify_fsbno(..., args.fsbno)) { > > > /* undo state and return */ > > > } > > > > > > --D > > > > > > > > + error = -ENOSPC; > > > > > + goto err2; > > > > > + } > > > > > /* > > > > > * Fill in the child block. > > > > > */ > > > > > @@ -787,6 +786,15 @@ xfs_bmap_extents_to_btree( > > > > > *curp = cur; > > > > > *logflagsp = XFS_ILOG_CORE | xfs_ilog_fbroot(whichfork); > > > > > return 0; > > > > > + > > > > > +err2: > > > > > + xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT, -1L); > > > > > +err1: > > > > > + xfs_iroot_realloc(ip, -1, whichfork); > > > > > + XFS_IFORK_FMT_SET(ip, whichfork, XFS_DINODE_FMT_EXTENTS); > > > > > + xfs_btree_del_cursor(cur, XFS_BTREE_ERROR); > > > > > + > > > > > + return error; > > > > > } > > > > > /* > > > > -- > > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > > > the body of a message to majordomo@xxxxxxxxxxxxxxx > > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > > the body of a message to majordomo@xxxxxxxxxxxxxxx > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > the body of a message to majordomo@xxxxxxxxxxxxxxx > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html
