* [PATCH] xfs: do not call xfs_buf_hash_destroy on a NULL pag
@ 2017-01-20 14:26 Colin King
2017-01-20 16:34 ` Darrick J. Wong
2017-01-20 19:26 ` Eric Sandeen
0 siblings, 2 replies; 7+ messages in thread
From: Colin King @ 2017-01-20 14:26 UTC (permalink / raw)
To: Darrick J . Wong, linux-xfs, linux-kernel
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>
---
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
^ permalink raw reply related [flat|nested] 7+ 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 1 sibling, 0 replies; 7+ 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] 7+ 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 1 sibling, 1 reply; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ messages in thread
end of thread, other threads:[~2017-01-24 18:35 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 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-20 23:04 ` Colin Ian King 2017-01-24 15:04 ` Bill O'Donnell 2017-01-24 18:34 ` 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