[PATCH 0/2] uboot: fs/btrfs: Fix read error on LZO compressed extents

[PATCH 0/2] uboot: fs/btrfs: Fix read error on LZO compressed extents

[Qu Wenruo]

There is a bug that uboot can't load LZO compressed data extent while
kernel can handle it without any problem.

It turns out to be a page boundary case. The 2nd patch is the proper
fix, backported from btrfs-progs.

The first patch is just to make my eyes less hurt.

I guess it's time to backport proper code from btrfs-progs, other than
using tons of immediate numbers.

Qu Wenruo (2):
  uboot: fs/btrfs: Use LZO_LEN to replace immediate number
  uboot: fs/btrfs: Fix LZO false decompression error caused by pending
    zero

 fs/btrfs/compression.c | 42 ++++++++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 10 deletions(-)

-- 
2.25.1

[PATCH 1/2] uboot: fs/btrfs: Use LZO_LEN to replace immediate number

[Qu Wenruo]

Just a cleanup. The immediate number makes my eye hurt.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/compression.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 346875d45a1b..4ef44ce11485 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -12,36 +12,38 @@
 #include <u-boot/zlib.h>
 #include <asm/unaligned.h>
 
+/* Header for each segment, LE32, recording the compressed size */
+#define LZO_LEN		4
 static u32 decompress_lzo(const u8 *cbuf, u32 clen, u8 *dbuf, u32 dlen)
 {
 	u32 tot_len, in_len, res;
 	size_t out_len;
 	int ret;
 
-	if (clen < 4)
+	if (clen < LZO_LEN)
 		return -1;
 
 	tot_len = le32_to_cpu(get_unaligned((u32 *)cbuf));
-	cbuf += 4;
-	clen -= 4;
-	tot_len -= 4;
+	cbuf += LZO_LEN;
+	clen -= LZO_LEN;
+	tot_len -= LZO_LEN;
 
 	if (tot_len == 0 && dlen)
 		return -1;
-	if (tot_len < 4)
+	if (tot_len < LZO_LEN)
 		return -1;
 
 	res = 0;
 
-	while (tot_len > 4) {
+	while (tot_len > LZO_LEN) {
 		in_len = le32_to_cpu(get_unaligned((u32 *)cbuf));
-		cbuf += 4;
-		clen -= 4;
+		cbuf += LZO_LEN;
+		clen -= LZO_LEN;
 
-		if (in_len > clen || tot_len < 4 + in_len)
+		if (in_len > clen || tot_len < LZO_LEN + in_len)
 			return -1;
 
-		tot_len -= 4 + in_len;
+		tot_len -= (LZO_LEN + in_len);
 
 		out_len = dlen;
 		ret = lzo1x_decompress_safe(cbuf, in_len, dbuf, &out_len);
-- 
2.25.1

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

[Qu Wenruo]

[BUG]
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;
 	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;
+
 		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;
+		if (rem_page < LZO_LEN) {
+			cbuf += rem_page;
+			tot_in += rem_page;
+			clen -= rem_page;
+			tot_len -= rem_page;
+		}
 	}
 
 	return res;
-- 
2.25.1

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

[Matthias Brugger]

Hi Qu,

On 19/03/2020 13:30, Qu Wenruo wrote:
> [BUG]
> 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;
>  	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;
> +
>  		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;

in U-Boot we use 4K page sizes, but the OS could use another page size (16K or
64k). Would we need to adapt that code to reflect which page size is used on the
medium we want to access?

Regards,
Matthias

> +		rem_page = PAGE_SIZE - mod_page;
> +		if (rem_page < LZO_LEN) {
> +			cbuf += rem_page;
> +			tot_in += rem_page;
> +			clen -= rem_page;
> +			tot_len -= rem_page;
> +		}
>  	}
>  
>  	return res;
> 

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

[David Sterba]

On Thu, Mar 19, 2020 at 02:33:28PM +0100, Matthias Brugger wrote:
> >  		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;
> 
> in U-Boot we use 4K page sizes, but the OS could use another page size (16K or
> 64k). Would we need to adapt that code to reflect which page size is used on the
> medium we want to access?

Yes, it is the 'sectorsize' as it's set up in fs_info or it's equivalent
in uboot. For kernel the page size == sectorsize is kind of implicit and
verified at mount time.

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

[Matthias Brugger]

On 19/03/2020 14:56, David Sterba wrote:
> On Thu, Mar 19, 2020 at 02:33:28PM +0100, Matthias Brugger wrote:
>>>  		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;
>>
>> in U-Boot we use 4K page sizes, but the OS could use another page size (16K or
>> 64k). Would we need to adapt that code to reflect which page size is used on the
>> medium we want to access?
> 
> Yes, it is the 'sectorsize' as it's set up in fs_info or it's equivalent
> in uboot. For kernel the page size == sectorsize is kind of implicit and
> verified at mount time.
> 

Does this mean we would need to add a Kconfig option to set the sectorsize in
U-Boot?

Regards,
Matthias

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

[David Sterba]

On Thu, Mar 19, 2020 at 03:34:12PM +0100, Matthias Brugger wrote:
> 
> 
> On 19/03/2020 14:56, David Sterba wrote:
> > On Thu, Mar 19, 2020 at 02:33:28PM +0100, Matthias Brugger wrote:
> >>>  		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;
> >>
> >> in U-Boot we use 4K page sizes, but the OS could use another page size (16K or
> >> 64k). Would we need to adapt that code to reflect which page size is used on the
> >> medium we want to access?
> > 
> > Yes, it is the 'sectorsize' as it's set up in fs_info or it's equivalent
> > in uboot. For kernel the page size == sectorsize is kind of implicit and
> > verified at mount time.
> > 
> 
> Does this mean we would need to add a Kconfig option to set the sectorsize in
> U-Boot?

No, the value depends on the filesystem so it can't be a config option.
What I mean is btrfs_super_block::sectorsize, where the superblock is
btrfs_info::sb.

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

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1799 bytes --]



