diff mbox series

[U-BOOT,v2] btrfs: fix offset when reading compressed extents

Message ID 20230418-btrfs-extent-reads-v2-1-772a6be2ea9a@codewreck.org (mailing list archive)
State New, archived
Headers show
Series [U-BOOT,v2] btrfs: fix offset when reading compressed extents | expand

Commit Message

Dominique Martinet April 18, 2023, 6:41 a.m. UTC
From: Dominique Martinet <dominique.martinet@atmark-techno.com>

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, e.g. reading a 10k file with a
single extent:
btrfs_file_read()
 -> btrfs_read_extent_reg
    (aligned part loop, until 8k)
 -> read_and_truncate_page
   -> btrfs_read_extent_reg
      (re-reads the last extent from 8k to the end,
      incorrectly reading the first 2k of data)

This can be reproduced as follow:
$ truncate -s 200M btr
$ mount btr -o compress /mnt
$ pat() { dd if=/dev/zero bs=1M count=$1 iflag=count_bytes status=none | tr '\0' "\\$2"; }
$ { pat 4K 1; pat 4K 2; pat 2K 3; }  > /mnt/file
$ sync
$ filefrag -v /mnt/file
File size of /mnt/file is 10240 (3 blocks of 4096 bytes)
 ext:     logical_offset:        physical_offset: length:   expected: flags:
   0:        0..       2:       3328..      3330:      3:             last,encoded,eof
$ umount /mnt

Then in u-boot:
=> load scsi 0 2000000 file
10240 bytes read in 3 ms (3.3 MiB/s)
=> md 2001ff0
02001ff0: 02020202 02020202 02020202 02020202  ................
02002000: 01010101 01010101 01010101 01010101  ................
02002010: 01010101 01010101 01010101 01010101  ................

(02002000 onwards should contain '03' pattern but went back to 01,
start of the extent)

After patch, data is read properly:
=> md 2001ff0
02001ff0: 02020202 02020202 02020202 02020202  ................
02002000: 03030303 03030303 03030303 03030303  ................
02002010: 03030303 03030303 03030303 03030303  ................

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 even if the previous code might not have been
handling offsets correctly either (something that booted now fails to
boot)

Fixes: a26a6bedafcf ("fs: btrfs: Introduce btrfs_read_extent_inline() and btrfs_read_extent_reg()")
Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
---
Changes in v2:
- Keep offset decomposition explicit where it is used
- Add reproducer/clarify explanation in commit message
- Drop other patches temporarily
- Link to v1: https://lore.kernel.org/r/20230418-btrfs-extent-reads-v1-0-47ba9839f0cc@codewreck.org
---
 fs/btrfs/inode.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)


---
base-commit: 5db4972a5bbdbf9e3af48ffc9bc4fec73b7b6a79
change-id: 20230418-btrfs-extent-reads-e2df6e329ad4

Best regards,

Comments

Qu Wenruo April 18, 2023, 7:20 a.m. UTC | #1
On 2023/4/18 14:41, Dominique Martinet wrote:
> From: Dominique Martinet <dominique.martinet@atmark-techno.com>
> 
> 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, e.g. reading a 10k file with a
> single extent:
> btrfs_file_read()
>   -> btrfs_read_extent_reg
>      (aligned part loop, until 8k)
>   -> read_and_truncate_page
>     -> btrfs_read_extent_reg
>        (re-reads the last extent from 8k to the end,
>        incorrectly reading the first 2k of data)
> 
> This can be reproduced as follow:
> $ truncate -s 200M btr
> $ mount btr -o compress /mnt
> $ pat() { dd if=/dev/zero bs=1M count=$1 iflag=count_bytes status=none | tr '\0' "\\$2"; }
> $ { pat 4K 1; pat 4K 2; pat 2K 3; }  > /mnt/file
> $ sync
> $ filefrag -v /mnt/file
> File size of /mnt/file is 10240 (3 blocks of 4096 bytes)
>   ext:     logical_offset:        physical_offset: length:   expected: flags:
>     0:        0..       2:       3328..      3330:      3:             last,encoded,eof
> $ umount /mnt
> 
> Then in u-boot:
> => load scsi 0 2000000 file
> 10240 bytes read in 3 ms (3.3 MiB/s)
> => md 2001ff0
> 02001ff0: 02020202 02020202 02020202 02020202  ................
> 02002000: 01010101 01010101 01010101 01010101  ................
> 02002010: 01010101 01010101 01010101 01010101  ................
> 
> (02002000 onwards should contain '03' pattern but went back to 01,
> start of the extent)
> 
> After patch, data is read properly:
> => md 2001ff0
> 02001ff0: 02020202 02020202 02020202 02020202  ................
> 02002000: 03030303 03030303 03030303 03030303  ................
> 02002010: 03030303 03030303 03030303 03030303  ................
> 
> 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 even if the previous code might not have been
> handling offsets correctly either (something that booted now fails to
> boot)
> 
> Fixes: a26a6bedafcf ("fs: btrfs: Introduce btrfs_read_extent_inline() and btrfs_read_extent_reg()")
> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
> Changes in v2:
> - Keep offset decomposition explicit where it is used
> - Add reproducer/clarify explanation in commit message
> - Drop other patches temporarily
> - Link to v1: https://lore.kernel.org/r/20230418-btrfs-extent-reads-v1-0-47ba9839f0cc@codewreck.org
> ---
>   fs/btrfs/inode.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 40025662f250..38e285bf94b0 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -511,7 +511,9 @@ 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 + btrfs_file_extent_offset(leaf, fi) + offset - key.offset,
> +	       len);
>   	ret = len;
>   out:
>   	free(cbuf);
> 
> ---
> base-commit: 5db4972a5bbdbf9e3af48ffc9bc4fec73b7b6a79
> change-id: 20230418-btrfs-extent-reads-e2df6e329ad4
> 
> Best regards,
Tom Rini May 8, 2023, 5:38 p.m. UTC | #2
On Tue, 18 Apr 2023 15:41:55 +0900, Dominique Martinet wrote:

> 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, e.g. reading a 10k file with a
> single extent:
> btrfs_file_read()
>  -> btrfs_read_extent_reg
>     (aligned part loop, until 8k)
>  -> read_and_truncate_page
>    -> btrfs_read_extent_reg
>       (re-reads the last extent from 8k to the end,
>       incorrectly reading the first 2k of data)
> 
> [...]

Applied to u-boot/master, thanks!
diff mbox series

Patch

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 40025662f250..38e285bf94b0 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -511,7 +511,9 @@  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 + btrfs_file_extent_offset(leaf, fi) + offset - key.offset,
+	       len);
 	ret = len;
 out:
 	free(cbuf);