[PATCH 1/1] xfs: fix a null pointer dereference in xfs_bmap_extents_to_btree

[PATCH 1/1] xfs: fix a null pointer dereference in xfs_bmap_extents_to_btree

[Shan Hai]

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

Re: [PATCH 1/1] xfs: fix a null pointer dereference in xfs_bmap_extents_to_btree

[Shan Hai]

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;
>   }
>   
>   /*

Re: [PATCH 1/1] xfs: fix a null pointer dereference in xfs_bmap_extents_to_btree

[Darrick J. Wong]

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

Re: [PATCH 1/1] xfs: fix a null pointer dereference in xfs_bmap_extents_to_btree

[Shan Hai]

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

Re: [PATCH 1/1] xfs: fix a null pointer dereference in xfs_bmap_extents_to_btree

[Shan Hai]

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

Re: [PATCH 1/1] xfs: fix a null pointer dereference in xfs_bmap_extents_to_btree

[Darrick J. Wong]

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