On 2020/3/20 上午12:28, David Sterba wrote:
> On Thu, Mar 19, 2020 at 03:34:12PM +0100, Matthias Brugger wrote:
>>
>>
>> On 19/03/2020 14:56, David Sterba wrote:
>>> On Thu, Mar 19, 2020 at 02:33:28PM +0100, Matthias Brugger wrote:
>>>>>  		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;
>>>>
>>>> in U-Boot we use 4K page sizes, but the OS could use another page size (16K or
>>>> 64k). Would we need to adapt that code to reflect which page size is used on the
>>>> medium we want to access?
>>>
>>> Yes, it is the 'sectorsize' as it's set up in fs_info or it's equivalent
>>> in uboot. For kernel the page size == sectorsize is kind of implicit and
>>> verified at mount time.
>>>
>>
>> Does this mean we would need to add a Kconfig option to set the sectorsize in
>> U-Boot?
> 
> No, the value depends on the filesystem so it can't be a config option.
> What I mean is btrfs_super_block::sectorsize, where the superblock is
> btrfs_info::sb.
> 

Sorry for the delayed reply. (Stupid filter setup).

Currently most Uboot boards should use the same page size setup for its
kernel, and most btrfs uses 4K sector size.

So for Uboot it should be no problem.

Although the best practice is to read the fs_info::sectorsize as David
mentioned, but the code base doesn't allow us to do that yet.

So I'm going to backport the read part code from btrfs-progs in the
near-future, and completely solve it, making it sector size independent.

Would this plan looks sound? Or we need to wait for the full
re-implementation?

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1802 bytes --]



On 2020/3/20 上午12:28, David Sterba wrote:
> On Thu, Mar 19, 2020 at 03:34:12PM +0100, Matthias Brugger wrote:
>>
>>
>> On 19/03/2020 14:56, David Sterba wrote:
>>> On Thu, Mar 19, 2020 at 02:33:28PM +0100, Matthias Brugger wrote:
>>>>>  		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;
>>>>
>>>> in U-Boot we use 4K page sizes, but the OS could use another page size (16K or
>>>> 64k). Would we need to adapt that code to reflect which page size is used on the
>>>> medium we want to access?
>>>
>>> Yes, it is the 'sectorsize' as it's set up in fs_info or it's equivalent
>>> in uboot. For kernel the page size == sectorsize is kind of implicit and
>>> verified at mount time.
>>>
>>
>> Does this mean we would need to add a Kconfig option to set the sectorsize in
>> U-Boot?
> 
> No, the value depends on the filesystem so it can't be a config option.
> What I mean is btrfs_super_block::sectorsize, where the superblock is
> btrfs_info::sb.
> 

