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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

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
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200325/d1eaa03d/attachment.sig>