Re: [PATCH v2] xfs: do not call xfs_buf_hash_destroy on a NULL pag

["Bill O'Donnell" <billodo@redhat.com>]

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