Sorry for the delayed reply. (Stupid filter setup).

Currently most Uboot boards should use the same page size setup for its
kernel, and most btrfs uses 4K sector size.

So for Uboot it should be no problem.

Although the best practice is to read the fs_info::sectorsize as David
mentioned, but the code base doesn't allow us to do that yet.

So I'm going to backport the read part code from btrfs-progs in the
near-future, and completely solve it, making it sector size independent.

Would this plan look OK to you? Or we need to wait for the full
re-implementation?

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Marek Behun]

On Tue, 24 Mar 2020 19:03:30 +0800
Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:

> Sorry for the delayed reply. (Stupid filter setup).
> 
> Currently most Uboot boards should use the same page size setup for its
> kernel, and most btrfs uses 4K sector size.
> 
> So for Uboot it should be no problem.
> 
> Although the best practice is to read the fs_info::sectorsize as David
> mentioned, but the code base doesn't allow us to do that yet.
> 
> So I'm going to backport the read part code from btrfs-progs in the
> near-future, and completely solve it, making it sector size independent.
> 
> Would this plan looks sound? Or we need to wait for the full
> re-implementation?
> 
> Thanks,
> Qu
> 

The situation is Linux is such that btrfs sectorsize must be same as
PAGE_SIZE, otherwise the Linux btrfs driver won't work. AFAIK there are
only few architectures where PAGE_SIZE is not 4 KiB. btrfs filesystems
created there cannot be mounted on systems with PAGE_SIZE = 4 KiB.

I don't know if U-Boot is used on non 4KiB PAGE_SIZE boards. If it is,
it should be solved, but I would check that before complicating the
code.

[PATCH 1/2] uboot: fs/btrfs: Use LZO_LEN to replace immediate number

[Qu Wenruo]

Just a cleanup. The immediate number makes my eye hurt.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/compression.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 346875d45a1b..4ef44ce11485 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -12,36 +12,38 @@
 #include <u-boot/zlib.h>
 #include <asm/unaligned.h>
 
+/* Header for each segment, LE32, recording the compressed size */
+#define LZO_LEN		4
 static u32 decompress_lzo(const u8 *cbuf, u32 clen, u8 *dbuf, u32 dlen)
 {
 	u32 tot_len, in_len, res;
 	size_t out_len;
 	int ret;
 
-	if (clen < 4)
+	if (clen < LZO_LEN)
 		return -1;
 
 	tot_len = le32_to_cpu(get_unaligned((u32 *)cbuf));
-	cbuf += 4;
-	clen -= 4;
-	tot_len -= 4;
+	cbuf += LZO_LEN;
+	clen -= LZO_LEN;
+	tot_len -= LZO_LEN;
 
 	if (tot_len == 0 && dlen)
 		return -1;
-	if (tot_len < 4)
+	if (tot_len < LZO_LEN)
 		return -1;
 
 	res = 0;
 
-	while (tot_len > 4) {
+	while (tot_len > LZO_LEN) {
 		in_len = le32_to_cpu(get_unaligned((u32 *)cbuf));
-		cbuf += 4;
-		clen -= 4;
+		cbuf += LZO_LEN;
+		clen -= LZO_LEN;
 
-		if (in_len > clen || tot_len < 4 + in_len)
+		if (in_len > clen || tot_len < LZO_LEN + in_len)
 			return -1;
 
-		tot_len -= 4 + in_len;
+		tot_len -= (LZO_LEN + in_len);
 
 		out_len = dlen;
 		ret = lzo1x_decompress_safe(cbuf, in_len, dbuf, &out_len);
-- 
2.25.1