linux-ext4 - Re: [PATCH] filefrag: fix wrong extent count calculation when using FIBMAP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20141007024339.GA10054@birch.djwong.org>
Date:	Mon, 6 Oct 2014 19:43:39 -0700
From:	"Darrick J. Wong" <darrick.wong@...cle.com>
To:	Xiaoguang Wang <wangxg.fnst@...fujitsu.com>
Cc:	linux-ext4@...r.kernel.org, tytso@....edu
Subject: Re: [PATCH] filefrag: fix wrong extent count calculation when using
 FIBMAP

On Mon, Oct 06, 2014 at 07:05:39PM +0800, Xiaoguang Wang wrote:
> When using FIBMAP and '-e' option is specified, the calculation for fiemap_extent
> is wrong, we wrongly updated fm_ext.fe_logical for every iteration, please see the
> code in the end of 'for' loop in fm_ext.fe_logical().
> 
> In an ext2 file system(block size is 1024 bytes),
>   dd if=/dev/zero of=testfile bs=1k count=15
>   Using debugfs, corresponding physical blocks are "2049 2050 2051 2052 2053 2054
> 2055 2056 2057 2058 2059 2060 1025 2061 2062 2063",  1025 is this indirect block.
> Before this patch, filefrag's output would be:
>   filefrag -B -e testfile
>   Filesystem type is: ef53
>   Filesystem cylinder groups approximately 16
>   File size of testfile is 15360 (15 blocks of 1024 bytes)
>    ext:     logical_offset:        physical_offset: length:   expected: flags:
>      0:        1..       2:       2050..      2051:      2:       2051: merged
>      1:        3..       4:       2052..      2053:      2:       2053: merged
>      2:        5..       6:       2054..      2055:      2:       2055: merged
>      3:        7..       8:       2056..      2057:      2:       2057: merged
>      4:        9..      10:       2058..      2059:      2:       2059: merged
>      5:       11..      12:       2060..      2061:      2:       2062: merged
>      6:       13..      14:       2062..      2063:      2:       2063: merged,eof
>      7:       14..      14:       2063..      2063:      1:       2063: merged,eof
> This output is not reasonable.

It's just plain whacky.  Why would logical offset 14 be listed twice? :)

> Fix this bug and try to make it readable. After this patch, the output would be:
>   ./filefrag -B -e mntpoint/testfile
>   Filesystem type is: ef53
>   Filesystem cylinder groups approximately 16
>   File size of mntpoint/testfile is 15360 (15 blocks of 1024 bytes)
>    ext:     logical_offset:        physical_offset: length:   expected: flags:
>      0:        0..      11:       2049..      2060:     12:       2062: merged
>      1:       12..      14:       2061..      2063:      3:       2063: merged,eof
>   mntpoint/testfile: 2 extents found, perfection would be 1 extent

Where'd the indirect block end up, if not in the middle of 2049-2063?

> Signed-off-by: Xiaoguang Wang <wangxg.fnst@...fujitsu.com>
> ---
>  misc/filefrag.c | 37 +++++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/misc/filefrag.c b/misc/filefrag.c
> index c1a8684..e9f7e68 100644
> --- a/misc/filefrag.c
> +++ b/misc/filefrag.c
> @@ -315,32 +315,41 @@ static int filefrag_fibmap(int fd, int blk_shift, int *num_extents,
>  			return rc;
>  		if (block == 0)
>  			continue;
> +		count++;
> +
>  		if (*num_extents == 0) {
>  			(*num_extents)++;
>  			if (force_extent) {
>  				print_extent_header();
> +				fm_ext.fe_logical = logical;
>  				fm_ext.fe_physical = block * st->st_blksize;
> +				fm_ext.fe_length = st->st_blksize;
>  			}
> +			last_block = block;
> +			continue;
>  		}
> -		count++;
> -		if (force_extent && last_block != 0 &&
> -		    (block != last_block + 1 ||
> -		     fm_ext.fe_logical + fm_ext.fe_length != logical)) {
> -			print_extent_info(&fm_ext, *num_extents - 1,
> -					  (last_block + 1) * st->st_blksize,
> -					  blk_shift, st);
> -			fm_ext.fe_length = 0;
> -			(*num_extents)++;
> +
> +		if (force_extent) {
> +			if (block != last_block + 1 ||
> +			    fm_ext.fe_length + fm_ext.fe_logical != logical) {
> +				print_extent_info(&fm_ext, *num_extents - 1,
> +						  (last_block + 1) *
> +						  st->st_blksize,
> +						  blk_shift, st);
> +				fm_ext.fe_logical = logical;
> +				fm_ext.fe_physical = block * st->st_blksize;
> +				fm_ext.fe_length = st->st_blksize;
> +				(*num_extents)++;
> +			} else {
> +				fm_ext.fe_length += st->st_blksize;
> +			}
>  		} else if (last_block && (block != last_block + 1)) {
> -			if (verbose)
> +			if (verbose) {
>  				printf("Discontinuity: Block %ld is at %lu (was "
>  				       "%lu)\n", i, block, last_block + 1);
> -			fm_ext.fe_length = 0;
> +			}

The { } is not needed for a single line if statement.

>  			(*num_extents)++;
>  		}
> -		fm_ext.fe_logical = logical;
> -		fm_ext.fe_physical = block * st->st_blksize;
> -		fm_ext.fe_length += st->st_blksize;
>  		last_block = block;

Otherwise, I think this looks decent.

--D

>  	}
>  
> -- 
> 1.8.2.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html