* [PATCH 1/1] xfs: fix a null pointer dereference in xfs_bmap_extents_to_btree @ 2018-04-18 2:09 Shan Hai 2018-04-18 2:13 ` Shan Hai 0 siblings, 1 reply; 6+ messages in thread From: Shan Hai @ 2018-04-18 2:09 UTC (permalink / raw) To: linux-xfs 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@oracle.com> --- 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) { + 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; } /* -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] xfs: fix a null pointer dereference in xfs_bmap_extents_to_btree 2018-04-18 2:09 [PATCH 1/1] xfs: fix a null pointer dereference in xfs_bmap_extents_to_btree Shan Hai @ 2018-04-18 2:13 ` Shan Hai 2018-04-18 2:24 ` Darrick J. Wong 0 siblings, 1 reply; 6+ messages in thread From: Shan Hai @ 2018-04-18 2:13 UTC (permalink / raw) To: linux-xfs 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@oracle.com> 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) { > + 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; > } > > /* ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] xfs: fix a null pointer dereference in xfs_bmap_extents_to_btree 2018-04-18 2:13 ` Shan Hai @ 2018-04-18 2:24 ` Darrick J. Wong 2018-04-18 2:41 ` Shan Hai 0 siblings, 1 reply; 6+ messages in thread From: Darrick J. Wong @ 2018-04-18 2:24 UTC (permalink / raw) To: Shan Hai; +Cc: linux-xfs 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@oracle.com> > > 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? 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] xfs: fix a null pointer dereference in xfs_bmap_extents_to_btree 2018-04-18 2:24 ` Darrick J. Wong @ 2018-04-18 2:41 ` Shan Hai 2018-04-19 1:18 ` Shan Hai 0 siblings, 1 reply; 6+ messages in thread From: Shan Hai @ 2018-04-18 2:41 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs 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@oracle.com> >> 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. 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@vger.kernel.org >> 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] xfs: fix a null pointer dereference in xfs_bmap_extents_to_btree 2018-04-18 2:41 ` Shan Hai @ 2018-04-19 1:18 ` Shan Hai 2018-08-11 0:48 ` Darrick J. Wong 0 siblings, 1 reply; 6+ messages in thread From: Shan Hai @ 2018-04-19 1:18 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs 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@oracle.com> >>> 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. 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@vger.kernel.org >>> 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@vger.kernel.org >> 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] xfs: fix a null pointer dereference in xfs_bmap_extents_to_btree 2018-04-19 1:18 ` Shan Hai @ 2018-08-11 0:48 ` Darrick J. Wong 0 siblings, 0 replies; 6+ messages in thread From: Darrick J. Wong @ 2018-08-11 0:48 UTC (permalink / raw) To: Shan Hai; +Cc: linux-xfs 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@oracle.com> > > > > 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@oracle.com> --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@vger.kernel.org > > > > 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@vger.kernel.org > > > 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@vger.kernel.org > > 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-08-11 3:21 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-04-18 2:09 [PATCH 1/1] xfs: fix a null pointer dereference in xfs_bmap_extents_to_btree Shan Hai 2018-04-18 2:13 ` Shan Hai 2018-04-18 2:24 ` Darrick J. Wong 2018-04-18 2:41 ` Shan Hai 2018-04-19 1:18 ` Shan Hai 2018-08-11 0:48 ` Darrick J. Wong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).