[PATCH 0/2] xfs_repair: clear new memory after realloc

[PATCH 0/2] xfs_repair: clear new memory after realloc

[Mike Grant]

This patchset fixes the seg-fault bug that was crashing an xfs_repair attempt
on a 150TB filesystem.  The problem, including traces and metadump link, is
in the mail thread "xfs_repair segfault + debug info" 29/May/2015.  After the
patch was applied, the xfs_repair ran through to completion.

There's also a cosmetic reformatting of nearby lines included, but this can be
dropped if preferred.

Mike Grant (2):
  xfs_repair: reformat lines to fit within 80 characters
  xfs_repair: clear new memory after realloc

 repair/phase6.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

-- 
2.1.0

(apologies for the following junk forcibly appended by my company)


Please visit our new website at www.pml.ac.uk and follow us on Twitter  @PlymouthMarine

Winner of the Environment & Conservation category, the Charity Awards 2014.

Plymouth Marine Laboratory (PML) is a company limited by guarantee registered in England & Wales, company number 4178503. Registered Charity No. 1091222. Registered Office: Prospect Place, The Hoe, Plymouth  PL1 3DH, UK. 

This message is private and confidential. If you have received this message in error, please notify the sender and remove it from your system. You are reminded that e-mail communications are not secure and may contain viruses; PML accepts no liability for any loss or damage which may be caused by viruses.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/2] xfs_repair: reformat lines to fit within 80 characters

[Mike Grant]

Signed-off-by: Mike Grant <mggr@pml.ac.uk>
---
 repair/phase6.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/repair/phase6.c b/repair/phase6.c
