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

[PATCH 2/2] xfs_repair: Fix rmaps_verify_btree() 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: 1512654

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 repair/rmap.c |   21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/repair/rmap.c b/repair/rmap.c
index 0253c0c36..8b76e290b 100644
--- a/repair/rmap.c
+++ b/repair/rmap.c
@@ -1002,7 +1002,7 @@ rmaps_verify_btree(
 	if (error) {
 		do_warn(_("Could not read AGF %u to check rmap btree.\n"),
 				agno);
-		goto err;
+		goto err_agf;
 	}
 
 	/* Leave the per-ag data "uninitialized" since we rewrite it later */
@@ -1011,7 +1011,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);
@@ -1021,7 +1021,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;
 		}
 
 		/*
@@ -1037,7 +1037,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) {
@@ -1088,13 +1088,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_agf:
+	libxfs_perag_put(pag);
 	free_slab_cursor(&rm_cur);
 }
 

[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 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
V3:
	- Rename the remaining 2 err labels to match what they are freeing

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

diff --git a/repair/rmap.c b/repair/rmap.c
index 9ec5e9e13..52106fd42 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_agf;
 	}
 
 	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_cur;
 		}
 
 		/*
@@ -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_cur;
 			}
 		}
 		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_cur:
+	libxfs_btree_del_cursor(bt_cur, XFS_BTREE_NOERROR);
+err_agf:
+	libxfs_buf_relse(agbp);
+err_pag:
+	libxfs_perag_put(pag);
 	free_slab_cursor(&rm_cur);
 }
 
-- 
2.30.2

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

[Darrick J. Wong]

On Thu, Dec 01, 2022 at 10:34:08AM +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: 1512654
> 
> 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
> V3:
> 	- Rename the remaining 2 err labels to match what they are freeing
> 
>  repair/rmap.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/repair/rmap.c b/repair/rmap.c
> index 9ec5e9e13..52106fd42 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_agf;
>  	}
>  
>  	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_cur;
>  		}
>  
>  		/*
> @@ -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_cur;
>  			}
>  		}
>  		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_cur:
> +	libxfs_btree_del_cursor(bt_cur, XFS_BTREE_NOERROR);
> +err_agf:
> +	libxfs_buf_relse(agbp);
> +err_pag:
> +	libxfs_perag_put(pag);
>  	free_slab_cursor(&rm_cur);
>  }
>  
> -- 
> 2.30.2
>