| Message ID | 20170207165403.11543-1-billodo@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
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); > } > -- 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
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 -- 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
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); }
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(-)