Re: [PATCH 1/2] xfs_repair: Fix check_refcount() error path

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Nov 09, 2022 at 12:28:09PM +0100, Carlos Maiolino wrote:
> On Mon, Sep 05, 2022 at 09:05:28AM +0200, Carlos Maiolino wrote:
> > On Fri, Sep 02, 2022 at 01:48:21PM -0700, Darrick J. Wong wrote:
> > > On Fri, Sep 02, 2022 at 03:43:39PM +0200, Carlos Maiolino wrote:
> > > > From: Carlos Maiolino <cmaiolino@redhat.com>
> > > >
> > > > Add proper exit error paths to avoid checking all pointers at the current path
> > > >
> > > > Fixes-coverity-id: 1512651
> > > >
> > > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > > > ---
> > > >  repair/rmap.c |   23 +++++++++++------------
> > > >  1 file changed, 11 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/repair/rmap.c b/repair/rmap.c
> > > > index a7c4b25b1..0253c0c36 100644
> > > > --- a/repair/rmap.c
> > > > +++ b/repair/rmap.c
> > > > @@ -1377,7 +1377,7 @@ check_refcounts(
> > > >  	if (error) {
> > > >  		do_warn(_("Could not read AGF %u to check refcount btree.\n"),
> > > >  				agno);
> > > > -		goto err;
> > > > +		goto err_agf;
> > > 
> > > Shouldn't this       ^^^^^^^ be err_pag, since we're erroring out and
> > > releasing the perag group reference?
> > 
> > At first I named it err_pag, but pag is used here only to read the agf, and when
> > reading agf fail is why we end up reaching this error path, so I thought it
> > would be more specific to name it err_agf.

The kernel error-out label naming convention (AFAICT) is that the label
says what is being cleaned up.  For example, xfs_create():

 out_trans_cancel:
	xfs_trans_cancel(tp);
 out_release_inode:
	/*
	 * Wait until after the current transaction is aborted to finish
	 * the
	 * setup of the inode and release the inode.  This prevents
	 * recursive
	 * transactions and deadlocks from xfs_inactive.
	 */
	if (ip) {
		xfs_finish_inode_setup(ip);
		xfs_irele(ip);
	}
 out_release_dquots:
	xfs_qm_dqrele(udqp);
	xfs_qm_dqrele(gdqp);
	xfs_qm_dqrele(pdqp);

	if (unlock_dp_on_error)
		xfs_iunlock(dp, XFS_ILOCK_EXCL);
	return error;
}

> > > Also ... don't the "if (XXX) free(XXX)" bits take care of all this?
> > > 
> > 
> > Yeah, it does. But that's exactly what coverity is complaining about. We check
> > for a NULL pointer 'after' we dereference it earlier, to be more specific:
> > 
> > ---
> > Type: Dereference before NULL check
> > Null-checking pag suggests that it may be null, but it has already been
> > dereferenced on all paths leading to the check
> > ---
> > 
> > Both patches fix the same issue type.

Eh, I suppose it does get the coding style closer to how things are done
most other places, so it's a good idea.

Also, while you're at it, the btree cursor deletion function has long
accepted negative (and positive) errno as the second argument, so you
can turn it into:

	libxfs_btree_del_cursor(bt_cur, error);

No need for XFS_BTREE_{NO,}ERROR.

> > > (I can't access Coverity any more, so I don't know what's in the
> > > report.)
> > > 
> > > --D
> > > 
> > > >  	}
> 
> Hi Darrick. Do you have any other opinion at this? Or should I consider it a
> no-no and discard those patches?

Sorry, I guess I forgot to reply to you. :(

--D

> Cheers.
> 
> -- 
> Carlos Maiolino