[PATCH v2 0/3] clear new memory after realloc

[PATCH v2 0/3] clear new memory after realloc

[Mike Grant]

v2
 - updated indentation per Brian Foster's suggestions
 - added a patch to correct the finalisation of the potentially-realloc'ed
   bplist, following IRC discussion with Brian

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.

The final for loops at the end of the function containing the realloc didn't
account for any extra buffers that might have been added; this is corrected.

A cosmetic reformatting of nearby lines is included, using a constant to reduce
line lengths.


Mike Grant (3):
  xfs_repair: reformat lines to fit within 80 characters
  xfs_repair: clear new memory after realloc
  xfs_repair: include any realloc'ed buffers in final putbuf

 repair/phase6.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 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 v2 1/3] xfs_repair: reformat lines to fit within 80 characters

[Mike Grant]

Also introduces a const to improve readability

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

diff --git a/repair/phase6.c b/repair/phase6.c
index 105bce4..6520196 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -2267,6 +2267,7 @@ longform_dir2_entry_check(xfs_mount_t	*mp,
 			dir_hash_tab_t	*hashtab)
 {
 	struct xfs_buf		**bplist;
+	const size_t 		bpsz = sizeof(struct xfs_buf*);
 	xfs_dablk_t		da_bno;
 	freetab_t		*freetab;
 	int			num_bps;
@@ -2293,10 +2294,10 @@ longform_dir2_entry_check(xfs_mount_t	*mp,
 		freetab->ents[i].s = 0;
 	}
 	num_bps = freetab->naents;
-	bplist = calloc(num_bps, sizeof(struct xfs_buf*));
+	bplist = calloc(num_bps, bpsz);
 	if (!bplist)
 		do_error(_("calloc failed in %s (%zu bytes)\n"),
-			__func__, num_bps * sizeof(struct xfs_buf*));
+			__func__, num_bps * bpsz);
 
 	/* is this a block, leaf, or node directory? */
 	libxfs_dir2_isblock(NULL, ip, &isblock);
@@ -2327,11 +2328,12 @@ 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 * bpsz);
 			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*));
+					num_bps * bpsz);
 		}
 
 		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

[PATCH v2 2/3] 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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/repair/phase6.c b/repair/phase6.c
index 6520196..e24cf62 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -2327,6 +2327,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 * bpsz);
 			if (!bplist)
@@ -2334,6 +2335,9 @@ longform_dir2_entry_check(xfs_mount_t	*mp,
 					_("realloc failed in %s (%zu bytes)\n"),
 					__func__,
 					num_bps * bpsz);
+			/* clear new memory as previous bplist was calloc'ed */
+			memset((void *) bplist + num_bps_prev * bpsz, 0,
+				(num_bps - num_bps_prev) * bpsz);
 		}
 
 		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

[PATCH v2 3/3] xfs_repair: include any realloc'ed buffers in final putbuf

[Mike Grant]

The realloc code included in commit 95dff16b1 potentially introduces
extra buffers to bplist.  These should be dealt with at the end of the
longform_dir2_entry_check function.  This replaces the originally estimated
number of entries (freetab->naents) with the actual number finally allocated
(num_bps).

Note: this has not been tested on a suitable filesystem image but
appears correct..

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

diff --git a/repair/phase6.c b/repair/phase6.c
index e24cf62..7f7d9b0 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -2409,14 +2409,14 @@ longform_dir2_entry_check(xfs_mount_t	*mp,
 out_fix:
 	if (!no_modify && (fixit || dotdot_update)) {
 		dir_hash_dup_names(hashtab);
-		for (i = 0; i < freetab->naents; i++)
+		for (i = 0; i < num_bps; i++)
 			if (bplist[i])
 				libxfs_putbuf(bplist[i]);
 		longform_dir2_rebuild(mp, ino, ip, irec, ino_offset, hashtab);
 		*num_illegal = 0;
 		*need_dot = 0;
 	} else {
-		for (i = 0; i < freetab->naents; i++)
+		for (i = 0; i < num_bps; i++)
 			if (bplist[i])
 				libxfs_putbuf(bplist[i]);
 	}
-- 
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 v2 1/3] xfs_repair: reformat lines to fit within 80 characters

[Christoph Hellwig]

On Fri, Jun 19, 2015 at 04:03:59PM +0100, Mike Grant wrote:
> Also introduces a const to improve readability

Wouldn't just using sizeof(*bplist) be easier?

>  {
>  	struct xfs_buf		**bplist;
> +	const size_t 		bpsz = sizeof(struct xfs_buf*);

Also if we stay with this version please add a space before the star,
e.g.

	const size_t		bpsz = sizeof(struct xfs_buf *);

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

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

[Christoph Hellwig]

Can you add a new realloc_zeroed helper?, e.g.

void *
realloc_zeroed(void *ptr, size_t old_size, size_t new_size)
{
	ptr = realloc(ptr, new_size);
	if (ptr)
		memset(ptr + old_size, 0, new_size - old_size);
	return ptr;
}

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

Re: [PATCH v2 3/3] xfs_repair: include any realloc'ed buffers in final putbuf

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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