Re: [PATCH 2/2] uboot: fs/btrfs: Fix LZO false decompression error caused by pending zero

On 2020/3/25 下午4:09, Marek Behun wrote:

On Thu, 19 Mar 2020 20:33:19 +0800 Qu Wenruo wqu@suse.com wrote:

...

[BUG]

The subject line should not contain uboot keyword. If you check git log for fs/btrfs, the commits always start with: fs: btrfs:

Also please don't use [BUG] [CAUSE] and [FIX] in commit messages.

Why? I think such section makes the analyse much easier to read.

You could have also added myself to CC, since I am the original author of U-Boots btrfs implementation. I just stumbled on your patches by chance.

Since there is no maintainer name in MAINTAINERS file, there is no way other guys would know who to CC.

Also do not add linux-btrfs mailing list for u-boot patches.

I don't see any reason why we can't include the mail list for more experts to review.

...

For certain btrfs files with compressed file extent, uboot will fail to load it:

btrfs_read_extent_reg: disk_bytenr=14229504 disk_len=73728 offset=0 nr_bytes=131 072 decompress_lzo: tot_len=70770 decompress_lzo: in_len=1389 decompress_lzo: in_len=2400 decompress_lzo: in_len=3002 decompress_lzo: in_len=1379 decompress_lzo: in_len=88539136 decompress_lzo: header error, in_len=88539136 clen=65534 tot_len=62580

NOTE: except the last line, all other lines are debug output.

[CAUSE] Btrfs lzo compression uses its own format to record compressed size (segmant header, LE32).

However to make decompression easier, we never put such segment header across page boundary.

In above case, the xxd dump of the lzo compressed data looks like this:

00001fe0: 4cdc 02fc 0bfd 02c0 dc02 0d13 0100 0001 L............... 00001ff0: 0000 0008 0300 0000 0000 0011 0000|0000 ................ 00002000: 4705 0000 0001 cc02 0000 0000 0000 1e01 G...............

'|' is the "expected" segment header start position.

But in that page, there are only 2 bytes left, can't contain the 4 bytes segment header.

So btrfs compression will skip that 2 bytes, put the segment header in next page directly.

Uboot doesn't have such check, and read the header with 2 bytes offset, result 0x05470000 (88539136), other than the expected result 0x00000547 (1351), resulting above error.

[FIX] Follow the btrfs-progs restore implementation, by introducing tot_in to record total processed bytes (including headers), and do proper page boundary skip to fix it.

Signed-off-by: Qu Wenruo wqu@suse.com

fs/btrfs/compression.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c index 4ef44ce11485..2a6ac8bb1029 100644 --- a/fs/btrfs/compression.c +++ b/fs/btrfs/compression.c @@ -9,6 +9,7 @@ #include <malloc.h> #include <linux/lzo.h> #include <linux/zstd.h> +#include <linux/compat.h> #include <u-boot/zlib.h> #include <asm/unaligned.h>

@@ -17,6 +18,7 @@ static u32 decompress_lzo(const u8 *cbuf, u32 clen, u8 *dbuf, u32 dlen) { u32 tot_len, in_len, res;

• u32 tot_in = 0;

This function does not define local variable values in declaration, please don't mix this. Also tot_in is of the same type as the variables above, so use u32 tot_len, tot_in, in_len, res;

Please give us the proper code style doc, I understand that each project or even each subsystem has its own style, but without proper doc it will be a mess to maintain.

So, please show the proper code style for us to follow.

And put tot_in = 0; after line 24: tot_len = le32_to_cpu(get_unaligned((u32 *)cbuf));

...

size_t out_len; int ret;

@@ -27,6 +29,7 @@ static u32 decompress_lzo(const u8 *cbuf, u32 clen, u8 *dbuf, u32 dlen) cbuf += LZO_LEN; clen -= LZO_LEN; tot_len -= LZO_LEN;

• tot_in += LZO_LEN;

  if (tot_len == 0 && dlen) return -1;

@@ -36,6 +39,9 @@ static u32 decompress_lzo(const u8 *cbuf, u32 clen, u8 *dbuf, u32 dlen) res = 0;

while (tot_len > LZO_LEN) {

• size_t mod_page;
  

• size_t rem_page;
  

The rest of the function uses u32, not size_t. Please use it as well. Also since the variables are of same type, please use one-line declaration only, eg: u32 mod_page, rem_page;

...

• in_len = le32_to_cpu(get_unaligned((u32 *)cbuf)); cbuf += LZO_LEN; clen -= LZO_LEN;

@@ -44,6 +50,7 @@ static u32 decompress_lzo(const u8 *cbuf, u32 clen, u8 *dbuf, u32 dlen) return -1;

tot_len -= (LZO_LEN + in_len);

• tot_in += (LZO_LEN + in_len);
  
  

  out_len = dlen; ret = lzo1x_decompress_safe(cbuf, in_len, dbuf, &out_len);

@@ -56,6 +63,19 @@ static u32 decompress_lzo(const u8 *cbuf, u32 clen, u8 *dbuf, u32 dlen) dlen -= out_len;

res += out_len;

• /*
  

•  * If the 4 bytes header does not fit to the rest of the page we
  

•  * have to move to next one, or we read some garbage.
  

•  */
  

• mod_page = tot_in % PAGE_SIZE;
  

• rem_page = PAGE_SIZE - mod_page;
  

Is there a reasof for mod_page? You could use just rem_page = PAGE_SIZE - (tot_in % PAGE_SIZE);

I tend to keep the difference from btrfs-progs to minimal, as that's what I tend to plan to backport soon.

...

• if (rem_page < LZO_LEN) {
  

• 	cbuf += rem_page;
  

• 	tot_in += rem_page;
  

• 	clen -= rem_page;
  

• 	tot_len -= rem_page;
  

• }
  

  }

  return res;

Sorry for this nitpicking, but I would like my driver to remain consistent in coding style.

I know uboot btrfs is mostly backported by yourself and I respect the work.

But please add yourself to maintainers files, and also consider use existing btrfs-progs code to make code sycn easier, which would avoid such bug from the very beginning.

Thanks, Qu

Marek