index 105bce4..0d66f9c 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -2327,9 +2327,11 @@ longform_dir2_entry_check(xfs_mount_t	*mp,
 		if (db >= num_bps) {
 			/* more data blocks than expected */
 			num_bps = db + 1;
-			bplist = realloc(bplist, num_bps * sizeof(struct xfs_buf*));
+			bplist = realloc(bplist, num_bps *
+				sizeof(struct xfs_buf*));
 			if (!bplist)
-				do_error(_("realloc failed in %s (%zu bytes)\n"),
+				do_error(
+					_("realloc failed in %s (%zu bytes)\n"),
 					__func__,
 					num_bps * sizeof(struct xfs_buf*));
 		}
-- 
2.1.0

(apologies for the following junk forcibly appended by my company)


Please visit our new website at www.pml.ac.uk and follow us on Twitter  @PlymouthMarine

Winner of the Environment & Conservation category, the Charity Awards 2014.

Plymouth Marine Laboratory (PML) is a company limited by guarantee registered in England & Wales, company number 4178503. Registered Charity No. 1091222. Registered Office: Prospect Place, The Hoe, Plymouth  PL1 3DH, UK. 

This message is private and confidential. If you have received this message in error, please notify the sender and remove it from your system. You are reminded that e-mail communications are not secure and may contain viruses; PML accepts no liability for any loss or damage which may be caused by viruses.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/2] xfs_repair: reformat lines to fit within 80 characters

[Brian Foster]

On Fri, Jun 19, 2015 at 10:21:35AM +0100, Mike Grant wrote:
> Signed-off-by: Mike Grant <mggr@pml.ac.uk>
> ---
>  repair/phase6.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/repair/phase6.c b/repair/phase6.c
> index 105bce4..0d66f9c 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -2327,9 +2327,11 @@ longform_dir2_entry_check(xfs_mount_t	*mp,
>  		if (db >= num_bps) {
>  			/* more data blocks than expected */
>  			num_bps = db + 1;
> -			bplist = realloc(bplist, num_bps * sizeof(struct xfs_buf*));
> +			bplist = realloc(bplist, num_bps *
> +				sizeof(struct xfs_buf*));

We usually try to align the split lines based on the "scope" within the
previous lines, for lack of a better term. For example, the above would
look something like this:

			bplist = realloc(bplist, num_bps *
					 sizeof(struct xfs_buf*));

... which is a bit easier to read at a glance. Anyways, Dave may or may
not pull this one and fix it up manually, but it's fine to me with the
above fix:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  			if (!bplist)
> -				do_error(_("realloc failed in %s (%zu bytes)\n"),
> +				do_error(
> +					_("realloc failed in %s (%zu bytes)\n"),
>  					__func__,
>  					num_bps * sizeof(struct xfs_buf*));
>  		}
> -- 
> 2.1.0
> 
> (apologies for the following junk forcibly appended by my company)
> 
> 
> Please visit our new website at www.pml.ac.uk and follow us on Twitter  @PlymouthMarine
> 
> Winner of the Environment & Conservation category, the Charity Awards 2014.
> 
> Plymouth Marine Laboratory (PML) is a company limited by guarantee registered in England & Wales, company number 4178503. Registered Charity No. 1091222. Registered Office: Prospect Place, The Hoe, Plymouth  PL1 3DH, UK. 
> 
> This message is private and confidential. If you have received this message in error, please notify the sender and remove it from your system. You are reminded that e-mail communications are not secure and may contain viruses; PML accepts no liability for any loss or damage which may be caused by viruses.
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/2] xfs_repair: clear new memory after realloc

[Mike Grant]

longform_dir2_entry_check in phase6.c has a calloc'ed array of xfs_buf
pointers (bplist).  It reallocs this array if there turns out to be more
blocks than expected.  However, realloc doesn't zero the new memory like
calloc.  In unusual circumstances*, things may then blow up later due to
random data populating the new part of the array.

This patch zeros the new part of the array.

* (bit speculative) as dir_read_buf zeros the element it's looking at, I
think this can only happen if the realloc adds several members and one
of the first is corrupt.  In my case, the realloc went from 35 to 37
members, meaning db must have been 36 without being 35.  A read error
then caused it to goto out_fix. The crash then occurred in the
libxfs_putbuf when looping through the bplist structure, checking it for
NULL pointers (and presumably tripping over the non-zeroed data at
position 35?)

Signed-off-by: Mike Grant <mggr@pml.ac.uk>
---
 repair/phase6.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/repair/phase6.c b/repair/phase6.c
index 0d66f9c..4dd7551 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -2326,6 +2326,7 @@ longform_dir2_entry_check(xfs_mount_t	*mp,
 		db = xfs_dir2_da_to_db(mp, da_bno);
 		if (db >= num_bps) {
 			/* more data blocks than expected */
+			int num_bps_prev = num_bps;
 			num_bps = db + 1;
 			bplist = realloc(bplist, num_bps *
 				sizeof(struct xfs_buf*));
@@ -2334,6 +2335,10 @@ longform_dir2_entry_check(xfs_mount_t	*mp,
 					_("realloc failed in %s (%zu bytes)\n"),
 					__func__,
 					num_bps * sizeof(struct xfs_buf*));
+			/* clear new memory as previous bplist was calloc'ed */
+			memset((void *) bplist + num_bps_prev
+				* sizeof(struct xfs_buf*), 0, (num_bps -
+				num_bps_prev) * sizeof(struct xfs_buf*));
 		}
 
 		if (isblock)
-- 
2.1.0

(apologies for the following junk forcibly appended by my company)


Please visit our new website at www.pml.ac.uk and follow us on Twitter  @PlymouthMarine

Winner of the Environment & Conservation category, the Charity Awards 2014.

Plymouth Marine Laboratory (PML) is a company limited by guarantee registered in England & Wales, company number 4178503. Registered Charity No. 1091222. Registered Office: Prospect Place, The Hoe, Plymouth  PL1 3DH, UK. 

This message is private and confidential. If you have received this message in error, please notify the sender and remove it from your system. You are reminded that e-mail communications are not secure and may contain viruses; PML accepts no liability for any loss or damage which may be caused by viruses.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs_repair: clear new memory after realloc

[Brian Foster]

On Fri, Jun 19, 2015 at 10:21:36AM +0100, Mike Grant wrote:
> longform_dir2_entry_check in phase6.c has a calloc'ed array of xfs_buf
> pointers (bplist).  It reallocs this array if there turns out to be more
> blocks than expected.  However, realloc doesn't zero the new memory like
> calloc.  In unusual circumstances*, things may then blow up later due to
> random data populating the new part of the array.
> 
> This patch zeros the new part of the array.
> 
> * (bit speculative) as dir_read_buf zeros the element it's looking at, I
> think this can only happen if the realloc adds several members and one
> of the first is corrupt.  In my case, the realloc went from 35 to 37
> members, meaning db must have been 36 without being 35.  A read error
> then caused it to goto out_fix. The crash then occurred in the
> libxfs_putbuf when looping through the bplist structure, checking it for
> NULL pointers (and presumably tripping over the non-zeroed data at
> position 35?)
> 
> Signed-off-by: Mike Grant <mggr@pml.ac.uk>
> ---
>  repair/phase6.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/repair/phase6.c b/repair/phase6.c
> index 0d66f9c..4dd7551 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -2326,6 +2326,7 @@ longform_dir2_entry_check(xfs_mount_t	*mp,
>  		db = xfs_dir2_da_to_db(mp, da_bno);
>  		if (db >= num_bps) {
>  			/* more data blocks than expected */
> +			int num_bps_prev = num_bps;
>  			num_bps = db + 1;
>  			bplist = realloc(bplist, num_bps *
>  				sizeof(struct xfs_buf*));
> @@ -2334,6 +2335,10 @@ longform_dir2_entry_check(xfs_mount_t	*mp,
>  					_("realloc failed in %s (%zu bytes)\n"),
>  					__func__,
>  					num_bps * sizeof(struct xfs_buf*));
> +			/* clear new memory as previous bplist was calloc'ed */
> +			memset((void *) bplist + num_bps_prev
> +				* sizeof(struct xfs_buf*), 0, (num_bps -
> +				num_bps_prev) * sizeof(struct xfs_buf*));

Similar issue here with how the lines are split. In looking at it, I'm
not even sure how I would split this one up cleanly tbh.. ;) Maybe we
should just create another local variable for the bp size and clean up
the whole hunk. E.g.:

			int bpsz = sizeof(struct xfs_buf *);

			...

			memset((void *)bplist + num_bps_prev * bpsz, 0,
				(num_bps - num_bps_prev) * bpsz);

In fact, we could include bpsz in the first patch.

Brian

>  		}
>  
>  		if (isblock)
> -- 
> 2.1.0
> 
> (apologies for the following junk forcibly appended by my company)
> 
> 
> Please visit our new website at www.pml.ac.uk and follow us on Twitter  @PlymouthMarine
> 
> Winner of the Environment & Conservation category, the Charity Awards 2014.
> 
> Plymouth Marine Laboratory (PML) is a company limited by guarantee registered in England & Wales, company number 4178503. Registered Charity No. 1091222. Registered Office: Prospect Place, The Hoe, Plymouth  PL1 3DH, UK. 
> 
> This message is private and confidential. If you have received this message in error, please notify the sender and remove it from your system. You are reminded that e-mail communications are not secure and may contain viruses; PML accepts no liability for any loss or damage which may be caused by viruses.
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs