[PATCH U-BOOT 1/3] btrfs: fix offset within btrfs_read_extent_reg()
Qu Wenruo
quwenruo.btrfs at gmx.com
Tue Apr 18 03:58:37 CEST 2023
On 2023/4/18 09:17, Dominique Martinet wrote:
> From: Dominique Martinet <dominique.martinet at atmark-techno.com>
The subject can be changed to "btrfs: fix offset when reading compressed
extents".
The original one is a little too generic.
>
> btrfs_read_extent_reg correctly computed the extent offset in the
> BTRFS_COMPRESS_NONE case, but did not account for the 'offset - key.offset'
> part correctly in the compressed case, making the function read
> incorrect data.
>
> In the case I examined, the last 4k of a file was corrupted and
> contained data from a few blocks prior:
> btrfs_file_read()
> -> btrfs_read_extent_reg
> (aligned part loop from 0x40480000 to 0x40ba0000, 128KB at a time)
> last read had 0x4000 bytes in extent, but aligned_end - cur limited
> the read to 0x3000 (offset 0x720000)
> -> read_and_truncate_page
> -> btrfs_read_extent_reg
> reading the last 0x1000 bytes from offset 0x73000 (end of the
> previous 0x4000) into 0x40ba3000
> here key.offset = 0x70000 so we need to use it to recover the
> 0x3000 offset.
You can use "btrfs ins dump-tree" to provide a much easier to read
on-disk data layout.
And you can also add a smaller reproducer.
>
> Confirmed by checking data, before patch:
> u-boot=> mmc load 1:1 $loadaddr boot/uImage
> u-boot=> md 0x40ba0000
> 40ba0000: c0232ff8 c0c722cb 030e94be bf10000c ./#.."..........
> u-boot=> md 0x40ba3000
> 40ba3000: c0232ff8 c0c722cb 030e94be bf10000c ./#.."..........
>
> After patch (also confirmed with data read from linux):
> u-boot=> md 0x40ba3000
> 40ba3000: 64cc9f03 81142256 6910000c 0c03483c ...dV".....i<H..
>
> Note that the code previously (before commit e3427184f38a ("fs: btrfs:
> Implement btrfs_file_read()")) did not split that read in two, so
> this is a regression.
>
> Fixes: a26a6bedafcf ("fs: btrfs: Introduce btrfs_read_extent_inline() and btrfs_read_extent_reg()")
> Signed-off-by: Dominique Martinet <dominique.martinet at atmark-techno.com>
> ---
> fs/btrfs/inode.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 40025662f250..3d6e39e6544d 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -443,6 +443,8 @@ int btrfs_read_extent_reg(struct btrfs_path *path,
> IS_ALIGNED(len, fs_info->sectorsize));
> ASSERT(offset >= key.offset &&
> offset + len <= key.offset + extent_num_bytes);
> + /* offset is now offset within extent */
> + offset = btrfs_file_extent_offset(leaf, fi) + offset - key.offset;
I prefer not to use the @offset, explained later.
>
> /* Preallocated or hole , fill @dest with zero */
> if (btrfs_file_extent_type(leaf, fi) == BTRFS_FILE_EXTENT_PREALLOC ||
> @@ -454,9 +456,7 @@ int btrfs_read_extent_reg(struct btrfs_path *path,
> if (btrfs_file_extent_compression(leaf, fi) == BTRFS_COMPRESS_NONE) {
> u64 logical;
>
> - logical = btrfs_file_extent_disk_bytenr(leaf, fi) +
> - btrfs_file_extent_offset(leaf, fi) +
> - offset - key.offset;
> + logical = btrfs_file_extent_disk_bytenr(leaf, fi) + offset;
This is touching non-compressed path, which is very weird as your commit
message said this part is correct.
I prefer the original one to show everything we need to take into
consideration.
Otherwise looks good to me.
Thanks,
Qu
> read = len;
>
> num_copies = btrfs_num_copies(fs_info, logical, len);
> @@ -511,7 +511,7 @@ int btrfs_read_extent_reg(struct btrfs_path *path,
> if (ret < dsize)
> memset(dbuf + ret, 0, dsize - ret);
> /* Then copy the needed part */
> - memcpy(dest, dbuf + btrfs_file_extent_offset(leaf, fi), len);
> + memcpy(dest, dbuf + offset, len);
> ret = len;
> out:
> free(cbuf);
>
More information about the U-Boot
mailing list