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@xxxxxxxxxxxxx>
> Signed-off-by: Bill O'Donnell <billodo@xxxxxxxxxx>
Will pick up for 4.11,
Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
--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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
