Re: [PATCH 2/2] repair: fix some valgrind reported errors on i686

[Alex Elder <aelder@sgi.com>]

On Thu, 2011-10-06 at 12:01 +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Fix a potential prefetch read problem due to the first loop
> execution of pf_batch_read potentially not initialising the fsbno
> varaible:
> 
> ==10177== Thread 6:
> ==10177== Conditional jump or move depends on uninitialised value(s)
> ==10177==    at 0x8079CAB: pf_batch_read (prefetch.c:408)
> ==10177==    by 0x6A2996D: clone (clone.S:130)
> ==10177==
> 
> Fix a bunch of invalid read/write errors due to excessive blkmap
> allocations when inode forks are corrupted. These show up some time
> after making a blkmap allocation for 536870913 extents on i686,
> which is followed some time later by a crash caused bymemory
> corruption.
> 
> This blkmap allocation size overflows 32 bits in such a
> way that it results in a 32 byte allocation and so access to the
> second extent results in access beyond the allocated memory and
> corrupts random memory.
> 
> ==5419== Invalid write of size 4
> ==5419==    at 0x80507DA: blkmap_set_ext (bmap.c:260)
> ==5419==    by 0x8055CF4: process_bmbt_reclist_int (dinode.c:712)
> ==5419==    by 0x8056206: process_bmbt_reclist (dinode.c:813)
> ==5419==    by 0x80579DA: process_exinode (dinode.c:1324)
> ==5419==    by 0x8059B77: process_dinode_int (dinode.c:2036)
> ==5419==    by 0x805ABE6: process_dinode (dinode.c:2823)
> ==5419==    by 0x8052493: process_inode_chunk.isra.4 (dino_chunks.c:777)
> ==5419==    by 0x8054012: process_aginodes (dino_chunks.c:1024)
> ==5419==    by 0xFFF: ???
> ==5419==  Address 0x944cfb8 is 0 bytes after a block of size 32 alloc'd
> ==5419==    at 0x48E1102: realloc (in
> /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
> ==5419==    by 0x80501F3: blkmap_alloc (bmap.c:56)
> ==5419==    by 0x80599F5: process_dinode_int (dinode.c:2027)
> ==5419==    by 0x805ABE6: process_dinode (dinode.c:2823)
> ==5419==    by 0x8052493: process_inode_chunk.isra.4 (dino_chunks.c:777)
> ==5419==    by 0x8054012: process_aginodes (dino_chunks.c:1024)
> ==5419==    by 0xFFF: ???
> 
> Add overflow detection code into the blkmap allocation code to avoid
> this problem, and also free large allocations once they are finished
> with to avoid pinning large amounts of memory due to the occasional
> large extent list in a filesystem.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

This is good but I have a few comments below, a couple of
which really indicate you need to update this.

					-Alex
 
> ---
>  repair/bmap.c     |   37 ++++++++++++++++++++++++++++++++++++-
>  repair/prefetch.c |    2 +-
>  2 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/repair/bmap.c b/repair/bmap.c
> index 79b9f79..1127a87 100644
> --- a/repair/bmap.c
> +++ b/repair/bmap.c
> @@ -47,6 +47,17 @@ blkmap_alloc(
>  	if (nex < 1)
>  		nex = 1;
>  
> +#if (BITS_PER_LONG != 64)

This should be == 32, not != 64.  (And if it
were possible, sizeof (int) == 32.)

> +	if (nex > (INT_MAX / sizeof(bmap_ext_t) - 1)) {

See the comment below about this calculation.

> +		do_warn(
> +	_("Number of extents requested in blkmap_alloc (%u) overflows 32 bits.\n"
> +	  "If this is not a corruption, then will need a 64 bit system\n"
		...then you will need...

> +	  "to repair this filesystem.\n"),
> +			nex);
> +		return NULL;
> +	}
> +#endif
> +
>  	key = whichfork ? ablkmap_key : dblkmap_key;
>  	blkmap = pthread_getspecific(key);
>  	if (!blkmap || blkmap->naexts < nex) {

. . .

> @@ -218,6 +244,15 @@ blkmap_grow(
>  	}
>  
>  	blkmap->naexts += 4;

The check needs to go *before* you update naexts.

> +#if (BITS_PER_LONG != 64)
> +	if (blkmap->naexts > (INT_MAX / sizeof(bmap_ext_t) - 1)) {

I don't really follow this calculation. I would expect
it to be based more closely on how BLKMAP_SIZE() is
defined.

If you move it before the increment I think it would
be better to use:
	if (BLKMAP_SIZE(nex) >= INT_MAX - sizeof (blkent_t *))
And since this would expose the internals of what
BLKMAP_SIZE() does, it might be nicer if some sort of
BLKMAP_NENTS_MAX constant were defined next to the
definition of BLKMAP_SIZE().



> +		do_error(
> +	_("Number of extents requested in blkmap_grow (%u) overflows 32 bits.\n"
> +	  "You need a 64 bit system to repair this filesystem.\n"),
> +			blkmap->naexts);
> +		return NULL;
> +	}
> +#endif
>  	blkmap = realloc(blkmap, BLKMAP_SIZE(blkmap->naexts));
>  	if (blkmap == NULL)
>  		do_error(_("realloc failed in blkmap_grow\n"));


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