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

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

[Carlos Maiolino]

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;
 	}
 
 	/* Leave the per-ag data "uninitialized" since we rewrite it later */
@@ -1386,7 +1386,7 @@ check_refcounts(
 	bt_cur = libxfs_refcountbt_init_cursor(mp, NULL, agbp, pag);
 	if (!bt_cur) {
 		do_warn(_("Not enough memory to check refcount data.\n"));
-		goto err;
+		goto err_bt_cur;
 	}
 
 	rl_rec = pop_slab_cursor(rl_cur);
@@ -1398,7 +1398,7 @@ check_refcounts(
 			do_warn(
 _("Could not read reference count record for (%u/%u).\n"),
 					agno, rl_rec->rc_startblock);
-			goto err;
+			goto err_loop;
 		}
 		if (!have) {
 			do_warn(
@@ -1413,7 +1413,7 @@ _("Missing reference count record for (%u/%u) len %u count %u\n"),
 			do_warn(
 _("Could not read reference count record for (%u/%u).\n"),
 					agno, rl_rec->rc_startblock);
-			goto err;
+			goto err_loop;
 		}
 		if (!i) {
 			do_warn(
@@ -1436,14 +1436,13 @@ next_loop:
 		rl_rec = pop_slab_cursor(rl_cur);
 	}
 
-err:
-	if (bt_cur)
-		libxfs_btree_del_cursor(bt_cur, error ? XFS_BTREE_ERROR :
-							XFS_BTREE_NOERROR);
-	if (pag)
-		libxfs_perag_put(pag);
-	if (agbp)
-		libxfs_buf_relse(agbp);
+err_loop:
+	libxfs_btree_del_cursor(bt_cur, error ?
+				XFS_BTREE_ERROR : XFS_BTREE_NOERROR);
+err_bt_cur:
+	libxfs_buf_relse(agbp);
+err_agf:
+	libxfs_perag_put(pag);
 	free_slab_cursor(&rl_cur);
 }
 

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

[Darrick J. Wong]

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?

Also ... don't the "if (XXX) free(XXX)" bits take care of all this?

(I can't access Coverity any more, so I don't know what's in the
report.)

--D

>  	}
>  
>  	/* Leave the per-ag data "uninitialized" since we rewrite it later */
> @@ -1386,7 +1386,7 @@ check_refcounts(
>  	bt_cur = libxfs_refcountbt_init_cursor(mp, NULL, agbp, pag);
>  	if (!bt_cur) {
>  		do_warn(_("Not enough memory to check refcount data.\n"));
> -		goto err;
> +		goto err_bt_cur;
>  	}
>  
>  	rl_rec = pop_slab_cursor(rl_cur);
> @@ -1398,7 +1398,7 @@ check_refcounts(
>  			do_warn(
>  _("Could not read reference count record for (%u/%u).\n"),
>  					agno, rl_rec->rc_startblock);
> -			goto err;
> +			goto err_loop;
>  		}
>  		if (!have) {
>  			do_warn(
> @@ -1413,7 +1413,7 @@ _("Missing reference count record for (%u/%u) len %u count %u\n"),
>  			do_warn(
>  _("Could not read reference count record for (%u/%u).\n"),
>  					agno, rl_rec->rc_startblock);
> -			goto err;
> +			goto err_loop;
>  		}
>  		if (!i) {
>  			do_warn(
> @@ -1436,14 +1436,13 @@ next_loop:
>  		rl_rec = pop_slab_cursor(rl_cur);
>  	}
>  
> -err:
> -	if (bt_cur)
> -		libxfs_btree_del_cursor(bt_cur, error ? XFS_BTREE_ERROR :
> -							XFS_BTREE_NOERROR);
> -	if (pag)
> -		libxfs_perag_put(pag);
> -	if (agbp)
> -		libxfs_buf_relse(agbp);
> +err_loop:
> +	libxfs_btree_del_cursor(bt_cur, error ?
> +				XFS_BTREE_ERROR : XFS_BTREE_NOERROR);
> +err_bt_cur:
> +	libxfs_buf_relse(agbp);
> +err_agf:
> +	libxfs_perag_put(pag);
>  	free_slab_cursor(&rl_cur);
>  }
>  
> 

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

[Carlos Maiolino]

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.
> 
> 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.

> (I can't access Coverity any more, so I don't know what's in the
> report.)
> 
> --D
> 
> >  	}
> >
> >  	/* Leave the per-ag data "uninitialized" since we rewrite it later */
> > @@ -1386,7 +1386,7 @@ check_refcounts(
> >  	bt_cur = libxfs_refcountbt_init_cursor(mp, NULL, agbp, pag);
> >  	if (!bt_cur) {
> >  		do_warn(_("Not enough memory to check refcount data.\n"));
> > -		goto err;
> > +		goto err_bt_cur;
> >  	}
> >
> >  	rl_rec = pop_slab_cursor(rl_cur);
> > @@ -1398,7 +1398,7 @@ check_refcounts(
> >  			do_warn(
> >  _("Could not read reference count record for (%u/%u).\n"),
> >  					agno, rl_rec->rc_startblock);
> > -			goto err;
> > +			goto err_loop;
> >  		}
> >  		if (!have) {
> >  			do_warn(
> > @@ -1413,7 +1413,7 @@ _("Missing reference count record for (%u/%u) len %u count %u\n"),
> >  			do_warn(
> >  _("Could not read reference count record for (%u/%u).\n"),
> >  					agno, rl_rec->rc_startblock);
> > -			goto err;
> > +			goto err_loop;
> >  		}
> >  		if (!i) {
> >  			do_warn(
> > @@ -1436,14 +1436,13 @@ next_loop:
> >  		rl_rec = pop_slab_cursor(rl_cur);
> >  	}
> >
> > -err:
> > -	if (bt_cur)
> > -		libxfs_btree_del_cursor(bt_cur, error ? XFS_BTREE_ERROR :
> > -							XFS_BTREE_NOERROR);
> > -	if (pag)
> > -		libxfs_perag_put(pag);
> > -	if (agbp)
> > -		libxfs_buf_relse(agbp);
> > +err_loop:
> > +	libxfs_btree_del_cursor(bt_cur, error ?
> > +				XFS_BTREE_ERROR : XFS_BTREE_NOERROR);
> > +err_bt_cur:
> > +	libxfs_buf_relse(agbp);
> > +err_agf:
> > +	libxfs_perag_put(pag);
> >  	free_slab_cursor(&rl_cur);
> >  }
> >
> >

-- 
Carlos Maiolino

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

[Carlos Maiolino]

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.
> > 
> > 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.
> 
> > (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?

Cheers.

-- 
Carlos Maiolino

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

[Darrick J. Wong]

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

[PATCH V2 0/2] xfsprogs: fix covscan issues

[cem]

From: Carlos Maiolino <cem@kernel.org>

Fix a couple of minor issues found by covscan

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>

Carlos Maiolino (2):
  xfs_repair: Fix check_refcount() error path
  xfs_repair: Fix rmaps_verify_btree() error path

 repair/rmap.c | 43 ++++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 23 deletions(-)

-- 
2.30.2

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

[cem]

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>
---
V2:
	- Rename error label from err_agf to err_pag
	- pass error directly to libxfs_btree_del_cursor() without
	  using ternary operator

 repair/rmap.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/repair/rmap.c b/repair/rmap.c
index 2c809fd4f..e76a8f611 100644
--- a/repair/rmap.c
+++ b/repair/rmap.c
@@ -1379,7 +1379,7 @@ check_refcounts(
 	if (error) {
 		do_warn(_("Could not read AGF %u to check refcount btree.\n"),
 				agno);
-		goto err;
+		goto err_pag;
 	}
 
 	/* Leave the per-ag data "uninitialized" since we rewrite it later */
@@ -1388,7 +1388,7 @@ check_refcounts(
 	bt_cur = libxfs_refcountbt_init_cursor(mp, NULL, agbp, pag);
 	if (!bt_cur) {
 		do_warn(_("Not enough memory to check refcount data.\n"));
-		goto err;
+		goto err_bt_cur;
 	}
 
 	rl_rec = pop_slab_cursor(rl_cur);
@@ -1401,7 +1401,7 @@ check_refcounts(
 			do_warn(
 _("Could not read reference count record for (%u/%u).\n"),
 					agno, rl_rec->rc_startblock);
-			goto err;
+			goto err_loop;
 		}
 		if (!have) {
 			do_warn(
@@ -1416,7 +1416,7 @@ _("Missing reference count record for (%u/%u) len %u count %u\n"),
 			do_warn(
 _("Could not read reference count record for (%u/%u).\n"),
 					agno, rl_rec->rc_startblock);
-			goto err;
+			goto err_loop;
 		}
 		if (!i) {
 			do_warn(
@@ -1446,14 +1446,12 @@ next_loop:
 		rl_rec = pop_slab_cursor(rl_cur);
 	}
 
-err:
-	if (bt_cur)
-		libxfs_btree_del_cursor(bt_cur, error ? XFS_BTREE_ERROR :
-							XFS_BTREE_NOERROR);
-	if (pag)
-		libxfs_perag_put(pag);
-	if (agbp)
-		libxfs_buf_relse(agbp);
+err_loop:
+	libxfs_btree_del_cursor(bt_cur, error);
+err_bt_cur:
+	libxfs_buf_relse(agbp);
+err_pag:
+	libxfs_perag_put(pag);
 	free_slab_cursor(&rl_cur);
 }
 
-- 
2.30.2

[PATCH 2/2] xfs_repair: Fix rmaps_verify_btree() error path

[cem]

From: Carlos Maiolino <cmaiolino@redhat.com>

Add proper exit error paths to avoid checking all pointers at the current path

Fixes-coverity-id: 1512654

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
V2:
	- Rename error label from err_agf to err_pag

 repair/rmap.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/repair/rmap.c b/repair/rmap.c
index e76a8f611..4001df57a 100644
--- a/repair/rmap.c
+++ b/repair/rmap.c
@@ -1004,7 +1004,7 @@ rmaps_verify_btree(
 	if (error) {
 		do_warn(_("Could not read AGF %u to check rmap btree.\n"),
 				agno);
-		goto err;
+		goto err_pag;
 	}
 
 	/* Leave the per-ag data "uninitialized" since we rewrite it later */
@@ -1013,7 +1013,7 @@ rmaps_verify_btree(
 	bt_cur = libxfs_rmapbt_init_cursor(mp, NULL, agbp, pag);
 	if (!bt_cur) {
 		do_warn(_("Not enough memory to check reverse mappings.\n"));
-		goto err;
+		goto err_bt_cur;
 	}
 
 	rm_rec = pop_slab_cursor(rm_cur);
@@ -1023,7 +1023,7 @@ rmaps_verify_btree(
 			do_warn(
 _("Could not read reverse-mapping record for (%u/%u).\n"),
 					agno, rm_rec->rm_startblock);
-			goto err;
+			goto err_loop;
 		}
 
 		/*
@@ -1039,7 +1039,7 @@ _("Could not read reverse-mapping record for (%u/%u).\n"),
 				do_warn(
 _("Could not read reverse-mapping record for (%u/%u).\n"),
 						agno, rm_rec->rm_startblock);
-				goto err;
+				goto err_loop;
 			}
 		}
 		if (!have) {
@@ -1090,13 +1090,12 @@ next_loop:
 		rm_rec = pop_slab_cursor(rm_cur);
 	}
 
-err:
-	if (bt_cur)
-		libxfs_btree_del_cursor(bt_cur, XFS_BTREE_NOERROR);
-	if (pag)
-		libxfs_perag_put(pag);
-	if (agbp)
-		libxfs_buf_relse(agbp);
+err_loop:
+	libxfs_btree_del_cursor(bt_cur, XFS_BTREE_NOERROR);
+err_bt_cur:
+	libxfs_buf_relse(agbp);
+err_pag:
+	libxfs_perag_put(pag);
 	free_slab_cursor(&rm_cur);
 }
 
-- 
2.30.2

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

[Darrick J. Wong]

On Mon, Nov 28, 2022 at 02:14:33PM +0100, cem@kernel.org 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>
> ---
> V2:
> 	- Rename error label from err_agf to err_pag
> 	- pass error directly to libxfs_btree_del_cursor() without
> 	  using ternary operator
> 
>  repair/rmap.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/repair/rmap.c b/repair/rmap.c
> index 2c809fd4f..e76a8f611 100644
> --- a/repair/rmap.c
> +++ b/repair/rmap.c
> @@ -1379,7 +1379,7 @@ check_refcounts(
>  	if (error) {
>  		do_warn(_("Could not read AGF %u to check refcount btree.\n"),
>  				agno);
> -		goto err;
> +		goto err_pag;
>  	}
>  
>  	/* Leave the per-ag data "uninitialized" since we rewrite it later */
> @@ -1388,7 +1388,7 @@ check_refcounts(
>  	bt_cur = libxfs_refcountbt_init_cursor(mp, NULL, agbp, pag);
>  	if (!bt_cur) {
>  		do_warn(_("Not enough memory to check refcount data.\n"));
> -		goto err;
> +		goto err_bt_cur;
>  	}
>  
>  	rl_rec = pop_slab_cursor(rl_cur);
> @@ -1401,7 +1401,7 @@ check_refcounts(
>  			do_warn(
>  _("Could not read reference count record for (%u/%u).\n"),
>  					agno, rl_rec->rc_startblock);
> -			goto err;
> +			goto err_loop;
>  		}
>  		if (!have) {
>  			do_warn(
> @@ -1416,7 +1416,7 @@ _("Missing reference count record for (%u/%u) len %u count %u\n"),
>  			do_warn(
>  _("Could not read reference count record for (%u/%u).\n"),
>  					agno, rl_rec->rc_startblock);
> -			goto err;
> +			goto err_loop;
>  		}
>  		if (!i) {
>  			do_warn(
> @@ -1446,14 +1446,12 @@ next_loop:
>  		rl_rec = pop_slab_cursor(rl_cur);
>  	}
>  
> -err:
> -	if (bt_cur)
> -		libxfs_btree_del_cursor(bt_cur, error ? XFS_BTREE_ERROR :
> -							XFS_BTREE_NOERROR);
> -	if (pag)
> -		libxfs_perag_put(pag);
> -	if (agbp)
> -		libxfs_buf_relse(agbp);
> +err_loop:
> +	libxfs_btree_del_cursor(bt_cur, error);
> +err_bt_cur:
> +	libxfs_buf_relse(agbp);
> +err_pag:
> +	libxfs_perag_put(pag);

So I see that you fixed one of the labels so that err_pag jumps to
releasing the perag pointer, but it's still the case that err_bt_cur
frees the AGF buffer, not the btree cursor; and that err_loop actually
frees the btree cursor.

--D

>  	free_slab_cursor(&rl_cur);
>  }
>  
> -- 
> 2.30.2
> 

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

[Carlos Maiolino]

On Mon, Nov 28, 2022 at 02:09:40PM -0800, Darrick J. Wong wrote:
> On Mon, Nov 28, 2022 at 02:14:33PM +0100, cem@kernel.org 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>
> > ---
> > V2:
> > 	- Rename error label from err_agf to err_pag
> > 	- pass error directly to libxfs_btree_del_cursor() without
> > 	  using ternary operator
> >
> >  repair/rmap.c | 22 ++++++++++------------
> >  1 file changed, 10 insertions(+), 12 deletions(-)
> >
> > diff --git a/repair/rmap.c b/repair/rmap.c
> > index 2c809fd4f..e76a8f611 100644
> > --- a/repair/rmap.c
> > +++ b/repair/rmap.c
> > @@ -1379,7 +1379,7 @@ check_refcounts(
> >  	if (error) {
> >  		do_warn(_("Could not read AGF %u to check refcount btree.\n"),
> >  				agno);
> > -		goto err;
> > +		goto err_pag;
> >  	}
> >
> >  	/* Leave the per-ag data "uninitialized" since we rewrite it later */
> > @@ -1388,7 +1388,7 @@ check_refcounts(
> >  	bt_cur = libxfs_refcountbt_init_cursor(mp, NULL, agbp, pag);
> >  	if (!bt_cur) {
> >  		do_warn(_("Not enough memory to check refcount data.\n"));
> > -		goto err;
> > +		goto err_bt_cur;
> >  	}
> >
> >  	rl_rec = pop_slab_cursor(rl_cur);
> > @@ -1401,7 +1401,7 @@ check_refcounts(
> >  			do_warn(
> >  _("Could not read reference count record for (%u/%u).\n"),
> >  					agno, rl_rec->rc_startblock);
> > -			goto err;
> > +			goto err_loop;
> >  		}
> >  		if (!have) {
> >  			do_warn(
> > @@ -1416,7 +1416,7 @@ _("Missing reference count record for (%u/%u) len %u count %u\n"),
> >  			do_warn(
> >  _("Could not read reference count record for (%u/%u).\n"),
> >  					agno, rl_rec->rc_startblock);
> > -			goto err;
> > +			goto err_loop;
> >  		}
> >  		if (!i) {
> >  			do_warn(
> > @@ -1446,14 +1446,12 @@ next_loop:
> >  		rl_rec = pop_slab_cursor(rl_cur);
> >  	}
> >
> > -err:
> > -	if (bt_cur)
> > -		libxfs_btree_del_cursor(bt_cur, error ? XFS_BTREE_ERROR :
> > -							XFS_BTREE_NOERROR);
> > -	if (pag)
> > -		libxfs_perag_put(pag);
> > -	if (agbp)
> > -		libxfs_buf_relse(agbp);
> > +err_loop:
> > +	libxfs_btree_del_cursor(bt_cur, error);
> > +err_bt_cur:
> > +	libxfs_buf_relse(agbp);
> > +err_pag:
> > +	libxfs_perag_put(pag);
> 
> So I see that you fixed one of the labels so that err_pag jumps to
> releasing the perag pointer, but it's still the case that err_bt_cur
> frees the AGF buffer, not the btree cursor; and that err_loop actually
> frees the btree cursor.

Totally true. I focused on your comments regarding err_pag, and forgot to review
the remaining labels. I'll fix it and send a V3.

Thanks for the review.

> 
> --D
> 
> >  	free_slab_cursor(&rl_cur);
> >  }
> >
> > --
> > 2.30.2
> >

-- 
Carlos Maiolino

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

[Carlos Maiolino]

> > > +err_loop:
> > > +	libxfs_btree_del_cursor(bt_cur, error);
> > > +err_bt_cur:
> > > +	libxfs_buf_relse(agbp);
> > > +err_pag:
> > > +	libxfs_perag_put(pag);
> >
> > So I see that you fixed one of the labels so that err_pag jumps to
> > releasing the perag pointer, but it's still the case that err_bt_cur
> > frees the AGF buffer, not the btree cursor; and that err_loop actually
> > frees the btree cursor.
> 
> Totally true. I focused on your comments regarding err_pag, and forgot to review
> the remaining labels. I'll fix it and send a V3.

Just to avoid unnecessary new versions :)
Are the fallowing names ok?

err_cur
err_agf
err_pag

Could be err_agbp too, but I'd rather be explicit this buffer belongs to the
agf.


> 
> Thanks for the review.
> 

-- 
Carlos Maiolino

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

[Darrick J. Wong]

On Wed, Nov 30, 2022 at 03:22:28PM +0100, Carlos Maiolino wrote:
> > > > +err_loop:
> > > > +	libxfs_btree_del_cursor(bt_cur, error);
> > > > +err_bt_cur:
> > > > +	libxfs_buf_relse(agbp);
> > > > +err_pag:
> > > > +	libxfs_perag_put(pag);
> > >
> > > So I see that you fixed one of the labels so that err_pag jumps to
> > > releasing the perag pointer, but it's still the case that err_bt_cur
> > > frees the AGF buffer, not the btree cursor; and that err_loop actually
> > > frees the btree cursor.
> > 
> > Totally true. I focused on your comments regarding err_pag, and forgot to review
> > the remaining labels. I'll fix it and send a V3.
> 
> Just to avoid unnecessary new versions :)
> Are the fallowing names ok?
> 
> err_cur
> err_agf
> err_pag

Yes, those are fine.  The label names reflect whatever gets cleaned up
immediately after the label.

> Could be err_agbp too, but I'd rather be explicit this buffer belongs to the
> agf.

Agreed.

--D

> 
> > 
> > Thanks for the review.
> > 
> 
> -- 
> Carlos Maiolino

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

[cem]

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>
---
V2:
	- Rename error label from err_agf to err_pag
	- pass error directly to libxfs_btree_del_cursor() without
	  using ternary operator
V3:
	- Rename the remaining 2 err labels to match what they are freeing

 repair/rmap.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/repair/rmap.c b/repair/rmap.c
index 2c809fd4f..9ec5e9e13 100644
--- a/repair/rmap.c
+++ b/repair/rmap.c
@@ -1379,7 +1379,7 @@ check_refcounts(
 	if (error) {
 		do_warn(_("Could not read AGF %u to check refcount btree.\n"),
 				agno);
-		goto err;
+		goto err_pag;
 	}
 
 	/* Leave the per-ag data "uninitialized" since we rewrite it later */
@@ -1388,7 +1388,7 @@ check_refcounts(
 	bt_cur = libxfs_refcountbt_init_cursor(mp, NULL, agbp, pag);
 	if (!bt_cur) {
 		do_warn(_("Not enough memory to check refcount data.\n"));
-		goto err;
+		goto err_agf;
 	}
 
 	rl_rec = pop_slab_cursor(rl_cur);
@@ -1401,7 +1401,7 @@ check_refcounts(
 			do_warn(
 _("Could not read reference count record for (%u/%u).\n"),
 					agno, rl_rec->rc_startblock);
-			goto err;
+			goto err_cur;
 		}
 		if (!have) {
 			do_warn(
@@ -1416,7 +1416,7 @@ _("Missing reference count record for (%u/%u) len %u count %u\n"),
 			do_warn(
 _("Could not read reference count record for (%u/%u).\n"),
 					agno, rl_rec->rc_startblock);
-			goto err;
+			goto err_cur;
 		}
 		if (!i) {
 			do_warn(
@@ -1446,14 +1446,12 @@ next_loop:
 		rl_rec = pop_slab_cursor(rl_cur);
 	}
 
-err:
-	if (bt_cur)
-		libxfs_btree_del_cursor(bt_cur, error ? XFS_BTREE_ERROR :
-							XFS_BTREE_NOERROR);
-	if (pag)
-		libxfs_perag_put(pag);
-	if (agbp)
-		libxfs_buf_relse(agbp);
+err_cur:
+	libxfs_btree_del_cursor(bt_cur, error);
+err_agf:
+	libxfs_buf_relse(agbp);
+err_pag:
+	libxfs_perag_put(pag);
 	free_slab_cursor(&rl_cur);
 }
 
-- 
2.30.2

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

[Darrick J. Wong]

On Thu, Dec 01, 2022 at 10:34:07AM +0100, cem@kernel.org 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>

Looks good to me,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
> V2:
> 	- Rename error label from err_agf to err_pag
> 	- pass error directly to libxfs_btree_del_cursor() without
> 	  using ternary operator
> V3:
> 	- Rename the remaining 2 err labels to match what they are freeing
> 
>  repair/rmap.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/repair/rmap.c b/repair/rmap.c
> index 2c809fd4f..9ec5e9e13 100644
> --- a/repair/rmap.c
> +++ b/repair/rmap.c
> @@ -1379,7 +1379,7 @@ check_refcounts(
>  	if (error) {
>  		do_warn(_("Could not read AGF %u to check refcount btree.\n"),
>  				agno);
> -		goto err;
> +		goto err_pag;
>  	}
>  
>  	/* Leave the per-ag data "uninitialized" since we rewrite it later */
> @@ -1388,7 +1388,7 @@ check_refcounts(
>  	bt_cur = libxfs_refcountbt_init_cursor(mp, NULL, agbp, pag);
>  	if (!bt_cur) {
>  		do_warn(_("Not enough memory to check refcount data.\n"));
> -		goto err;
> +		goto err_agf;
>  	}
>  
>  	rl_rec = pop_slab_cursor(rl_cur);
> @@ -1401,7 +1401,7 @@ check_refcounts(
>  			do_warn(
>  _("Could not read reference count record for (%u/%u).\n"),
>  					agno, rl_rec->rc_startblock);
> -			goto err;
> +			goto err_cur;
>  		}
>  		if (!have) {
>  			do_warn(
> @@ -1416,7 +1416,7 @@ _("Missing reference count record for (%u/%u) len %u count %u\n"),
>  			do_warn(
>  _("Could not read reference count record for (%u/%u).\n"),
>  					agno, rl_rec->rc_startblock);
> -			goto err;
> +			goto err_cur;
>  		}
>  		if (!i) {
>  			do_warn(
> @@ -1446,14 +1446,12 @@ next_loop:
>  		rl_rec = pop_slab_cursor(rl_cur);
>  	}
>  
> -err:
> -	if (bt_cur)
> -		libxfs_btree_del_cursor(bt_cur, error ? XFS_BTREE_ERROR :
> -							XFS_BTREE_NOERROR);
> -	if (pag)
> -		libxfs_perag_put(pag);
> -	if (agbp)
> -		libxfs_buf_relse(agbp);
> +err_cur:
> +	libxfs_btree_del_cursor(bt_cur, error);
> +err_agf:
> +	libxfs_buf_relse(agbp);
> +err_pag:
> +	libxfs_perag_put(pag);
>  	free_slab_cursor(&rl_cur);
>  }
>  
> -- 
> 2.30.2
>