Re: [PATCH U-BOOT v2 3/3] fs: btrfs: Fix LZO false decompression error caused by pending zero

2020-04-17 Thread Tom Rini
On Thu, Mar 26, 2020 at 01:35:56PM +0800, Qu Wenruo wrote:

> 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.
> 
> Btrfs lzo compression uses its own format to record compressed size
> (segment 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:
> 
> 1fe0: 4cdc 02fc 0bfd 02c0 dc02 0d13 0100 0001  L...
> 1ff0:  0008 0300   0011 |  
> 2000: 4705  0001 cc02    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 0x0547 (88539136), other than the expected result
> 0x0547 (1351), resulting above error.
> 
> 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.
> 
> Please note that, current code base doesn't parse fs_info thus we can't
> grab sector size easily, so it uses PAGE_SIZE, and relying on fs open
> time check to exclude unsupported sector size.
> 
> Signed-off-by: Qu Wenruo 
> Cc: Marek Behun 
> Reviewed-by: Marek BehĂșn 

Applied to u-boot/master, thanks!

-- 
Tom


signature.asc
Description: PGP signature


Re: [PATCH U-BOOT v2 3/3] fs: btrfs: Fix LZO false decompression error caused by pending zero

2020-03-26 Thread Marek Behun
On Thu, 26 Mar 2020 13:35:56 +0800
Qu Wenruo  wrote:

> 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.
> 
> Btrfs lzo compression uses its own format to record compressed size
> (segment 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:
> 
> 1fe0: 4cdc 02fc 0bfd 02c0 dc02 0d13 0100 0001  L...
> 1ff0:  0008 0300   0011 |  
> 2000: 4705  0001 cc02    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 0x0547 (88539136), other than the expected result
> 0x0547 (1351), resulting above error.
> 
> 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.
> 
> Please note that, current code base doesn't parse fs_info thus we can't
> grab sector size easily, so it uses PAGE_SIZE, and relying on fs open
> time check to exclude unsupported sector size.
> 
> Signed-off-by: Qu Wenruo 
> Cc: Marek Behun 
> ---
>  fs/btrfs/compression.c | 20 +++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 4ef44ce11485..b1884fc15ee0 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -9,6 +9,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -16,7 +17,7 @@
>  #define LZO_LEN  4
>  static u32 decompress_lzo(const u8 *cbuf, u32 clen, u8 *dbuf, u32 dlen)
>  {
> - u32 tot_len, in_len, res;
> + u32 tot_len, tot_in, in_len, res;
>   size_t out_len;
>   int ret;
>  
> @@ -24,9 +25,11 @@ static u32 decompress_lzo(const u8 *cbuf, u32 clen, u8 
> *dbuf, u32 dlen)
>   return -1;
>  
>   tot_len = le32_to_cpu(get_unaligned((u32 *)cbuf));
> + tot_in = 0;
>   cbuf += LZO_LEN;
>   clen -= LZO_LEN;
>   tot_len -= LZO_LEN;
> + tot_in += LZO_LEN;
>  
>   if (tot_len == 0 && dlen)
>   return -1;
> @@ -36,6 +39,8 @@ static u32 decompress_lzo(const u8 *cbuf, u32 clen, u8 
> *dbuf, u32 dlen)
>   res = 0;
>  
>   while (tot_len > LZO_LEN) {
> + u32 rem_page;
> +
>   in_len = le32_to_cpu(get_unaligned((u32 *)cbuf));
>   cbuf += LZO_LEN;
>   clen -= LZO_LEN;
> @@ -44,6 +49,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, _len);
> @@ -56,6 +62,18 @@ 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.
> +  */
> + rem_page = PAGE_SIZE - (tot_in % PAGE_SIZE);
> + if (rem_page < LZO_LEN) {
> + cbuf += rem_page;
> + tot_in += rem_page;
> + clen -= rem_page;
> + tot_len -= rem_page;
> + }
>   }
>  
>   return res;

Reviewed-by: Marek BehĂșn