[PATCH 0/2] xfsdump: fix problems in cb_add_inogrp

[PATCH 0/2] xfsdump: fix problems in cb_add_inogrp

[rjohnston]

The memset in cb_add_inogrp will segfault when the index oldsize
overflows. In cb_add_inogrp(), the temp variables used in
calculating the new i2gmap segment offset should be int64 instead
of intgen_t (int32).

A second bug also occurs because we already compensate for the
length of each item in oldsize so are 32bit wrap becomes a 40bit
wrap.

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

[PATCH 1/2] xfsdump: use 64bit local variables in inode.c

[rjohnston]

[-- Attachment #1: xfsdump-use-64bit-local-variables-in-inode-c.patch --]
[-- Type: text/plain, Size: 4212 bytes --]

The memset in cb_add_inogrp will segfault when the index oldsize
overflows. In cb_add_inogrp(), the temp variables used in
calculating the new i2gmap segment offset should be int64 instead
of intgen_t (int32).

Fix this:
  a. simplify the calculation of oldsize by moving it
     before hnkmaplen is incremented.
  b. change the index variables int64 to prevent overflow. 

Signed-off-by: Rich Johnston <rjohnston@sgi.com>
---
 dump/inomap.c |   41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

Index: b/dump/inomap.c
===================================================================
--- a/dump/inomap.c
+++ b/dump/inomap.c
@@ -71,7 +71,7 @@ static intgen_t cb_context( bool_t last,
 			    drange_t *,
 			    startpt_t *,
 			    size_t,
-			    intgen_t,
+			    int64_t,
 			    bool_t,
 			    bool_t *);
 static void cb_context_free( void );
@@ -96,7 +96,7 @@ static off64_t estimate_dump_space( xfs_
 
 /* inomap primitives
  */
-static intgen_t inomap_init( intgen_t igrpcnt );
+static intgen_t inomap_init( int64_t igrpcnt );
 static void inomap_add( void *, xfs_ino_t ino, gen_t gen, intgen_t );
 static intgen_t inomap_set_state( void *, xfs_ino_t ino, intgen_t );
 static void inomap_set_gen(void *, xfs_ino_t, gen_t );
@@ -160,7 +160,7 @@ inomap_build( jdm_fshandle_t *fshandlep,
 	xfs_bstat_t *bstatbufp;
 	size_t bstatbuflen;
 	bool_t pruneneeded = BOOL_FALSE;
-	intgen_t igrpcnt = 0;
+	int64_t igrpcnt = 0;
 	intgen_t stat;
 	intgen_t rval;
 
@@ -449,7 +449,7 @@ cb_context( bool_t last,
 	    drange_t *resumerangep,
 	    startpt_t *startptp,
 	    size_t startptcnt,
-	    intgen_t igrpcnt,
+	    int64_t igrpcnt,
 	    bool_t skip_unchanged_dirs,
 	    bool_t *pruneneededp )
 {
@@ -949,14 +949,14 @@ struct i2gseg {
 typedef struct i2gseg i2gseg_t;
 
 typedef struct seg_addr {
-	intgen_t hnkoff;
-	intgen_t segoff;
-	intgen_t inooff;
+	int64_t hnkoff;
+	int64_t segoff;
+	int64_t inooff;
 } seg_addr_t;
 
 static struct inomap {
 	hnk_t *hnkmap;
-	intgen_t hnkmaplen;
+	int64_t hnkmaplen;
 	i2gseg_t *i2gmap;
 	seg_addr_t lastseg;
 } inomap;
@@ -1040,7 +1040,7 @@ SEG_GET_BITS( seg_t *segp, xfs_ino_t ino
 /* context for inomap construction - initialized by map_init
  */
 static intgen_t
-inomap_init( intgen_t igrpcnt )
+inomap_init( int64_t igrpcnt )
 {
 	ASSERT( sizeof( hnk_t ) == HNKSZ );
 
@@ -1066,7 +1066,7 @@ inomap_getsz( void )
 static inline bool_t
 inomap_validaddr( seg_addr_t *addrp )
 {
-	intgen_t maxseg;
+	int64_t maxseg;
 
 	if ( addrp->hnkoff < 0 || addrp->hnkoff > inomap.lastseg.hnkoff )
 		return BOOL_FALSE;
@@ -1093,13 +1093,13 @@ inomap_addr2seg( seg_addr_t *addrp )
 	return &hunkp->seg[addrp->segoff];
 }
 
-static inline intgen_t
+static inline int64_t
 inomap_addr2segix( seg_addr_t *addrp )
 {
 	return ( addrp->hnkoff * SEGPERHNK ) + addrp->segoff;
 }
 
-static inline intgen_t
+static inline int64_t
 inomap_lastseg( intgen_t hnkoff )
 {
 	if ( hnkoff == inomap.lastseg.hnkoff )
@@ -1125,8 +1125,11 @@ cb_add_inogrp( void *arg1, intgen_t fsfd
 		lastsegp->segoff = 0;
 
 		if (lastsegp->hnkoff == inomap.hnkmaplen) {
-			intgen_t numsegs;
-			intgen_t oldsize;
+			int64_t numsegs;
+			int64_t oldsize;
+
+			oldsize = inomap.hnkmaplen * SEGPERHNK
+				  * sizeof(i2gseg_t);
 
 			inomap.hnkmaplen++;
 			inomap.hnkmap = (hnk_t *)
@@ -1140,8 +1143,6 @@ cb_add_inogrp( void *arg1, intgen_t fsfd
 				return -1;
 
 			/* zero the new portion of the i2gmap */
-			oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
-
 			memset(inomap.i2gmap + oldsize,
 			       0,
 			       SEGPERHNK * sizeof(i2gseg_t));
@@ -1199,8 +1200,8 @@ static bool_t
 inomap_find_hnk( seg_addr_t *addrp, xfs_ino_t ino )
 {
 	hnk_t *hunkp;
-	intgen_t lower;
-	intgen_t upper;
+	int64_t lower;
+	int64_t upper;
 
 	lower = 0;
 	upper = inomap.lastseg.hnkoff;
@@ -1231,8 +1232,8 @@ static bool_t
 inomap_find_seg( seg_addr_t *addrp, xfs_ino_t ino )
 {
 	seg_t *segp;
-	intgen_t lower;
-	intgen_t upper;
+	int64_t lower;
+	int64_t upper;
 
 	if ( !inomap_validaddr( addrp ) ) {
 		inomap_reset_context( addrp );

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

[PATCH 2/2] xfsdump: don't do pointer math twice

[rjohnston]

[-- Attachment #1: fix-pointer-math-in-cb_add_inogrp.patch --]
[-- Type: text/plain, Size: 1108 bytes --]

The pointer math when calculating the address for the call to memset
is incorrect, so we are clearing the wrong memory location.

i2gmap is of type i2gseg_t 

oldsize has already computed the pointer offset
	oldsize = inomap.hnkmaplen * SEGPERHNK * sizeof(i2gseg_t);
the memset call is using
	inomap.i2gmap + oldsize == &inomap.i2gmap[oldsize]
so we were doing the pointer math twice.

We already compensate for the length of each item in oldsize so
adding need to add a (char *) cast to the memset parameter.

---
 dump/inomap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: b/dump/inomap.c
===================================================================
--- a/dump/inomap.c
+++ b/dump/inomap.c
@@ -1143,7 +1143,7 @@ cb_add_inogrp( void *arg1, intgen_t fsfd
 				return -1;
 
 			/* zero the new portion of the i2gmap */
-			memset(inomap.i2gmap + oldsize,
+			memset((char *)inomap.i2gmap + oldsize,
 			       0,
 			       SEGPERHNK * sizeof(i2gseg_t));
 		}

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

Re: [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp

[Eric Sandeen]

On 8/21/15 9:01 AM, rjohnston@sgi.com wrote:
> The memset in cb_add_inogrp will segfault when the index oldsize
> overflows. In cb_add_inogrp(), the temp variables used in
> calculating the new i2gmap segment offset should be int64 instead
> of intgen_t (int32).
> 
> A second bug also occurs because we already compensate for the
> length of each item in oldsize so are 32bit wrap becomes a 40bit
> wrap.

Hi -

Are there any testcases for these?  xfsdump is alien code, I swear;
I'm not quite sure offhand how to tickle any of these bugs.

Thanks,
-Eric

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

Re: [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp

[Rich Johnston]

On 08/21/2015 10:47 AM, Eric Sandeen wrote:
> On 8/21/15 9:01 AM, rjohnston@sgi.com wrote:
>> The memset in cb_add_inogrp will segfault when the index oldsize
>> overflows. In cb_add_inogrp(), the temp variables used in
>> calculating the new i2gmap segment offset should be int64 instead
>> of intgen_t (int32).
>>
>> A second bug also occurs because we already compensate for the
>> length of each item in oldsize so are 32bit wrap becomes a 40bit
>> wrap.
>
> Hi -
>
> Are there any testcases for these?  xfsdump is alien code, I swear;
> I'm not quite sure offhand how to tickle any of these bugs.
>
> Thanks,
> -Eric
>
No I thought simple examination shows the bug.
It was a customer bug.

The number of inodes that we needed before wrapping was a couple hundred 
inodes.

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

Re: [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp

[Eric Sandeen]

On 8/21/15 11:38 AM, Rich Johnston wrote:
> On 08/21/2015 10:47 AM, Eric Sandeen wrote:
>> On 8/21/15 9:01 AM, rjohnston@sgi.com wrote:
>>> The memset in cb_add_inogrp will segfault when the index oldsize
>>> overflows. In cb_add_inogrp(), the temp variables used in
>>> calculating the new i2gmap segment offset should be int64 instead
>>> of intgen_t (int32).
>>>
>>> A second bug also occurs because we already compensate for the
>>> length of each item in oldsize so are 32bit wrap becomes a 40bit
>>> wrap.
>>
>> Hi -
>>
>> Are there any testcases for these?  xfsdump is alien code, I swear;
>> I'm not quite sure offhand how to tickle any of these bugs.
>>
>> Thanks,
>> -Eric
>>
> No I thought simple examination shows the bug.

Nothing is simple in xfsdump, IMHO.  At least to the uninitiated.  :)

> It was a customer bug.
> 
> The number of inodes that we needed before wrapping was a couple hundred inodes.

I did eventually manage to hit the segfault, thanks.

-Eric

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

Re: [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp

[Rich Johnston]

On 08/21/2015 11:39 AM, Eric Sandeen wrote:
> On 8/21/15 11:38 AM, Rich Johnston wrote:
>> On 08/21/2015 10:47 AM, Eric Sandeen wrote:
>>> On 8/21/15 9:01 AM, rjohnston@sgi.com wrote:
>>>> The memset in cb_add_inogrp will segfault when the index oldsize
>>>> overflows. In cb_add_inogrp(), the temp variables used in
>>>> calculating the new i2gmap segment offset should be int64 instead
>>>> of intgen_t (int32).
>>>>
>>>> A second bug also occurs because we already compensate for the
>>>> length of each item in oldsize so are 32bit wrap becomes a 40bit
>>>> wrap.
>>>
>>> Hi -
>>>
>>> Are there any testcases for these?  xfsdump is alien code, I swear;
>>> I'm not quite sure offhand how to tickle any of these bugs.
>>>
>>> Thanks,
>>> -Eric
>>>
>> No I thought simple examination shows the bug.
>
> Nothing is simple in xfsdump, IMHO.  At least to the uninitiated.  :)
>
:)
>> It was a customer bug.
>>
>> The number of inodes that we needed before wrapping was a couple hundred inodes.
                                                                     ^^^^
make that *million*
>
> I did eventually manage to hit the segfault, thanks.
>
> -Eric
>

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

Re: [PATCH 1/2] xfsdump: use 64bit local variables in inode.c

[Eric Sandeen]

On 8/21/15 9:01 AM, rjohnston@sgi.com wrote:
> The memset in cb_add_inogrp will segfault when the index oldsize
> overflows. In cb_add_inogrp(), the temp variables used in
> calculating the new i2gmap segment offset should be int64 instead
> of intgen_t (int32).
> 
> Fix this:
>   a. simplify the calculation of oldsize by moving it
>      before hnkmaplen is incremented.

I think it'd make more sense to put a) in the second patch; it's
unrelated to the variable size changes.

Still wrapping my head around what these variables are for, but
makes sense at first glance.

Thanks,
-Eric

>   b. change the index variables int64 to prevent overflow. 
> 
> Signed-off-by: Rich Johnston <rjohnston@sgi.com>
> ---
>  dump/inomap.c |   41 +++++++++++++++++++++--------------------
>  1 file changed, 21 insertions(+), 20 deletions(-)
> 
> Index: b/dump/inomap.c
> ===================================================================
> --- a/dump/inomap.c
> +++ b/dump/inomap.c
> @@ -71,7 +71,7 @@ static intgen_t cb_context( bool_t last,
>  			    drange_t *,
>  			    startpt_t *,
>  			    size_t,
> -			    intgen_t,
> +			    int64_t,
>  			    bool_t,
>  			    bool_t *);
>  static void cb_context_free( void );
> @@ -96,7 +96,7 @@ static off64_t estimate_dump_space( xfs_
>  
>  /* inomap primitives
>   */
> -static intgen_t inomap_init( intgen_t igrpcnt );
> +static intgen_t inomap_init( int64_t igrpcnt );
>  static void inomap_add( void *, xfs_ino_t ino, gen_t gen, intgen_t );
>  static intgen_t inomap_set_state( void *, xfs_ino_t ino, intgen_t );
>  static void inomap_set_gen(void *, xfs_ino_t, gen_t );
> @@ -160,7 +160,7 @@ inomap_build( jdm_fshandle_t *fshandlep,
>  	xfs_bstat_t *bstatbufp;
>  	size_t bstatbuflen;
>  	bool_t pruneneeded = BOOL_FALSE;
> -	intgen_t igrpcnt = 0;
> +	int64_t igrpcnt = 0;
>  	intgen_t stat;
>  	intgen_t rval;
>  
> @@ -449,7 +449,7 @@ cb_context( bool_t last,
>  	    drange_t *resumerangep,
>  	    startpt_t *startptp,
>  	    size_t startptcnt,
> -	    intgen_t igrpcnt,
> +	    int64_t igrpcnt,
>  	    bool_t skip_unchanged_dirs,
>  	    bool_t *pruneneededp )
>  {
> @@ -949,14 +949,14 @@ struct i2gseg {
>  typedef struct i2gseg i2gseg_t;
>  
>  typedef struct seg_addr {
> -	intgen_t hnkoff;
> -	intgen_t segoff;
> -	intgen_t inooff;
> +	int64_t hnkoff;
> +	int64_t segoff;
> +	int64_t inooff;
>  } seg_addr_t;
>  
>  static struct inomap {
>  	hnk_t *hnkmap;
> -	intgen_t hnkmaplen;
> +	int64_t hnkmaplen;
>  	i2gseg_t *i2gmap;
>  	seg_addr_t lastseg;
>  } inomap;
> @@ -1040,7 +1040,7 @@ SEG_GET_BITS( seg_t *segp, xfs_ino_t ino
>  /* context for inomap construction - initialized by map_init
>   */
>  static intgen_t
> -inomap_init( intgen_t igrpcnt )
> +inomap_init( int64_t igrpcnt )
>  {
>  	ASSERT( sizeof( hnk_t ) == HNKSZ );
>  
> @@ -1066,7 +1066,7 @@ inomap_getsz( void )
>  static inline bool_t
>  inomap_validaddr( seg_addr_t *addrp )
>  {
> -	intgen_t maxseg;
> +	int64_t maxseg;
>  
>  	if ( addrp->hnkoff < 0 || addrp->hnkoff > inomap.lastseg.hnkoff )
>  		return BOOL_FALSE;
> @@ -1093,13 +1093,13 @@ inomap_addr2seg( seg_addr_t *addrp )
>  	return &hunkp->seg[addrp->segoff];
>  }
>  
> -static inline intgen_t
> +static inline int64_t
>  inomap_addr2segix( seg_addr_t *addrp )
>  {
>  	return ( addrp->hnkoff * SEGPERHNK ) + addrp->segoff;
>  }
>  
> -static inline intgen_t
> +static inline int64_t
>  inomap_lastseg( intgen_t hnkoff )
>  {
>  	if ( hnkoff == inomap.lastseg.hnkoff )
> @@ -1125,8 +1125,11 @@ cb_add_inogrp( void *arg1, intgen_t fsfd
>  		lastsegp->segoff = 0;
>  
>  		if (lastsegp->hnkoff == inomap.hnkmaplen) {
> -			intgen_t numsegs;
> -			intgen_t oldsize;
> +			int64_t numsegs;
> +			int64_t oldsize;
> +
> +			oldsize = inomap.hnkmaplen * SEGPERHNK
> +				  * sizeof(i2gseg_t);
>  
>  			inomap.hnkmaplen++;
>  			inomap.hnkmap = (hnk_t *)
> @@ -1140,8 +1143,6 @@ cb_add_inogrp( void *arg1, intgen_t fsfd
>  				return -1;
>  
>  			/* zero the new portion of the i2gmap */
> -			oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
> -
>  			memset(inomap.i2gmap + oldsize,
>  			       0,
>  			       SEGPERHNK * sizeof(i2gseg_t));
> @@ -1199,8 +1200,8 @@ static bool_t
>  inomap_find_hnk( seg_addr_t *addrp, xfs_ino_t ino )
>  {
>  	hnk_t *hunkp;
> -	intgen_t lower;
> -	intgen_t upper;
> +	int64_t lower;
> +	int64_t upper;
>  
>  	lower = 0;
>  	upper = inomap.lastseg.hnkoff;
> @@ -1231,8 +1232,8 @@ static bool_t
>  inomap_find_seg( seg_addr_t *addrp, xfs_ino_t ino )
>  {
>  	seg_t *segp;
> -	intgen_t lower;
> -	intgen_t upper;
> +	int64_t lower;
> +	int64_t upper;
>  
>  	if ( !inomap_validaddr( addrp ) ) {
>  		inomap_reset_context( addrp );
> 
> _______________________________________________
> 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

Re: [PATCH 2/2] xfsdump: don't do pointer math twice

[Eric Sandeen]

On 8/21/15 9:01 AM, rjohnston@sgi.com wrote:
> The pointer math when calculating the address for the call to memset
> is incorrect, so we are clearing the wrong memory location.
> 
> i2gmap is of type i2gseg_t 
> 
> oldsize has already computed the pointer offset
> 	oldsize = inomap.hnkmaplen * SEGPERHNK * sizeof(i2gseg_t);
> the memset call is using
> 	inomap.i2gmap + oldsize == &inomap.i2gmap[oldsize]
> so we were doing the pointer math twice.
> 
> We already compensate for the length of each item in oldsize so
> adding need to add a (char *) cast to the memset parameter.


What about just doing:

			memset(&inomap.i2gmap[numsegs - SEGPERHNK], 0,
 			       SEGPERHNK * sizeof(i2gseg_t));

which more clearly shows that we're setting the new array members
to zero.

(could do oldsegs = inomap.hnkmaplen * SEGPERHNK; prior to the
hnkmaplen++, if that makes it any more readable...)

*shrug* it seems a little clearer to me, anyway.

Thanks,
-Eric

> ---
>  dump/inomap.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: b/dump/inomap.c
> ===================================================================
> --- a/dump/inomap.c
> +++ b/dump/inomap.c
> @@ -1143,7 +1143,7 @@ cb_add_inogrp( void *arg1, intgen_t fsfd
>  				return -1;
>  
>  			/* zero the new portion of the i2gmap */
> -			memset(inomap.i2gmap + oldsize,
> +			memset((char *)inomap.i2gmap + oldsize,
>  			       0,
>  			       SEGPERHNK * sizeof(i2gseg_t));
>  		}
> 
> _______________________________________________
> 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

Re: [PATCH 1/2] xfsdump: use 64bit local variables in inode.c

[Eric Sandeen]

On 8/21/15 9:01 AM, rjohnston@sgi.com wrote:
> The memset in cb_add_inogrp will segfault when the index oldsize
> overflows. In cb_add_inogrp(), the temp variables used in
> calculating the new i2gmap segment offset should be int64 instead
> of intgen_t (int32).
> 
> Fix this:
>   a. simplify the calculation of oldsize by moving it
>      before hnkmaplen is incremented.
>   b. change the index variables int64 to prevent overflow. 

Ok, so looking at how we count inode groups, it's down this path:

inomap_build( ... igrpcnt ...)
    inogrp_iter( fsfd, cb_count_inogrp, (void *)&igrpcnt, &stat );
    cb_context( ... igrpcnt ... )
        inomap_init(igrpcnt)

We fill in igrpcnt with the inogrp_iter() call, which
does bulkstats in groups / sizes of INOGRPLEN, which is:

#define INOGRPLEN       256

and for each "group" of 256 inodes, igrpcnt increments by one, right?

so I guess that means that if we have more than 2^31 inode groups,
i.e. 2^31 * 256 = 500 billion inodes, (int) igrpcnt could overflow.

That's probably not what you're trying to solve, right?
So does igrpcnt really need to be 64 bits?

If this is just about overflowing oldsize because it's in bytes,
maybe my suggestion for patch number 2 solves that without the need
for a bunch more 64 bit vars - in fact it getse rid of "oldsize"
altogether.  But more questions below...

(Sorry, I don't really grok xfsdump, just trying to follow
things through by rote, so bear with me ...)

> Signed-off-by: Rich Johnston <rjohnston@sgi.com>
> ---
>  dump/inomap.c |   41 +++++++++++++++++++++--------------------
>  1 file changed, 21 insertions(+), 20 deletions(-)
> 
> Index: b/dump/inomap.c
> ===================================================================
> --- a/dump/inomap.c
> +++ b/dump/inomap.c
> @@ -71,7 +71,7 @@ static intgen_t cb_context( bool_t last,
>  			    drange_t *,
>  			    startpt_t *,
>  			    size_t,
> -			    intgen_t,
> +			    int64_t,
>  			    bool_t,
>  			    bool_t *);
>  static void cb_context_free( void );
> @@ -96,7 +96,7 @@ static off64_t estimate_dump_space( xfs_
>  
>  /* inomap primitives
>   */
> -static intgen_t inomap_init( intgen_t igrpcnt );
> +static intgen_t inomap_init( int64_t igrpcnt );
>  static void inomap_add( void *, xfs_ino_t ino, gen_t gen, intgen_t );
>  static intgen_t inomap_set_state( void *, xfs_ino_t ino, intgen_t );
>  static void inomap_set_gen(void *, xfs_ino_t, gen_t );
> @@ -160,7 +160,7 @@ inomap_build( jdm_fshandle_t *fshandlep,
>  	xfs_bstat_t *bstatbufp;
>  	size_t bstatbuflen;
>  	bool_t pruneneeded = BOOL_FALSE;
> -	intgen_t igrpcnt = 0;
> +	int64_t igrpcnt = 0;
>  	intgen_t stat;
>  	intgen_t rval;
>  
> @@ -449,7 +449,7 @@ cb_context( bool_t last,
>  	    drange_t *resumerangep,
>  	    startpt_t *startptp,
>  	    size_t startptcnt,
> -	    intgen_t igrpcnt,
> +	    int64_t igrpcnt,
>  	    bool_t skip_unchanged_dirs,
>  	    bool_t *pruneneededp )
>  {
> @@ -949,14 +949,14 @@ struct i2gseg {
>  typedef struct i2gseg i2gseg_t;
>  
>  typedef struct seg_addr {
> -	intgen_t hnkoff;
> -	intgen_t segoff;
> -	intgen_t inooff;
> +	int64_t hnkoff;
> +	int64_t segoff;
> +	int64_t inooff;

What are these offsets "into?"

Can these offset values really exceed 32 bits?  I'll have to admit
I'm lost, but I don't see it yet in the code.  OTOH, if they can,
then things like

static inline intgen_t
inomap_lastseg( intgen_t hnkoff )
{
        if ( hnkoff == inomap.lastseg.hnkoff )
                return inomap.lastseg.segoff;
        else
                return SEGPERHNK - 1;
}

would need these type changes chased through as well, right?

>  } seg_addr_t;
>  
>  static struct inomap {
>  	hnk_t *hnkmap;
> -	intgen_t hnkmaplen;
> +	int64_t hnkmaplen;

hnkmaplen is initialized in inomap_init as:

inomap.hnkmaplen = (igrpcnt + SEGPERHNK - 1) / SEGPERHNK;

so if 32 bits is sufficient for igrpcnt, it's sufficient for hnkmaplen
too.  (after init, it only increments one at a time).

>  	i2gseg_t *i2gmap;
>  	seg_addr_t lastseg;
>  } inomap;
> @@ -1040,7 +1040,7 @@ SEG_GET_BITS( seg_t *segp, xfs_ino_t ino
>  /* context for inomap construction - initialized by map_init
>   */
>  static intgen_t
> -inomap_init( intgen_t igrpcnt )
> +inomap_init( int64_t igrpcnt )
>  {
>  	ASSERT( sizeof( hnk_t ) == HNKSZ );
>  
> @@ -1066,7 +1066,7 @@ inomap_getsz( void )
>  static inline bool_t
>  inomap_validaddr( seg_addr_t *addrp )
>  {
> -	intgen_t maxseg;
> +	int64_t maxseg;

...

        maxseg = ( addrp->hnkoff == inomap.lastseg.hnkoff ) ?
                        inomap.lastseg.segoff : SEGPERHNK - 1;

so maxseg just needs to be the same type as segoff....

>  
>  	if ( addrp->hnkoff < 0 || addrp->hnkoff > inomap.lastseg.hnkoff )
>  		return BOOL_FALSE;
> @@ -1093,13 +1093,13 @@ inomap_addr2seg( seg_addr_t *addrp )
>  	return &hunkp->seg[addrp->segoff];
>  }
>  
> -static inline intgen_t
> +static inline int64_t
>  inomap_addr2segix( seg_addr_t *addrp )
>  {
>  	return ( addrp->hnkoff * SEGPERHNK ) + addrp->segoff;
>  }
>  
> -static inline intgen_t
> +static inline int64_t
>  inomap_lastseg( intgen_t hnkoff )
>  {
>  	if ( hnkoff == inomap.lastseg.hnkoff )
> @@ -1125,8 +1125,11 @@ cb_add_inogrp( void *arg1, intgen_t fsfd
>  		lastsegp->segoff = 0;
>  
>  		if (lastsegp->hnkoff == inomap.hnkmaplen) {
> -			intgen_t numsegs;
> -			intgen_t oldsize;
> +			int64_t numsegs;
> +			int64_t oldsize;
> +
> +			oldsize = inomap.hnkmaplen * SEGPERHNK
> +				  * sizeof(i2gseg_t);
>  
>  			inomap.hnkmaplen++;
>  			inomap.hnkmap = (hnk_t *)
> @@ -1140,8 +1143,6 @@ cb_add_inogrp( void *arg1, intgen_t fsfd
>  				return -1;
>  
>  			/* zero the new portion of the i2gmap */
> -			oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
> -
>  			memset(inomap.i2gmap + oldsize,
>  			       0,
>  			       SEGPERHNK * sizeof(i2gseg_t));
> @@ -1199,8 +1200,8 @@ static bool_t
>  inomap_find_hnk( seg_addr_t *addrp, xfs_ino_t ino )
>  {
>  	hnk_t *hunkp;
> -	intgen_t lower;
> -	intgen_t upper;
> +	int64_t lower;
> +	int64_t upper;
>  
>  	lower = 0;
>  	upper = inomap.lastseg.hnkoff;
> @@ -1231,8 +1232,8 @@ static bool_t
>  inomap_find_seg( seg_addr_t *addrp, xfs_ino_t ino )
>  {
>  	seg_t *segp;
> -	intgen_t lower;
> -	intgen_t upper;
> +	int64_t lower;
> +	int64_t upper;
>  
>  	if ( !inomap_validaddr( addrp ) ) {
>  		inomap_reset_context( addrp );
> 
> _______________________________________________
> 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

Re: [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp

[Rich Johnston]

On 08/21/2015 10:47 AM, Eric Sandeen wrote:
> Hi -
>
> Are there any testcases for these?  xfsdump is alien code, I swear;
> I'm not quite sure offhand how to tickle any of these bugs.
>
> Thanks,
> -Eric
>

Hey Eric,

Many thanks for taking the time to review these patches.
I take a look at all your responses and get a V2 out early next week.

Thanks
--Rich

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

Re: [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp

[Eric Sandeen]

On 8/21/15 3:08 PM, Rich Johnston wrote:
> On 08/21/2015 10:47 AM, Eric Sandeen wrote:
>> Hi -
>>
>> Are there any testcases for these?  xfsdump is alien code, I swear;
>> I'm not quite sure offhand how to tickle any of these bugs.
>>
>> Thanks,
>> -Eric
>>
> 
> Hey Eric,
> 
> Many thanks for taking the time to review these patches.
> I take a look at all your responses and get a V2 out early next week.

Sounds good.  If I was off track about the need for the extra 64-bit vars,
just educate me.  ;)

-Eric

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

Re: [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp

[Rich Johnston]

On 08/21/2015 03:11 PM, Eric Sandeen wrote:
> On 8/21/15 3:08 PM, Rich Johnston wrote:
>> On 08/21/2015 10:47 AM, Eric Sandeen wrote:
>>> Hi -
>>>
>>> Are there any testcases for these?  xfsdump is alien code, I swear;
>>> I'm not quite sure offhand how to tickle any of these bugs.
>>>
>>> Thanks,
>>> -Eric
>>>
>>
>> Hey Eric,
>>
>> Many thanks for taking the time to review these patches.
>> I take a look at all your responses and get a V2 out early next week.
>
> Sounds good.  If I was off track about the need for the extra 64-bit vars,
> just educate me.  ;)
>
> -Eric
>
Ok you convinced me, there is no need for patch 1 and patch 2 can be
solved by removing the local variable oldsize.

A new patch has been submitted.
[PATCH] xfsdump: prevent segfault in cb_add_inogrp

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