* Re: [PATCH] xfs: do not call xfs_buf_hash_destroy on a NULL pag
2017-01-20 14:26 [PATCH] xfs: do not call xfs_buf_hash_destroy on a NULL pag Colin King
@ 2017-01-20 16:34 ` Darrick J. Wong
2017-01-20 19:26 ` Eric Sandeen
` (3 subsequent siblings)
4 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2017-01-20 16:34 UTC (permalink / raw)
To: Colin King; +Cc: linux-xfs, linux-kernel
On Fri, Jan 20, 2017 at 02:26:42PM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> If pag cannot be allocated, the current error exit path will trip
> a null pointer deference error when calling xfs_buf_hash_destroy
> with a null pag. Fix this by adding a new error exit lable and
> jumping to this, avoiding the hash destroy and unnecessary kmem_free
> on pag.
>
> Fixes CoverityScan CID#1397628 ("Dereference after null check")
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Applied, thanks.
--D
> ---
> fs/xfs/xfs_mount.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 9b9540d..4e66cd19 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -207,7 +207,7 @@ xfs_initialize_perag(
>
> pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL);
> if (!pag)
> - goto out_unwind;
> + goto out_unwind_pags;
> pag->pag_agno = index;
> pag->pag_mount = mp;
> spin_lock_init(&pag->pag_ici_lock);
> @@ -242,6 +242,7 @@ xfs_initialize_perag(
> out_unwind:
> xfs_buf_hash_destroy(pag);
> kmem_free(pag);
> +out_unwind_pags:
> for (; index > first_initialised; index--) {
> pag = radix_tree_delete(&mp->m_perag_tree, index);
> xfs_buf_hash_destroy(pag);
> --
> 2.10.2
>
> --
> 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] 21+ messages in thread* Re: [PATCH] xfs: do not call xfs_buf_hash_destroy on a NULL pag
2017-01-20 14:26 [PATCH] xfs: do not call xfs_buf_hash_destroy on a NULL pag Colin King
2017-01-20 16:34 ` Darrick J. Wong
@ 2017-01-20 19:26 ` Eric Sandeen
2017-01-20 20:47 ` Darrick J. Wong
2017-01-24 21:08 ` [PATCH v2] " Bill O'Donnell
` (2 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Eric Sandeen @ 2017-01-20 19:26 UTC (permalink / raw)
To: Colin King, Darrick J . Wong, linux-xfs, linux-kernel
On 1/20/17 8:26 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> If pag cannot be allocated, the current error exit path will trip
> a null pointer deference error when calling xfs_buf_hash_destroy
> with a null pag. Fix this by adding a new error exit lable and
> jumping to this, avoiding the hash destroy and unnecessary kmem_free
> on pag.
>
> Fixes CoverityScan CID#1397628 ("Dereference after null check")
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Hm, I think this leaves the code with issues.
> ---
> fs/xfs/xfs_mount.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 9b9540d..4e66cd19 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -207,7 +207,7 @@ xfs_initialize_perag(
>
> pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL);
> if (!pag)
> - goto out_unwind;
> + goto out_unwind_pags;
So let's say we got to index == 3 at the top of the loop, and
this fails.
We succeeded in initializing 0, 1, and 2, but 3 failed.
So we go to out_unwind_pags with index == 3...
> pag->pag_agno = index;
> pag->pag_mount = mp;
> spin_lock_init(&pag->pag_ici_lock);
> @@ -242,6 +242,7 @@ xfs_initialize_perag(
> out_unwind:
> xfs_buf_hash_destroy(pag);
> kmem_free(pag);
> +out_unwind_pags:
... where index == 3, and:
> for (; index > first_initialised; index--) {
> pag = radix_tree_delete(&mp->m_perag_tree, index);
this should fail, because it never got inserted, and...
> xfs_buf_hash_destroy(pag);
this still tries to destroy a NULL pag, no?
There also seems to be an existing issue w/the code where ag 0 is
never torn down in the error case, because first_initialized doesn't
stay set to 0:
if (!first_initialised)
first_initialised = index;
And we don't even tear down ag 1, because:
> for (; index > first_initialised; index--) {
> pag = radix_tree_delete(&mp->m_perag_tree, index);
when the loop reaches the first initialized AG, it stops.
So we seem to always leak at least 2 if we managed to get far enough
to initialize them.
-Eric
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH] xfs: do not call xfs_buf_hash_destroy on a NULL pag
2017-01-20 19:26 ` Eric Sandeen
@ 2017-01-20 20:47 ` Darrick J. Wong
2017-01-20 23:04 ` Colin Ian King
0 siblings, 1 reply; 21+ messages in thread
From: Darrick J. Wong @ 2017-01-20 20:47 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Colin King, linux-xfs, linux-kernel
On Fri, Jan 20, 2017 at 01:26:12PM -0600, Eric Sandeen wrote:
> On 1/20/17 8:26 AM, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> >
> > If pag cannot be allocated, the current error exit path will trip
> > a null pointer deference error when calling xfs_buf_hash_destroy
> > with a null pag. Fix this by adding a new error exit lable and
> > jumping to this, avoiding the hash destroy and unnecessary kmem_free
> > on pag.
> >
> > Fixes CoverityScan CID#1397628 ("Dereference after null check")
> >
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
>
> Hm, I think this leaves the code with issues.
>
> > ---
> > fs/xfs/xfs_mount.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > index 9b9540d..4e66cd19 100644
> > --- a/fs/xfs/xfs_mount.c
> > +++ b/fs/xfs/xfs_mount.c
> > @@ -207,7 +207,7 @@ xfs_initialize_perag(
> >
> > pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL);
> > if (!pag)
> > - goto out_unwind;
> > + goto out_unwind_pags;
>
> So let's say we got to index == 3 at the top of the loop, and
> this fails.
>
> We succeeded in initializing 0, 1, and 2, but 3 failed.
>
> So we go to out_unwind_pags with index == 3...
>
> > pag->pag_agno = index;
> > pag->pag_mount = mp;
> > spin_lock_init(&pag->pag_ici_lock);
> > @@ -242,6 +242,7 @@ xfs_initialize_perag(
> > out_unwind:
> > xfs_buf_hash_destroy(pag);
> > kmem_free(pag);
> > +out_unwind_pags:
>
> ... where index == 3, and:
>
> > for (; index > first_initialised; index--) {
> > pag = radix_tree_delete(&mp->m_perag_tree, index);
>
> this should fail, because it never got inserted, and...
>
> > xfs_buf_hash_destroy(pag);
>
> this still tries to destroy a NULL pag, no?
>
> There also seems to be an existing issue w/the code where ag 0 is
> never torn down in the error case, because first_initialized doesn't
> stay set to 0:
>
> if (!first_initialised)
> first_initialised = index;
>
> And we don't even tear down ag 1, because:
>
> > for (; index > first_initialised; index--) {
> > pag = radix_tree_delete(&mp->m_perag_tree, index);
>
> when the loop reaches the first initialized AG, it stops.
>
> So we seem to always leak at least 2 if we managed to get far enough
> to initialize them.
Ugh, yeah, the the whole error exit from that function is fubar...
Anyone want to clean this up?
--D
>
> -Eric
>
> >
> --
> 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] 21+ messages in thread* Re: [PATCH] xfs: do not call xfs_buf_hash_destroy on a NULL pag
2017-01-20 20:47 ` Darrick J. Wong
@ 2017-01-20 23:04 ` Colin Ian King
2017-01-24 15:04 ` Bill O'Donnell
0 siblings, 1 reply; 21+ messages in thread
From: Colin Ian King @ 2017-01-20 23:04 UTC (permalink / raw)
To: Darrick J. Wong, Eric Sandeen; +Cc: linux-xfs, linux-kernel
On 20/01/17 20:47, Darrick J. Wong wrote:
> On Fri, Jan 20, 2017 at 01:26:12PM -0600, Eric Sandeen wrote:
>> On 1/20/17 8:26 AM, Colin King wrote:
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> If pag cannot be allocated, the current error exit path will trip
>>> a null pointer deference error when calling xfs_buf_hash_destroy
>>> with a null pag. Fix this by adding a new error exit lable and
>>> jumping to this, avoiding the hash destroy and unnecessary kmem_free
>>> on pag.
>>>
>>> Fixes CoverityScan CID#1397628 ("Dereference after null check")
>>>
>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>
>> Hm, I think this leaves the code with issues.
>>
>>> ---
>>> fs/xfs/xfs_mount.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
>>> index 9b9540d..4e66cd19 100644
>>> --- a/fs/xfs/xfs_mount.c
>>> +++ b/fs/xfs/xfs_mount.c
>>> @@ -207,7 +207,7 @@ xfs_initialize_perag(
>>>
>>> pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL);
>>> if (!pag)
>>> - goto out_unwind;
>>> + goto out_unwind_pags;
>>
>> So let's say we got to index == 3 at the top of the loop, and
>> this fails.
>>
>> We succeeded in initializing 0, 1, and 2, but 3 failed.
>>
>> So we go to out_unwind_pags with index == 3...
>>
>>> pag->pag_agno = index;
>>> pag->pag_mount = mp;
>>> spin_lock_init(&pag->pag_ici_lock);
>>> @@ -242,6 +242,7 @@ xfs_initialize_perag(
>>> out_unwind:
>>> xfs_buf_hash_destroy(pag);
>>> kmem_free(pag);
>>> +out_unwind_pags:
>>
>> ... where index == 3, and:
>>
>>> for (; index > first_initialised; index--) {
>>> pag = radix_tree_delete(&mp->m_perag_tree, index);
>>
>> this should fail, because it never got inserted, and...
>>
>>> xfs_buf_hash_destroy(pag);
>>
>> this still tries to destroy a NULL pag, no?
>>
>> There also seems to be an existing issue w/the code where ag 0 is
>> never torn down in the error case, because first_initialized doesn't
>> stay set to 0:
>>
>> if (!first_initialised)
>> first_initialised = index;
>>
>> And we don't even tear down ag 1, because:
>>
>>> for (; index > first_initialised; index--) {
>>> pag = radix_tree_delete(&mp->m_perag_tree, index);
>>
>> when the loop reaches the first initialized AG, it stops.
>>
>> So we seem to always leak at least 2 if we managed to get far enough
>> to initialize them.
>
> Ugh, yeah, the the whole error exit from that function is fubar...
> Anyone want to clean this up?
I may step back on this if somebody else wants to fix this up properly.
>
> --D
>
>>
>> -Eric
>>
>>>
>> --
>> 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] 21+ messages in thread* Re: [PATCH] xfs: do not call xfs_buf_hash_destroy on a NULL pag
2017-01-20 23:04 ` Colin Ian King
@ 2017-01-24 15:04 ` Bill O'Donnell
2017-01-24 18:34 ` Darrick J. Wong
0 siblings, 1 reply; 21+ messages in thread
From: Bill O'Donnell @ 2017-01-24 15:04 UTC (permalink / raw)
To: Colin Ian King; +Cc: Darrick J. Wong, Eric Sandeen, linux-xfs, linux-kernel
On Fri, Jan 20, 2017 at 11:04:42PM +0000, Colin Ian King wrote:
> On 20/01/17 20:47, Darrick J. Wong wrote:
> > On Fri, Jan 20, 2017 at 01:26:12PM -0600, Eric Sandeen wrote:
> >> On 1/20/17 8:26 AM, Colin King wrote:
> >>> From: Colin Ian King <colin.king@canonical.com>
> >>>
> >>> If pag cannot be allocated, the current error exit path will trip
> >>> a null pointer deference error when calling xfs_buf_hash_destroy
> >>> with a null pag. Fix this by adding a new error exit lable and
> >>> jumping to this, avoiding the hash destroy and unnecessary kmem_free
> >>> on pag.
> >>>
> >>> Fixes CoverityScan CID#1397628 ("Dereference after null check")
> >>>
> >>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> >>
> >> Hm, I think this leaves the code with issues.
> >>
> >>> ---
> >>> fs/xfs/xfs_mount.c | 3 ++-
> >>> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> >>> index 9b9540d..4e66cd19 100644
> >>> --- a/fs/xfs/xfs_mount.c
> >>> +++ b/fs/xfs/xfs_mount.c
> >>> @@ -207,7 +207,7 @@ xfs_initialize_perag(
> >>>
> >>> pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL);
> >>> if (!pag)
> >>> - goto out_unwind;
> >>> + goto out_unwind_pags;
> >>
> >> So let's say we got to index == 3 at the top of the loop, and
> >> this fails.
> >>
> >> We succeeded in initializing 0, 1, and 2, but 3 failed.
> >>
> >> So we go to out_unwind_pags with index == 3...
> >>
> >>> pag->pag_agno = index;
> >>> pag->pag_mount = mp;
> >>> spin_lock_init(&pag->pag_ici_lock);
> >>> @@ -242,6 +242,7 @@ xfs_initialize_perag(
> >>> out_unwind:
> >>> xfs_buf_hash_destroy(pag);
> >>> kmem_free(pag);
> >>> +out_unwind_pags:
> >>
> >> ... where index == 3, and:
> >>
> >>> for (; index > first_initialised; index--) {
> >>> pag = radix_tree_delete(&mp->m_perag_tree, index);
> >>
> >> this should fail, because it never got inserted, and...
> >>
> >>> xfs_buf_hash_destroy(pag);
> >>
> >> this still tries to destroy a NULL pag, no?
> >>
> >> There also seems to be an existing issue w/the code where ag 0 is
> >> never torn down in the error case, because first_initialized doesn't
> >> stay set to 0:
> >>
> >> if (!first_initialised)
> >> first_initialised = index;
> >>
> >> And we don't even tear down ag 1, because:
> >>
> >>> for (; index > first_initialised; index--) {
> >>> pag = radix_tree_delete(&mp->m_perag_tree, index);
> >>
> >> when the loop reaches the first initialized AG, it stops.
> >>
> >> So we seem to always leak at least 2 if we managed to get far enough
> >> to initialize them.
> >
> > Ugh, yeah, the the whole error exit from that function is fubar...
> > Anyone want to clean this up?
>
> I may step back on this if somebody else wants to fix this up properly.
I'll take it.
-Bill
>
> >
> > --D
> >
> >>
> >> -Eric
> >>
> >>>
> >> --
> >> 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] 21+ messages in thread* Re: [PATCH] xfs: do not call xfs_buf_hash_destroy on a NULL pag
2017-01-24 15:04 ` Bill O'Donnell
@ 2017-01-24 18:34 ` Darrick J. Wong
0 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2017-01-24 18:34 UTC (permalink / raw)
To: Bill O'Donnell; +Cc: Colin Ian King, Eric Sandeen, linux-xfs, linux-kernel
On Tue, Jan 24, 2017 at 09:04:57AM -0600, Bill O'Donnell wrote:
> On Fri, Jan 20, 2017 at 11:04:42PM +0000, Colin Ian King wrote:
> > On 20/01/17 20:47, Darrick J. Wong wrote:
> > > On Fri, Jan 20, 2017 at 01:26:12PM -0600, Eric Sandeen wrote:
> > >> On 1/20/17 8:26 AM, Colin King wrote:
> > >>> From: Colin Ian King <colin.king@canonical.com>
> > >>>
> > >>> If pag cannot be allocated, the current error exit path will trip
> > >>> a null pointer deference error when calling xfs_buf_hash_destroy
> > >>> with a null pag. Fix this by adding a new error exit lable and
> > >>> jumping to this, avoiding the hash destroy and unnecessary kmem_free
> > >>> on pag.
> > >>>
> > >>> Fixes CoverityScan CID#1397628 ("Dereference after null check")
> > >>>
> > >>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > >>
> > >> Hm, I think this leaves the code with issues.
> > >>
> > >>> ---
> > >>> fs/xfs/xfs_mount.c | 3 ++-
> > >>> 1 file changed, 2 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > >>> index 9b9540d..4e66cd19 100644
> > >>> --- a/fs/xfs/xfs_mount.c
> > >>> +++ b/fs/xfs/xfs_mount.c
> > >>> @@ -207,7 +207,7 @@ xfs_initialize_perag(
> > >>>
> > >>> pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL);
> > >>> if (!pag)
> > >>> - goto out_unwind;
> > >>> + goto out_unwind_pags;
> > >>
> > >> So let's say we got to index == 3 at the top of the loop, and
> > >> this fails.
> > >>
> > >> We succeeded in initializing 0, 1, and 2, but 3 failed.
> > >>
> > >> So we go to out_unwind_pags with index == 3...
> > >>
> > >>> pag->pag_agno = index;
> > >>> pag->pag_mount = mp;
> > >>> spin_lock_init(&pag->pag_ici_lock);
> > >>> @@ -242,6 +242,7 @@ xfs_initialize_perag(
> > >>> out_unwind:
> > >>> xfs_buf_hash_destroy(pag);
> > >>> kmem_free(pag);
> > >>> +out_unwind_pags:
> > >>
> > >> ... where index == 3, and:
> > >>
> > >>> for (; index > first_initialised; index--) {
> > >>> pag = radix_tree_delete(&mp->m_perag_tree, index);
> > >>
> > >> this should fail, because it never got inserted, and...
> > >>
> > >>> xfs_buf_hash_destroy(pag);
> > >>
> > >> this still tries to destroy a NULL pag, no?
> > >>
> > >> There also seems to be an existing issue w/the code where ag 0 is
> > >> never torn down in the error case, because first_initialized doesn't
> > >> stay set to 0:
> > >>
> > >> if (!first_initialised)
> > >> first_initialised = index;
> > >>
> > >> And we don't even tear down ag 1, because:
> > >>
> > >>> for (; index > first_initialised; index--) {
> > >>> pag = radix_tree_delete(&mp->m_perag_tree, index);
> > >>
> > >> when the loop reaches the first initialized AG, it stops.
> > >>
> > >> So we seem to always leak at least 2 if we managed to get far enough
> > >> to initialize them.
> > >
> > > Ugh, yeah, the the whole error exit from that function is fubar...
> > > Anyone want to clean this up?
> >
> > I may step back on this if somebody else wants to fix this up properly.
>
> I'll take it.
Acknowledged. :)
--D
> -Bill
>
>
> >
> > >
> > > --D
> > >
> > >>
> > >> -Eric
> > >>
> > >>>
> > >> --
> > >> 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] 21+ messages in thread
* [PATCH v2] xfs: do not call xfs_buf_hash_destroy on a NULL pag
2017-01-20 14:26 [PATCH] xfs: do not call xfs_buf_hash_destroy on a NULL pag Colin King
2017-01-20 16:34 ` Darrick J. Wong
2017-01-20 19:26 ` Eric Sandeen
@ 2017-01-24 21:08 ` Bill O'Donnell
2017-01-24 21:21 ` Darrick J. Wong
2017-01-25 19:04 ` [PATCH v3] " Bill O'Donnell
2017-01-28 19:19 ` [PATCH] xfs: correct null checks and error processing in xfs_initialize_perag Bill O'Donnell
4 siblings, 1 reply; 21+ messages in thread
From: Bill O'Donnell @ 2017-01-24 21:08 UTC (permalink / raw)
To: linux-xfs
From: Colin Ian King <colin.king@canonical.com>
If pag cannot be allocated, the current error exit path will trip
a null pointer deference error when calling xfs_buf_hash_destroy
with a null pag. Fix this by adding a new error exit lable and
jumping to this, avoiding the hash destroy and unnecessary kmem_free
on pag.
Fixes CoverityScan CID#1397628 ("Dereference after null check")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
------------
v2: correct error exit in xfs_initialize_perag() to properly unwind
pags if error encountered.
Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
fs/xfs/xfs_mount.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 9b9540d..67bb6f2 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -187,9 +187,10 @@ xfs_initialize_perag(
xfs_agnumber_t *maxagi)
{
xfs_agnumber_t index;
- xfs_agnumber_t first_initialised = 0;
+ xfs_agnumber_t last_valid_agindex = 0;
xfs_perag_t *pag;
int error = -ENOMEM;
+ int i;
/*
* Walk the current per-ag tree so we don't try to initialise AGs
@@ -197,17 +198,16 @@ xfs_initialize_perag(
* AGs we don't find ready for initialisation.
*/
for (index = 0; index < agcount; index++) {
+ last_valid_agindex = index;
pag = xfs_perag_get(mp, index);
if (pag) {
xfs_perag_put(pag);
continue;
}
- if (!first_initialised)
- first_initialised = index;
pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL);
if (!pag)
- goto out_unwind;
+ goto out_unwind_pags;
pag->pag_agno = index;
pag->pag_mount = mp;
spin_lock_init(&pag->pag_ici_lock);
@@ -242,8 +242,11 @@ xfs_initialize_perag(
out_unwind:
xfs_buf_hash_destroy(pag);
kmem_free(pag);
- for (; index > first_initialised; index--) {
- pag = radix_tree_delete(&mp->m_perag_tree, index);
+out_unwind_pags:
+ for (i = last_valid_agindex; i >= 0; i--) {
+ pag = radix_tree_delete(&mp->m_perag_tree, (xfs_agnumber_t)i);
+ if (!pag)
+ break;
xfs_buf_hash_destroy(pag);
kmem_free(pag);
}
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2] xfs: do not call xfs_buf_hash_destroy on a NULL pag
2017-01-24 21:08 ` [PATCH v2] " Bill O'Donnell
@ 2017-01-24 21:21 ` Darrick J. Wong
2017-01-24 21:28 ` Bill O'Donnell
0 siblings, 1 reply; 21+ messages in thread
From: Darrick J. Wong @ 2017-01-24 21:21 UTC (permalink / raw)
To: Bill O'Donnell; +Cc: linux-xfs
You might want to cc the maintainer. ;)
On Tue, Jan 24, 2017 at 03:08:48PM -0600, Bill O'Donnell wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> If pag cannot be allocated, the current error exit path will trip
> a null pointer deference error when calling xfs_buf_hash_destroy
> with a null pag. Fix this by adding a new error exit lable and
> jumping to this, avoiding the hash destroy and unnecessary kmem_free
> on pag.
>
> Fixes CoverityScan CID#1397628 ("Dereference after null check")
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>
> ------------
> v2: correct error exit in xfs_initialize_perag() to properly unwind
> pags if error encountered.
>
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
> fs/xfs/xfs_mount.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 9b9540d..67bb6f2 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -187,9 +187,10 @@ xfs_initialize_perag(
> xfs_agnumber_t *maxagi)
> {
> xfs_agnumber_t index;
> - xfs_agnumber_t first_initialised = 0;
> + xfs_agnumber_t last_valid_agindex = 0;
> xfs_perag_t *pag;
> int error = -ENOMEM;
> + int i;
>
> /*
> * Walk the current per-ag tree so we don't try to initialise AGs
> @@ -197,17 +198,16 @@ xfs_initialize_perag(
> * AGs we don't find ready for initialisation.
> */
> for (index = 0; index < agcount; index++) {
> + last_valid_agindex = index;
> pag = xfs_perag_get(mp, index);
> if (pag) {
> xfs_perag_put(pag);
> continue;
> }
> - if (!first_initialised)
> - first_initialised = index;
>
> pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL);
> if (!pag)
> - goto out_unwind;
> + goto out_unwind_pags;
> pag->pag_agno = index;
> pag->pag_mount = mp;
> spin_lock_init(&pag->pag_ici_lock);
> @@ -242,8 +242,11 @@ xfs_initialize_perag(
> out_unwind:
> xfs_buf_hash_destroy(pag);
> kmem_free(pag);
> - for (; index > first_initialised; index--) {
> - pag = radix_tree_delete(&mp->m_perag_tree, index);
> +out_unwind_pags:
> + for (i = last_valid_agindex; i >= 0; i--) {
xfs_initialize_perag can be called towards the end of a growfs operation
to initialize the perag structures for the new AGs. If the
initialization fails, we want to roll back to the number of AGs we had
before, which means that we cannot delete the perag structures for the
not-recently-created AGs. That (I think) was what first_initialised was
trying to do (though it does't do that correctly in the case that we're
mounting).
Also, you could start the loop with "index - 1", right?
--D
> + pag = radix_tree_delete(&mp->m_perag_tree, (xfs_agnumber_t)i);
> + if (!pag)
> + break;
> xfs_buf_hash_destroy(pag);
> kmem_free(pag);
> }
> --
> 2.9.3
>
> --
> 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] 21+ messages in thread* Re: [PATCH v2] xfs: do not call xfs_buf_hash_destroy on a NULL pag
2017-01-24 21:21 ` Darrick J. Wong
@ 2017-01-24 21:28 ` Bill O'Donnell
0 siblings, 0 replies; 21+ messages in thread
From: Bill O'Donnell @ 2017-01-24 21:28 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Tue, Jan 24, 2017 at 01:21:55PM -0800, Darrick J. Wong wrote:
> You might want to cc the maintainer. ;)
will do on v3 ;)
>
> On Tue, Jan 24, 2017 at 03:08:48PM -0600, Bill O'Donnell wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> >
> > If pag cannot be allocated, the current error exit path will trip
> > a null pointer deference error when calling xfs_buf_hash_destroy
> > with a null pag. Fix this by adding a new error exit lable and
> > jumping to this, avoiding the hash destroy and unnecessary kmem_free
> > on pag.
> >
> > Fixes CoverityScan CID#1397628 ("Dereference after null check")
> >
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> >
> > ------------
> > v2: correct error exit in xfs_initialize_perag() to properly unwind
> > pags if error encountered.
> >
> > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > ---
> > fs/xfs/xfs_mount.c | 15 +++++++++------
> > 1 file changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > index 9b9540d..67bb6f2 100644
> > --- a/fs/xfs/xfs_mount.c
> > +++ b/fs/xfs/xfs_mount.c
> > @@ -187,9 +187,10 @@ xfs_initialize_perag(
> > xfs_agnumber_t *maxagi)
> > {
> > xfs_agnumber_t index;
> > - xfs_agnumber_t first_initialised = 0;
> > + xfs_agnumber_t last_valid_agindex = 0;
> > xfs_perag_t *pag;
> > int error = -ENOMEM;
> > + int i;
> >
> > /*
> > * Walk the current per-ag tree so we don't try to initialise AGs
> > @@ -197,17 +198,16 @@ xfs_initialize_perag(
> > * AGs we don't find ready for initialisation.
> > */
> > for (index = 0; index < agcount; index++) {
> > + last_valid_agindex = index;
> > pag = xfs_perag_get(mp, index);
> > if (pag) {
> > xfs_perag_put(pag);
> > continue;
> > }
> > - if (!first_initialised)
> > - first_initialised = index;
> >
> > pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL);
> > if (!pag)
> > - goto out_unwind;
> > + goto out_unwind_pags;
> > pag->pag_agno = index;
> > pag->pag_mount = mp;
> > spin_lock_init(&pag->pag_ici_lock);
> > @@ -242,8 +242,11 @@ xfs_initialize_perag(
> > out_unwind:
> > xfs_buf_hash_destroy(pag);
> > kmem_free(pag);
> > - for (; index > first_initialised; index--) {
> > - pag = radix_tree_delete(&mp->m_perag_tree, index);
> > +out_unwind_pags:
> > + for (i = last_valid_agindex; i >= 0; i--) {
>
> xfs_initialize_perag can be called towards the end of a growfs operation
> to initialize the perag structures for the new AGs. If the
> initialization fails, we want to roll back to the number of AGs we had
> before, which means that we cannot delete the perag structures for the
> not-recently-created AGs. That (I think) was what first_initialised was
> trying to do (though it does't do that correctly in the case that we're
> mounting).
Yep, I need to reinstate first_initialized and use it.
Thanks-
Bill
>
> Also, you could start the loop with "index - 1", right?
>
> --D
>
> > + pag = radix_tree_delete(&mp->m_perag_tree, (xfs_agnumber_t)i);
> > + if (!pag)
> > + break;
> > xfs_buf_hash_destroy(pag);
> > kmem_free(pag);
> > }
> > --
> > 2.9.3
> >
> > --
> > 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] 21+ messages in thread
* [PATCH v3] xfs: do not call xfs_buf_hash_destroy on a NULL pag
2017-01-20 14:26 [PATCH] xfs: do not call xfs_buf_hash_destroy on a NULL pag Colin King
` (2 preceding siblings ...)
2017-01-24 21:08 ` [PATCH v2] " Bill O'Donnell
@ 2017-01-25 19:04 ` Bill O'Donnell
2017-01-26 17:58 ` Eric Sandeen
2017-01-28 19:19 ` [PATCH] xfs: correct null checks and error processing in xfs_initialize_perag Bill O'Donnell
4 siblings, 1 reply; 21+ messages in thread
From: Bill O'Donnell @ 2017-01-25 19:04 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong
From: Colin Ian King <colin.king@canonical.com>
If pag cannot be allocated, the current error exit path will trip
a null pointer deference error when calling xfs_buf_hash_destroy
with a null pag. Fix this by adding a new error exit lable and
jumping to this, avoiding the hash destroy and unnecessary kmem_free
on pag.
Fixes CoverityScan CID#1397628 ("Dereference after null check")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
------------
v2: correct error exit in xfs_initialize_perag() to properly unwind
pags if error encountered.
v3: correction to error case: ensure previous valid pags not torn
down and only new initialized pags are torn down.
Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
fs/xfs/xfs_mount.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 9b9540d..afc49ac 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -188,8 +188,10 @@ xfs_initialize_perag(
{
xfs_agnumber_t index;
xfs_agnumber_t first_initialised = 0;
+ xfs_agnumber_t next_agindex = 0;
xfs_perag_t *pag;
int error = -ENOMEM;
+ int i;
/*
* Walk the current per-ag tree so we don't try to initialise AGs
@@ -200,14 +202,15 @@ xfs_initialize_perag(
pag = xfs_perag_get(mp, index);
if (pag) {
xfs_perag_put(pag);
+ next_agindex = index + 1;
continue;
}
- if (!first_initialised)
+ if (!first_initialised && (next_agindex > 0))
first_initialised = index;
pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL);
if (!pag)
- goto out_unwind;
+ goto out_unwind_pags;
pag->pag_agno = index;
pag->pag_mount = mp;
spin_lock_init(&pag->pag_ici_lock);
@@ -242,8 +245,11 @@ xfs_initialize_perag(
out_unwind:
xfs_buf_hash_destroy(pag);
kmem_free(pag);
- for (; index > first_initialised; index--) {
- pag = radix_tree_delete(&mp->m_perag_tree, index);
+out_unwind_pags:
+ for (i = index; i >= first_initialised; i--) {
+ pag = radix_tree_delete(&mp->m_perag_tree, (xfs_agnumber_t)i);
+ if (!pag)
+ continue;
xfs_buf_hash_destroy(pag);
kmem_free(pag);
}
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3] xfs: do not call xfs_buf_hash_destroy on a NULL pag
2017-01-25 19:04 ` [PATCH v3] " Bill O'Donnell
@ 2017-01-26 17:58 ` Eric Sandeen
2017-01-26 18:55 ` Bill O'Donnell
0 siblings, 1 reply; 21+ messages in thread
From: Eric Sandeen @ 2017-01-26 17:58 UTC (permalink / raw)
To: Bill O'Donnell, linux-xfs; +Cc: darrick.wong
On 1/25/17 1:04 PM, Bill O'Donnell wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> If pag cannot be allocated, the current error exit path will trip
> a null pointer deference error when calling xfs_buf_hash_destroy
> with a null pag. Fix this by adding a new error exit lable and
> jumping to this, avoiding the hash destroy and unnecessary kmem_free
> on pag.
There are enough other changes that it probably needs a more accurate
commit log and subject, and TBH probably switch Colin to reported-by.
> Fixes CoverityScan CID#1397628 ("Dereference after null check")
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>
> ------------
> v2: correct error exit in xfs_initialize_perag() to properly unwind
> pags if error encountered.
>
> v3: correction to error case: ensure previous valid pags not torn
> down and only new initialized pags are torn down.
patch changelog goes under the "---" below, it shouldn't be part of the
commitlog, FYI.
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
> fs/xfs/xfs_mount.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 9b9540d..afc49ac 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -188,8 +188,10 @@ xfs_initialize_perag(
> {
> xfs_agnumber_t index;
> xfs_agnumber_t first_initialised = 0;
> + xfs_agnumber_t next_agindex = 0;
> xfs_perag_t *pag;
> int error = -ENOMEM;
> + int i;
>
> /*
> * Walk the current per-ag tree so we don't try to initialise AGs
> @@ -200,14 +202,15 @@ xfs_initialize_perag(
> pag = xfs_perag_get(mp, index);
> if (pag) {
> xfs_perag_put(pag);
> + next_agindex = index + 1;
a little odd to keep incrementing next_agindex when we really
only care that it's nonzero, right?
> continue;
> }
> - if (!first_initialised)
> + if (!first_initialised && (next_agindex > 0))
> first_initialised = index;
ok, so we deem this index as first_initialized, but:
> pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL);
> if (!pag)
> - goto out_unwind;
> + goto out_unwind_pags;
...then it could fail. Which is why the unwind loop at the end still
needs to check for (this) null pag, and skip if so. In fact if you
ever get to that loop, I think it's guaranteed that the first time
through will find the null pag for this index and skip it.
It'd be a little nicer to move the first_initialized assignment down,
after it's actually /been/ allocated and initialized.
This patch also leaves this in place:
if (xfs_buf_hash_init(pag))
goto out_unwind;
...
out_unwind:
xfs_buf_hash_destroy(pag);
kmem_free(pag);
so isn't this doing buf_hash_destroy on something that has failed to
init in the first place? I have no idea if that's safe, but best to
just be clean about it.
In the big picture, there are 3 things happening in this loop that may
need to be unwound for this and/or prior pags on an error:
1) pag memory allocation
2) xfs_buf_hash_init
3) radix_tree_insert
For any given iteration through the loop, any of the above which succeed
must be unwound for /this/ pag, and then all prior initialized pags must
be unwound. So I'd expect something like this, though please check the
details and adjust labels to your preference. :)
bool first_init_done = false;
for (index = 0; index < agcount; index++) {
if (pag exists)
continue;
allocate(pag)
if (failure)
goto unwind_prior_pags;
xfs_buf_hash_init(pag)
if (failure)
goto free_pag;
radix_tree_insert(pag)
if (failure)
goto hash_destroy;
/* this pag is fully initialized now */
if (!first_init_done) {
first_initialized = index;
first_init_done = true;
}
}
/* unwind this pag on init failures */
hash_destroy:
xfs_buf_hash_destroy(pag);
free_pag:
kmem_free(pag);
unwind_prior_pags:
/* back up to previous fully initialized pag and unwind it+prior */
index--;
for (; index >= first_initialised; index--)
pag = radix_tree_delete(index)
xfs_buf_hash_destroy(pag)
kmem_free(pag)
> pag->pag_agno = index;
> pag->pag_mount = mp;
> spin_lock_init(&pag->pag_ici_lock);
> @@ -242,8 +245,11 @@ xfs_initialize_perag(
> out_unwind:
> xfs_buf_hash_destroy(pag);
> kmem_free(pag);
> - for (; index > first_initialised; index--) {
> - pag = radix_tree_delete(&mp->m_perag_tree, index);
> +out_unwind_pags:
> + for (i = index; i >= first_initialised; i--) {
> + pag = radix_tree_delete(&mp->m_perag_tree, (xfs_agnumber_t)i);
(don't make i an int if it's really supposed to be an xfs_agnumber_t,
which is unsigned)
-Eric
> + if (!pag)
> + continue;
> xfs_buf_hash_destroy(pag);
> kmem_free(pag);
> }
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3] xfs: do not call xfs_buf_hash_destroy on a NULL pag
2017-01-26 17:58 ` Eric Sandeen
@ 2017-01-26 18:55 ` Bill O'Donnell
2017-01-26 20:27 ` Eric Sandeen
0 siblings, 1 reply; 21+ messages in thread
From: Bill O'Donnell @ 2017-01-26 18:55 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs, darrick.wong
On Thu, Jan 26, 2017 at 11:58:37AM -0600, Eric Sandeen wrote:
> On 1/25/17 1:04 PM, Bill O'Donnell wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> >
> > If pag cannot be allocated, the current error exit path will trip
> > a null pointer deference error when calling xfs_buf_hash_destroy
> > with a null pag. Fix this by adding a new error exit lable and
> > jumping to this, avoiding the hash destroy and unnecessary kmem_free
> > on pag.
>
> There are enough other changes that it probably needs a more accurate
> commit log and subject, and TBH probably switch Colin to reported-by.
>
> > Fixes CoverityScan CID#1397628 ("Dereference after null check")
> >
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> >
> > ------------
> > v2: correct error exit in xfs_initialize_perag() to properly unwind
> > pags if error encountered.
> >
> > v3: correction to error case: ensure previous valid pags not torn
> > down and only new initialized pags are torn down.
>
> patch changelog goes under the "---" below, it shouldn't be part of the
> commitlog, FYI.
>
> > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > ---
> > fs/xfs/xfs_mount.c | 14 ++++++++++----
> > 1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > index 9b9540d..afc49ac 100644
> > --- a/fs/xfs/xfs_mount.c
> > +++ b/fs/xfs/xfs_mount.c
> > @@ -188,8 +188,10 @@ xfs_initialize_perag(
> > {
> > xfs_agnumber_t index;
> > xfs_agnumber_t first_initialised = 0;
> > + xfs_agnumber_t next_agindex = 0;
> > xfs_perag_t *pag;
> > int error = -ENOMEM;
> > + int i;
> >
> > /*
> > * Walk the current per-ag tree so we don't try to initialise AGs
> > @@ -200,14 +202,15 @@ xfs_initialize_perag(
> > pag = xfs_perag_get(mp, index);
> > if (pag) {
> > xfs_perag_put(pag);
> > + next_agindex = index + 1;
>
> a little odd to keep incrementing next_agindex when we really
> only care that it's nonzero, right?
It could just be a flag, but still would need to be checked if true/false.
>
> > continue;
> > }
> > - if (!first_initialised)
> > + if (!first_initialised && (next_agindex > 0))
> > first_initialised = index;
>
> ok, so we deem this index as first_initialized, but:
>
> > pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL);
> > if (!pag)
> > - goto out_unwind;
> > + goto out_unwind_pags;
>
> ...then it could fail. Which is why the unwind loop at the end still
> needs to check for (this) null pag, and skip if so. In fact if you
> ever get to that loop, I think it's guaranteed that the first time
> through will find the null pag for this index and skip it.
>
> It'd be a little nicer to move the first_initialized assignment down,
> after it's actually /been/ allocated and initialized.
>
> This patch also leaves this in place:
>
> if (xfs_buf_hash_init(pag))
> goto out_unwind;
> ...
> out_unwind:
> xfs_buf_hash_destroy(pag);
> kmem_free(pag);
>
> so isn't this doing buf_hash_destroy on something that has failed to
> init in the first place? I have no idea if that's safe, but best to
> just be clean about it.
>
> In the big picture, there are 3 things happening in this loop that may
> need to be unwound for this and/or prior pags on an error:
>
> 1) pag memory allocation
> 2) xfs_buf_hash_init
> 3) radix_tree_insert
>
> For any given iteration through the loop, any of the above which succeed
> must be unwound for /this/ pag, and then all prior initialized pags must
> be unwound. So I'd expect something like this, though please check the
> details and adjust labels to your preference. :)
>
> bool first_init_done = false;
>
> for (index = 0; index < agcount; index++) {
> if (pag exists)
> continue;
>
> allocate(pag)
> if (failure)
> goto unwind_prior_pags;
>
> xfs_buf_hash_init(pag)
> if (failure)
> goto free_pag;
>
> radix_tree_insert(pag)
> if (failure)
> goto hash_destroy;
>
> /* this pag is fully initialized now */
> if (!first_init_done) {
> first_initialized = index;
> first_init_done = true;
> }
> }
>
> /* unwind this pag on init failures */
> hash_destroy:
> xfs_buf_hash_destroy(pag);
> free_pag:
> kmem_free(pag);
> unwind_prior_pags:
> /* back up to previous fully initialized pag and unwind it+prior */
> index--;
> for (; index >= first_initialised; index--)
> pag = radix_tree_delete(index)
> xfs_buf_hash_destroy(pag)
> kmem_free(pag)
>
>
> > pag->pag_agno = index;
> > pag->pag_mount = mp;
> > spin_lock_init(&pag->pag_ici_lock);
> > @@ -242,8 +245,11 @@ xfs_initialize_perag(
> > out_unwind:
> > xfs_buf_hash_destroy(pag);
> > kmem_free(pag);
> > - for (; index > first_initialised; index--) {
> > - pag = radix_tree_delete(&mp->m_perag_tree, index);
> > +out_unwind_pags:
> > + for (i = index; i >= first_initialised; i--) {
> > + pag = radix_tree_delete(&mp->m_perag_tree, (xfs_agnumber_t)i);
>
> (don't make i an int if it's really supposed to be an xfs_agnumber_t,
> which is unsigned)
It might be wrong, but I used an int in order to handle the case where
first_initialized==0, and a decrement of an unsigned would roll over
instead of going negative (so i >= first_initialized would be true again).
>
> -Eric
>
> > + if (!pag)
> > + continue;
> > xfs_buf_hash_destroy(pag);
> > kmem_free(pag);
> > }
> >
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3] xfs: do not call xfs_buf_hash_destroy on a NULL pag
2017-01-26 18:55 ` Bill O'Donnell
@ 2017-01-26 20:27 ` Eric Sandeen
0 siblings, 0 replies; 21+ messages in thread
From: Eric Sandeen @ 2017-01-26 20:27 UTC (permalink / raw)
To: Bill O'Donnell; +Cc: linux-xfs, darrick.wong
On 1/26/17 12:55 PM, Bill O'Donnell wrote:
>>> @@ -242,8 +245,11 @@ xfs_initialize_perag(
>>> out_unwind:
>>> xfs_buf_hash_destroy(pag);
>>> kmem_free(pag);
>>> - for (; index > first_initialised; index--) {
>>> - pag = radix_tree_delete(&mp->m_perag_tree, index);
>>> +out_unwind_pags:
>>> + for (i = index; i >= first_initialised; i--) {
>>> + pag = radix_tree_delete(&mp->m_perag_tree, (xfs_agnumber_t)i);
>> (don't make i an int if it's really supposed to be an xfs_agnumber_t,
>> which is unsigned)
> It might be wrong, but I used an int in order to handle the case where
> first_initialized==0, and a decrement of an unsigned would roll over
> instead of going negative (so i >= first_initialized would be true again).
Oh, urgh. I see.
OTOH AG count is unsigned, so if you have index > 2147483648 AGs,
your "i >= first" will fail because i will be negative in /that/
comparison.
Ok, how about something like:
out_unwind_pags:
/* start unwinding at previous index */
index--;
do {
/* uninitialize pag at this index */
index--;
} while (index && index >= first_initialised);
-Eric
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] xfs: correct null checks and error processing in xfs_initialize_perag
2017-01-20 14:26 [PATCH] xfs: do not call xfs_buf_hash_destroy on a NULL pag Colin King
` (3 preceding siblings ...)
2017-01-25 19:04 ` [PATCH v3] " Bill O'Donnell
@ 2017-01-28 19:19 ` Bill O'Donnell
2017-02-03 22:57 ` Eric Sandeen
` (2 more replies)
4 siblings, 3 replies; 21+ messages in thread
From: Bill O'Donnell @ 2017-01-28 19:19 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong
If pag cannot be allocated, the current error exit path will trip
a null pointer deference error when calling xfs_buf_hash_destroy
with a null pag. Fix this by adding a new error exit labels and
jumping to those accordingly, avoiding the hash destroy and
unnecessary kmem_free on pag.
Up to three things need to be properly unwound:
1) pag memory allocation
2) xfs_buf_hash_init
3) radix_tree_insert
For any given iteration through the loop, any of the above which
succeed must be unwound for /this/ pag, and then all prior
initialized pags must be unwound.
Fixes CoverityScan CID#1397628 ("Dereference after null check")
Reported-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
fs/xfs/xfs_mount.c | 37 ++++++++++++++++++++++++++-----------
1 file changed, 26 insertions(+), 11 deletions(-)
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 9b9540d..b074d2a 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -190,6 +190,7 @@ xfs_initialize_perag(
xfs_agnumber_t first_initialised = 0;
xfs_perag_t *pag;
int error = -ENOMEM;
+ bool first_init_done = false;
/*
* Walk the current per-ag tree so we don't try to initialise AGs
@@ -202,22 +203,20 @@ xfs_initialize_perag(
xfs_perag_put(pag);
continue;
}
- if (!first_initialised)
- first_initialised = index;
pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL);
if (!pag)
- goto out_unwind;
+ goto out_unwind_new_pags;
pag->pag_agno = index;
pag->pag_mount = mp;
spin_lock_init(&pag->pag_ici_lock);
mutex_init(&pag->pag_ici_reclaim_lock);
INIT_RADIX_TREE(&pag->pag_ici_root, GFP_ATOMIC);
if (xfs_buf_hash_init(pag))
- goto out_unwind;
+ goto out_free_pag;
if (radix_tree_preload(GFP_NOFS))
- goto out_unwind;
+ goto out_free_pag;
spin_lock(&mp->m_perag_lock);
if (radix_tree_insert(&mp->m_perag_tree, index, pag)) {
@@ -225,10 +224,15 @@ xfs_initialize_perag(
spin_unlock(&mp->m_perag_lock);
radix_tree_preload_end();
error = -EEXIST;
- goto out_unwind;
+ goto out_hash_destroy;
}
spin_unlock(&mp->m_perag_lock);
radix_tree_preload_end();
+ /* a pag is fully initialized */
+ if (!first_init_done) {
+ first_initialised = index;
+ first_init_done = true;
+ }
}
index = xfs_set_inode_alloc(mp, agcount);
@@ -239,13 +243,24 @@ xfs_initialize_perag(
mp->m_ag_prealloc_blocks = xfs_prealloc_blocks(mp);
return 0;
-out_unwind:
+out_hash_destroy:
xfs_buf_hash_destroy(pag);
+out_free_pag:
kmem_free(pag);
- for (; index > first_initialised; index--) {
- pag = radix_tree_delete(&mp->m_perag_tree, index);
- xfs_buf_hash_destroy(pag);
- kmem_free(pag);
+out_unwind_new_pags:
+ /* unwind any newly initialized pags */
+ if (first_init_done) {
+ index--;
+ do {
+ pag = radix_tree_delete(&mp->m_perag_tree, index);
+ if (pag) {
+ xfs_buf_hash_destroy(pag);
+ kmem_free(pag);
+ }
+ if (!index)
+ break;
+ index--;
+ } while (index >= first_initialised);
}
return error;
}
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH] xfs: correct null checks and error processing in xfs_initialize_perag
2017-01-28 19:19 ` [PATCH] xfs: correct null checks and error processing in xfs_initialize_perag Bill O'Donnell
@ 2017-02-03 22:57 ` Eric Sandeen
2017-02-06 17:08 ` [PATCH v2] " Bill O'Donnell
2017-02-07 16:54 ` [PATCH v3] " Bill O'Donnell
2 siblings, 0 replies; 21+ messages in thread
From: Eric Sandeen @ 2017-02-03 22:57 UTC (permalink / raw)
To: Bill O'Donnell, linux-xfs; +Cc: darrick.wong
On 1/28/17 1:19 PM, Bill O'Donnell wrote:
> If pag cannot be allocated, the current error exit path will trip
> a null pointer deference error when calling xfs_buf_hash_destroy
> with a null pag. Fix this by adding a new error exit labels and
> jumping to those accordingly, avoiding the hash destroy and
> unnecessary kmem_free on pag.
>
> Up to three things need to be properly unwound:
>
> 1) pag memory allocation
> 2) xfs_buf_hash_init
> 3) radix_tree_insert
>
> For any given iteration through the loop, any of the above which
> succeed must be unwound for /this/ pag, and then all prior
> initialized pags must be unwound.
>
> Fixes CoverityScan CID#1397628 ("Dereference after null check")
>
> Reported-by: Colin Ian King <colin.king@canonical.com>
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
> fs/xfs/xfs_mount.c | 37 ++++++++++++++++++++++++++-----------
> 1 file changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 9b9540d..b074d2a 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -190,6 +190,7 @@ xfs_initialize_perag(
> xfs_agnumber_t first_initialised = 0;
> xfs_perag_t *pag;
> int error = -ENOMEM;
> + bool first_init_done = false;
>
> /*
> * Walk the current per-ag tree so we don't try to initialise AGs
> @@ -202,22 +203,20 @@ xfs_initialize_perag(
> xfs_perag_put(pag);
> continue;
> }
> - if (!first_initialised)
> - first_initialised = index;
>
> pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL);
> if (!pag)
> - goto out_unwind;
> + goto out_unwind_new_pags;
> pag->pag_agno = index;
> pag->pag_mount = mp;
> spin_lock_init(&pag->pag_ici_lock);
> mutex_init(&pag->pag_ici_reclaim_lock);
> INIT_RADIX_TREE(&pag->pag_ici_root, GFP_ATOMIC);
> if (xfs_buf_hash_init(pag))
> - goto out_unwind;
> + goto out_free_pag;
now the hash is initialized
> if (radix_tree_preload(GFP_NOFS))
> - goto out_unwind;
> + goto out_free_pag;
but we doesn't destroy it if we fail here and goto out_free_pag
> spin_lock(&mp->m_perag_lock);
> if (radix_tree_insert(&mp->m_perag_tree, index, pag)) {
> @@ -225,10 +224,15 @@ xfs_initialize_perag(
> spin_unlock(&mp->m_perag_lock);
> radix_tree_preload_end();
> error = -EEXIST;
> - goto out_unwind;
> + goto out_hash_destroy;
> }
> spin_unlock(&mp->m_perag_lock);
> radix_tree_preload_end();
> + /* a pag is fully initialized */
> + if (!first_init_done) {
> + first_initialised = index;
> + first_init_done = true;
> + }
> }
>
> index = xfs_set_inode_alloc(mp, agcount);
> @@ -239,13 +243,24 @@ xfs_initialize_perag(
> mp->m_ag_prealloc_blocks = xfs_prealloc_blocks(mp);
> return 0;
>
> -out_unwind:
> +out_hash_destroy:
> xfs_buf_hash_destroy(pag);
> +out_free_pag:
> kmem_free(pag);
> - for (; index > first_initialised; index--) {
> - pag = radix_tree_delete(&mp->m_perag_tree, index);
> - xfs_buf_hash_destroy(pag);
> - kmem_free(pag);
> +out_unwind_new_pags:
> + /* unwind any newly initialized pags */
> + if (first_init_done) {
> + index--;
> + do {
> + pag = radix_tree_delete(&mp->m_perag_tree, index);
> + if (pag) {
I think that if this has all been done right keeping careful
track of what was last initialized, there is no need to test
if (pag). If you'd like to use if (pag) in lieu of some of the
other controls that's fine too, but I don't really see a reason
for both.
Just a style thing I guess, take it or leave it.
-Eric
> + xfs_buf_hash_destroy(pag);
> + kmem_free(pag);
> + }
> + if (!index)
> + break;
> + index--;
> + } while (index >= first_initialised);
> }
> return error;
> }
>
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH v2] xfs: correct null checks and error processing in xfs_initialize_perag
2017-01-28 19:19 ` [PATCH] xfs: correct null checks and error processing in xfs_initialize_perag Bill O'Donnell
2017-02-03 22:57 ` Eric Sandeen
@ 2017-02-06 17:08 ` Bill O'Donnell
2017-02-06 19:25 ` Darrick J. Wong
2017-02-07 16:54 ` [PATCH v3] " Bill O'Donnell
2 siblings, 1 reply; 21+ messages in thread
From: Bill O'Donnell @ 2017-02-06 17:08 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong
If pag cannot be allocated, the current error exit path will trip
a null pointer deference error when calling xfs_buf_hash_destroy
with a null pag. Fix this by adding a new error exit labels and
jumping to those accordingly, avoiding the hash destroy and
unnecessary kmem_free on pag.
Up to three things need to be properly unwound:
1) pag memory allocation
2) xfs_buf_hash_init
3) radix_tree_insert
For any given iteration through the loop, any of the above which
succeed must be unwound for /this/ pag, and then all prior
initialized pags must be unwound.
Fixes CoverityScan CID#1397628 ("Dereference after null check")
Reported-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
v2: correct jump to out_hash_destroy for case where hash is initialized.
use NULLAGNUMBER to simplify first_initialised loop logic.
fs/xfs/xfs_mount.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 9b9540d..3c28af1 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -187,7 +187,7 @@ xfs_initialize_perag(
xfs_agnumber_t *maxagi)
{
xfs_agnumber_t index;
- xfs_agnumber_t first_initialised = 0;
+ xfs_agnumber_t first_initialised = NULLAGNUMBER;
xfs_perag_t *pag;
int error = -ENOMEM;
@@ -202,22 +202,20 @@ xfs_initialize_perag(
xfs_perag_put(pag);
continue;
}
- if (!first_initialised)
- first_initialised = index;
pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL);
if (!pag)
- goto out_unwind;
+ goto out_unwind_new_pags;
pag->pag_agno = index;
pag->pag_mount = mp;
spin_lock_init(&pag->pag_ici_lock);
mutex_init(&pag->pag_ici_reclaim_lock);
INIT_RADIX_TREE(&pag->pag_ici_root, GFP_ATOMIC);
if (xfs_buf_hash_init(pag))
- goto out_unwind;
+ goto out_free_pag;
if (radix_tree_preload(GFP_NOFS))
- goto out_unwind;
+ goto out_hash_destroy;
spin_lock(&mp->m_perag_lock);
if (radix_tree_insert(&mp->m_perag_tree, index, pag)) {
@@ -225,10 +223,13 @@ xfs_initialize_perag(
spin_unlock(&mp->m_perag_lock);
radix_tree_preload_end();
error = -EEXIST;
- goto out_unwind;
+ goto out_hash_destroy;
}
spin_unlock(&mp->m_perag_lock);
radix_tree_preload_end();
+ /* first new pag is fully initialized */
+ if (first_initialised == NULLAGNUMBER)
+ first_initialised = index;
}
index = xfs_set_inode_alloc(mp, agcount);
@@ -239,10 +240,13 @@ xfs_initialize_perag(
mp->m_ag_prealloc_blocks = xfs_prealloc_blocks(mp);
return 0;
-out_unwind:
+out_hash_destroy:
xfs_buf_hash_destroy(pag);
+out_free_pag:
kmem_free(pag);
- for (; index > first_initialised; index--) {
+out_unwind_new_pags:
+ /* unwind any prior newly initialized pags */
+ while (index-- >= first_initialised && index != NULLAGNUMBER) {
pag = radix_tree_delete(&mp->m_perag_tree, index);
xfs_buf_hash_destroy(pag);
kmem_free(pag);
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2] xfs: correct null checks and error processing in xfs_initialize_perag
2017-02-06 17:08 ` [PATCH v2] " Bill O'Donnell
@ 2017-02-06 19:25 ` Darrick J. Wong
0 siblings, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2017-02-06 19:25 UTC (permalink / raw)
To: Bill O'Donnell; +Cc: linux-xfs
On Mon, Feb 06, 2017 at 11:08:30AM -0600, Bill O'Donnell wrote:
> If pag cannot be allocated, the current error exit path will trip
> a null pointer deference error when calling xfs_buf_hash_destroy
> with a null pag. Fix this by adding a new error exit labels and
> jumping to those accordingly, avoiding the hash destroy and
> unnecessary kmem_free on pag.
>
> Up to three things need to be properly unwound:
>
> 1) pag memory allocation
> 2) xfs_buf_hash_init
> 3) radix_tree_insert
>
> For any given iteration through the loop, any of the above which
> succeed must be unwound for /this/ pag, and then all prior
> initialized pags must be unwound.
>
> Fixes CoverityScan CID#1397628 ("Dereference after null check")
>
> Reported-by: Colin Ian King <colin.king@canonical.com>
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
> v2: correct jump to out_hash_destroy for case where hash is initialized.
> use NULLAGNUMBER to simplify first_initialised loop logic.
>
> fs/xfs/xfs_mount.c | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 9b9540d..3c28af1 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -187,7 +187,7 @@ xfs_initialize_perag(
> xfs_agnumber_t *maxagi)
> {
> xfs_agnumber_t index;
> - xfs_agnumber_t first_initialised = 0;
> + xfs_agnumber_t first_initialised = NULLAGNUMBER;
> xfs_perag_t *pag;
> int error = -ENOMEM;
>
> @@ -202,22 +202,20 @@ xfs_initialize_perag(
> xfs_perag_put(pag);
> continue;
> }
> - if (!first_initialised)
> - first_initialised = index;
>
> pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL);
> if (!pag)
> - goto out_unwind;
> + goto out_unwind_new_pags;
> pag->pag_agno = index;
> pag->pag_mount = mp;
> spin_lock_init(&pag->pag_ici_lock);
> mutex_init(&pag->pag_ici_reclaim_lock);
> INIT_RADIX_TREE(&pag->pag_ici_root, GFP_ATOMIC);
> if (xfs_buf_hash_init(pag))
> - goto out_unwind;
> + goto out_free_pag;
>
> if (radix_tree_preload(GFP_NOFS))
> - goto out_unwind;
> + goto out_hash_destroy;
>
> spin_lock(&mp->m_perag_lock);
> if (radix_tree_insert(&mp->m_perag_tree, index, pag)) {
> @@ -225,10 +223,13 @@ xfs_initialize_perag(
> spin_unlock(&mp->m_perag_lock);
> radix_tree_preload_end();
> error = -EEXIST;
> - goto out_unwind;
> + goto out_hash_destroy;
> }
> spin_unlock(&mp->m_perag_lock);
> radix_tree_preload_end();
> + /* first new pag is fully initialized */
> + if (first_initialised == NULLAGNUMBER)
> + first_initialised = index;
> }
>
> index = xfs_set_inode_alloc(mp, agcount);
> @@ -239,10 +240,13 @@ xfs_initialize_perag(
> mp->m_ag_prealloc_blocks = xfs_prealloc_blocks(mp);
> return 0;
>
> -out_unwind:
> +out_hash_destroy:
> xfs_buf_hash_destroy(pag);
> +out_free_pag:
> kmem_free(pag);
> - for (; index > first_initialised; index--) {
> +out_unwind_new_pags:
> + /* unwind any prior newly initialized pags */
> + while (index-- >= first_initialised && index != NULLAGNUMBER) {
I think there's a bug here...
(Ugh, post-decrement of a variable used twice in a loop conditional. :))
Let's say that AG0-3 were initialized, and now we're growfs'ing to 6
AGs. AG4 initializes and inserts w/o problem, so now we have
first_initialized == 4 and index == 5.
Then kmem_zalloc fails for AG5, so we jump to out_unwind_new_pags:
while (5 >= 4 && 4? 5? != NULLAGNUMBER)
Ok, now index == 4 because of the postdecrement...
radix_tree_delete(&mp->m_perag_tree, 4);
<delete stuff>
while (4 >= 4 && ...)
But now index == 3 again because of the postdecrement, so we...
radix_tree_delete(&mp->m_perag_tree, 3);
<delete stuff>
I think we just blew away AG 3's pag data.
Now that we use NULLAGNUMBER to mean "we haven't inserted anything in
the perag_tree", why not do:
for (index = first_initialized; index < agcount; index++) {
pag = radix_tree_delete(&mp->m_perag_tree, index);
if (!pag)
break;
xfs_buf_hash_destroy(pag);
kmem_free(pag);
}
?
--D
> pag = radix_tree_delete(&mp->m_perag_tree, index);
> xfs_buf_hash_destroy(pag);
> kmem_free(pag);
> --
> 2.9.3
>
> --
> 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] 21+ messages in thread
* [PATCH v3] xfs: correct null checks and error processing in xfs_initialize_perag
2017-01-28 19:19 ` [PATCH] xfs: correct null checks and error processing in xfs_initialize_perag Bill O'Donnell
2017-02-03 22:57 ` Eric Sandeen
2017-02-06 17:08 ` [PATCH v2] " Bill O'Donnell
@ 2017-02-07 16:54 ` Bill O'Donnell
2017-02-07 20:49 ` Eric Sandeen
2017-02-07 20:59 ` Darrick J. Wong
2 siblings, 2 replies; 21+ messages in thread
From: Bill O'Donnell @ 2017-02-07 16:54 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong
If pag cannot be allocated, the current error exit path will trip
a null pointer deference error when calling xfs_buf_hash_destroy
with a null pag. Fix this by adding a new error exit labels and
jumping to those accordingly, avoiding the hash destroy and
unnecessary kmem_free on pag.
Up to three things need to be properly unwound:
1) pag memory allocation
2) xfs_buf_hash_init
3) radix_tree_insert
For any given iteration through the loop, any of the above which
succeed must be unwound for /this/ pag, and then all prior
initialized pags must be unwound.
Fixes CoverityScan CID#1397628 ("Dereference after null check")
Reported-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
v3: correct indexing error in out_unwind_new_pags loop,
avoiding destruction of valid pags. Exit loop if !pag.
v2: correct jump to out_hash_destroy for case where hash is initialized.
use NULLAGNUMBER to simplify first_initialised loop logic.
fs/xfs/xfs_mount.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 9b9540d..1f1e4ae 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -187,7 +187,7 @@ xfs_initialize_perag(
xfs_agnumber_t *maxagi)
{
xfs_agnumber_t index;
- xfs_agnumber_t first_initialised = 0;
+ xfs_agnumber_t first_initialised = NULLAGNUMBER;
xfs_perag_t *pag;
int error = -ENOMEM;
@@ -202,22 +202,20 @@ xfs_initialize_perag(
xfs_perag_put(pag);
continue;
}
- if (!first_initialised)
- first_initialised = index;
pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL);
if (!pag)
- goto out_unwind;
+ goto out_unwind_new_pags;
pag->pag_agno = index;
pag->pag_mount = mp;
spin_lock_init(&pag->pag_ici_lock);
mutex_init(&pag->pag_ici_reclaim_lock);
INIT_RADIX_TREE(&pag->pag_ici_root, GFP_ATOMIC);
if (xfs_buf_hash_init(pag))
- goto out_unwind;
+ goto out_free_pag;
if (radix_tree_preload(GFP_NOFS))
- goto out_unwind;
+ goto out_hash_destroy;
spin_lock(&mp->m_perag_lock);
if (radix_tree_insert(&mp->m_perag_tree, index, pag)) {
@@ -225,10 +223,13 @@ xfs_initialize_perag(
spin_unlock(&mp->m_perag_lock);
radix_tree_preload_end();
error = -EEXIST;
- goto out_unwind;
+ goto out_hash_destroy;
}
spin_unlock(&mp->m_perag_lock);
radix_tree_preload_end();
+ /* first new pag is fully initialized */
+ if (first_initialised == NULLAGNUMBER)
+ first_initialised = index;
}
index = xfs_set_inode_alloc(mp, agcount);
@@ -239,11 +240,16 @@ xfs_initialize_perag(
mp->m_ag_prealloc_blocks = xfs_prealloc_blocks(mp);
return 0;
-out_unwind:
+out_hash_destroy:
xfs_buf_hash_destroy(pag);
+out_free_pag:
kmem_free(pag);
- for (; index > first_initialised; index--) {
+out_unwind_new_pags:
+ /* unwind any prior newly initialized pags */
+ for (index = first_initialised; index < agcount; index++) {
pag = radix_tree_delete(&mp->m_perag_tree, index);
+ if (!pag)
+ break;
xfs_buf_hash_destroy(pag);
kmem_free(pag);
}
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3] xfs: correct null checks and error processing in xfs_initialize_perag
2017-02-07 16:54 ` [PATCH v3] " Bill O'Donnell
@ 2017-02-07 20:49 ` Eric Sandeen
2017-02-07 20:59 ` Darrick J. Wong
1 sibling, 0 replies; 21+ messages in thread
From: Eric Sandeen @ 2017-02-07 20:49 UTC (permalink / raw)
To: Bill O'Donnell, linux-xfs; +Cc: darrick.wong
On 2/7/17 10:54 AM, Bill O'Donnell wrote:
> If pag cannot be allocated, the current error exit path will trip
> a null pointer deference error when calling xfs_buf_hash_destroy
> with a null pag. Fix this by adding a new error exit labels and
> jumping to those accordingly, avoiding the hash destroy and
> unnecessary kmem_free on pag.
>
> Up to three things need to be properly unwound:
>
> 1) pag memory allocation
> 2) xfs_buf_hash_init
> 3) radix_tree_insert
>
> For any given iteration through the loop, any of the above which
> succeed must be unwound for /this/ pag, and then all prior
> initialized pags must be unwound.
>
> Fixes CoverityScan CID#1397628 ("Dereference after null check")
>
> Reported-by: Colin Ian King <colin.king@canonical.com>
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
Yeah, this looks good to me, thanks.
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> ---
> v3: correct indexing error in out_unwind_new_pags loop,
> avoiding destruction of valid pags. Exit loop if !pag.
> v2: correct jump to out_hash_destroy for case where hash is initialized.
> use NULLAGNUMBER to simplify first_initialised loop logic.
>
> fs/xfs/xfs_mount.c | 24 +++++++++++++++---------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 9b9540d..1f1e4ae 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -187,7 +187,7 @@ xfs_initialize_perag(
> xfs_agnumber_t *maxagi)
> {
> xfs_agnumber_t index;
> - xfs_agnumber_t first_initialised = 0;
> + xfs_agnumber_t first_initialised = NULLAGNUMBER;
> xfs_perag_t *pag;
> int error = -ENOMEM;
>
> @@ -202,22 +202,20 @@ xfs_initialize_perag(
> xfs_perag_put(pag);
> continue;
> }
> - if (!first_initialised)
> - first_initialised = index;
>
> pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL);
> if (!pag)
> - goto out_unwind;
> + goto out_unwind_new_pags;
> pag->pag_agno = index;
> pag->pag_mount = mp;
> spin_lock_init(&pag->pag_ici_lock);
> mutex_init(&pag->pag_ici_reclaim_lock);
> INIT_RADIX_TREE(&pag->pag_ici_root, GFP_ATOMIC);
> if (xfs_buf_hash_init(pag))
> - goto out_unwind;
> + goto out_free_pag;
>
> if (radix_tree_preload(GFP_NOFS))
> - goto out_unwind;
> + goto out_hash_destroy;
>
> spin_lock(&mp->m_perag_lock);
> if (radix_tree_insert(&mp->m_perag_tree, index, pag)) {
> @@ -225,10 +223,13 @@ xfs_initialize_perag(
> spin_unlock(&mp->m_perag_lock);
> radix_tree_preload_end();
> error = -EEXIST;
> - goto out_unwind;
> + goto out_hash_destroy;
> }
> spin_unlock(&mp->m_perag_lock);
> radix_tree_preload_end();
> + /* first new pag is fully initialized */
> + if (first_initialised == NULLAGNUMBER)
> + first_initialised = index;
> }
>
> index = xfs_set_inode_alloc(mp, agcount);
> @@ -239,11 +240,16 @@ xfs_initialize_perag(
> mp->m_ag_prealloc_blocks = xfs_prealloc_blocks(mp);
> return 0;
>
> -out_unwind:
> +out_hash_destroy:
> xfs_buf_hash_destroy(pag);
> +out_free_pag:
> kmem_free(pag);
> - for (; index > first_initialised; index--) {
> +out_unwind_new_pags:
> + /* unwind any prior newly initialized pags */
> + for (index = first_initialised; index < agcount; index++) {
> pag = radix_tree_delete(&mp->m_perag_tree, index);
> + if (!pag)
> + break;
> xfs_buf_hash_destroy(pag);
> kmem_free(pag);
> }
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3] xfs: correct null checks and error processing in xfs_initialize_perag
2017-02-07 16:54 ` [PATCH v3] " Bill O'Donnell
2017-02-07 20:49 ` Eric Sandeen
@ 2017-02-07 20:59 ` Darrick J. Wong
1 sibling, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2017-02-07 20:59 UTC (permalink / raw)
To: Bill O'Donnell; +Cc: linux-xfs
On Tue, Feb 07, 2017 at 10:54:03AM -0600, Bill O'Donnell wrote:
> If pag cannot be allocated, the current error exit path will trip
> a null pointer deference error when calling xfs_buf_hash_destroy
> with a null pag. Fix this by adding a new error exit labels and
> jumping to those accordingly, avoiding the hash destroy and
> unnecessary kmem_free on pag.
>
> Up to three things need to be properly unwound:
>
> 1) pag memory allocation
> 2) xfs_buf_hash_init
> 3) radix_tree_insert
>
> For any given iteration through the loop, any of the above which
> succeed must be unwound for /this/ pag, and then all prior
> initialized pags must be unwound.
>
> Fixes CoverityScan CID#1397628 ("Dereference after null check")
>
> Reported-by: Colin Ian King <colin.king@canonical.com>
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
Will pick up for 4.11,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> v3: correct indexing error in out_unwind_new_pags loop,
> avoiding destruction of valid pags. Exit loop if !pag.
> v2: correct jump to out_hash_destroy for case where hash is initialized.
> use NULLAGNUMBER to simplify first_initialised loop logic.
>
> fs/xfs/xfs_mount.c | 24 +++++++++++++++---------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 9b9540d..1f1e4ae 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -187,7 +187,7 @@ xfs_initialize_perag(
> xfs_agnumber_t *maxagi)
> {
> xfs_agnumber_t index;
> - xfs_agnumber_t first_initialised = 0;
> + xfs_agnumber_t first_initialised = NULLAGNUMBER;
> xfs_perag_t *pag;
> int error = -ENOMEM;
>
> @@ -202,22 +202,20 @@ xfs_initialize_perag(
> xfs_perag_put(pag);
> continue;
> }
> - if (!first_initialised)
> - first_initialised = index;
>
> pag = kmem_zalloc(sizeof(*pag), KM_MAYFAIL);
> if (!pag)
> - goto out_unwind;
> + goto out_unwind_new_pags;
> pag->pag_agno = index;
> pag->pag_mount = mp;
> spin_lock_init(&pag->pag_ici_lock);
> mutex_init(&pag->pag_ici_reclaim_lock);
> INIT_RADIX_TREE(&pag->pag_ici_root, GFP_ATOMIC);
> if (xfs_buf_hash_init(pag))
> - goto out_unwind;
> + goto out_free_pag;
>
> if (radix_tree_preload(GFP_NOFS))
> - goto out_unwind;
> + goto out_hash_destroy;
>
> spin_lock(&mp->m_perag_lock);
> if (radix_tree_insert(&mp->m_perag_tree, index, pag)) {
> @@ -225,10 +223,13 @@ xfs_initialize_perag(
> spin_unlock(&mp->m_perag_lock);
> radix_tree_preload_end();
> error = -EEXIST;
> - goto out_unwind;
> + goto out_hash_destroy;
> }
> spin_unlock(&mp->m_perag_lock);
> radix_tree_preload_end();
> + /* first new pag is fully initialized */
> + if (first_initialised == NULLAGNUMBER)
> + first_initialised = index;
> }
>
> index = xfs_set_inode_alloc(mp, agcount);
> @@ -239,11 +240,16 @@ xfs_initialize_perag(
> mp->m_ag_prealloc_blocks = xfs_prealloc_blocks(mp);
> return 0;
>
> -out_unwind:
> +out_hash_destroy:
> xfs_buf_hash_destroy(pag);
> +out_free_pag:
> kmem_free(pag);
> - for (; index > first_initialised; index--) {
> +out_unwind_new_pags:
> + /* unwind any prior newly initialized pags */
> + for (index = first_initialised; index < agcount; index++) {
> pag = radix_tree_delete(&mp->m_perag_tree, index);
> + if (!pag)
> + break;
> xfs_buf_hash_destroy(pag);
> kmem_free(pag);
> }
> --
> 2.9.3
>
> --
> 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] 21+ messages in thread