Re: [PATCH 3/5] repair: remove BITS_PER_LONG cpp checks in bmap.[ch]

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Sun, Jan 26, 2020 at 12:35:39PM +0100, Christoph Hellwig wrote:
> Add a little helper to validate the nex count so that we can use compile
> time magic checks for sizeof long directly.  Also don't print the max
> in case of an overflow as the value will always be the same.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  repair/bmap.c | 29 +++++++++++++++++++++--------
>  repair/bmap.h | 13 -------------
>  2 files changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/repair/bmap.c b/repair/bmap.c
> index 44e43ab4..0d717cd3 100644
> --- a/repair/bmap.c
> +++ b/repair/bmap.c
> @@ -22,6 +22,22 @@
>  pthread_key_t	dblkmap_key;
>  pthread_key_t	ablkmap_key;
>  
> +/*
> + * For 32 bit platforms, we are limited to extent arrays of 2^31 bytes, which
> + * limits the number of extents in an inode we can check. If we don't limit the
> + * valid range, we can overflow the BLKMAP_SIZE() calculation and allocate less
> + * memory than we think we needed, and hence walk off the end of the array and
> + * corrupt memory.
> + */
> +static inline bool
> +blkmap_nex_valid(
> +	xfs_extnum_t	nex)
> +{
> +	if (sizeof(long) < 64 && nex >= INT_MAX / sizeof(bmap_ext_t))

sizeof(long) < 8

Frankly I suspect the maximum array length on 32 bit platforms is far
less than 2^31 bytes...

--D

> +		return false;
> +	return true;
> +}
> +
>  blkmap_t *
>  blkmap_alloc(
>  	xfs_extnum_t	nex,
> @@ -35,8 +51,7 @@ blkmap_alloc(
>  	if (nex < 1)
>  		nex = 1;
>  
> -#if (BITS_PER_LONG == 32)	/* on 64-bit platforms this is never true */
> -	if (nex > BLKMAP_NEXTS_MAX) {
> +	if (!blkmap_nex_valid(nex)) {
>  		do_warn(
>  	_("Number of extents requested in blkmap_alloc (%d) overflows 32 bits.\n"
>  	  "If this is not a corruption, then you will need a 64 bit system\n"
> @@ -44,7 +59,6 @@ blkmap_alloc(
>  			nex);
>  		return NULL;
>  	}
> -#endif
>  
>  	key = whichfork ? ablkmap_key : dblkmap_key;
>  	blkmap = pthread_getspecific(key);
> @@ -278,20 +292,19 @@ blkmap_grow(
>  		ASSERT(pthread_getspecific(key) == blkmap);
>  	}
>  
> -#if (BITS_PER_LONG == 32)	/* on 64-bit platforms this is never true */
> -	if (new_naexts > BLKMAP_NEXTS_MAX) {
> +	if (!blkmap_nex_valid(new_naexts)) {
>  		do_error(
>  	_("Number of extents requested in blkmap_grow (%d) overflows 32 bits.\n"
>  	  "You need a 64 bit system to repair this filesystem.\n"),
>  			new_naexts);
>  		return NULL;
>  	}
> -#endif
> +
>  	if (new_naexts <= 0) {
>  		do_error(
>  	_("Number of extents requested in blkmap_grow (%d) overflowed the\n"
> -	  "maximum number of supported extents (%d).\n"),
> -			new_naexts, BLKMAP_NEXTS_MAX);
> +	  "maximum number of supported extents.\n"),
> +			new_naexts);
>  		return NULL;
>  	}
>  
> diff --git a/repair/bmap.h b/repair/bmap.h
> index 4b588df8..df9602b3 100644
> --- a/repair/bmap.h
> +++ b/repair/bmap.h
> @@ -28,19 +28,6 @@ typedef	struct blkmap {
>  #define	BLKMAP_SIZE(n)	\
>  	(offsetof(blkmap_t, exts) + (sizeof(bmap_ext_t) * (n)))
>  
> -/*
> - * For 32 bit platforms, we are limited to extent arrays of 2^31 bytes, which
> - * limits the number of extents in an inode we can check. If we don't limit the
> - * valid range, we can overflow the BLKMAP_SIZE() calculation and allocate less
> - * memory than we think we needed, and hence walk off the end of the array and
> - * corrupt memory.
> - */
> -#if BITS_PER_LONG == 32
> -#define BLKMAP_NEXTS_MAX	((INT_MAX / sizeof(bmap_ext_t)) - 1)
> -#else
> -#define BLKMAP_NEXTS_MAX	INT_MAX
> -#endif
> -
>  extern pthread_key_t dblkmap_key;
>  extern pthread_key_t ablkmap_key;
>  
> -- 
> 2.24.1
>