[PATCH 0/4] Fix long standing -EOPNOTSUPP problem caused by

[PATCH 0/4] Fix long standing -EOPNOTSUPP problem caused by

[Qu Wenruo]

Kernel doesn't support dropping range inside inline extent, and prevents
such thing happening by limiting max inline extent size to
min(max_inline, sectorsize - 1) in cow_file_range_inline().

However btrfs-progs only inherit the BTRFS_MAX_INLINE_DATA_SIZE() macro,
which doesn't have sectorsize check.
And since btrfs-progs defaults to 16K nodesize, above macro allows large
inline extent over 15K size.

This leads to unexpected kernel behavior.

The bug exists from the very beginning of btrfs-convert, dating back to
2008 when btrfs-convert is first introduced.

Qu Wenruo (4):
  btrfs-progs: Limit inline extent below page size
  btrfs-progs: check/original mode: Check inline extent size
  btrfs-progs: check/lowmem mode: Check inline extent size
  btrfs-progs: test/convert: Add test case for invalid large inline data
    extent

 check/main.c                                       |  4 ++++
 check/mode-lowmem.c                                |  8 ++++++++
 check/mode-original.h                              |  1 +
 ctree.h                                            | 11 +++++++++--
 .../016-invalid-large-inline-extent/test.sh        | 22 ++++++++++++++++++++++
 5 files changed, 44 insertions(+), 2 deletions(-)
 create mode 100755 tests/convert-tests/016-invalid-large-inline-extent/test.sh

-- 
2.16.2

[PATCH 1/4] btrfs-progs: Limit inline extent below page size

[Qu Wenruo]

Kernel doesn't support to drop extent inside an inlined extent.
And kernel tends to limit inline extent just below sectorsize, so also
limit it in btrfs-progs.

This fixes unexpected -EOPNOTSUPP error from __btrfs_drop_extents() on
converted btrfs.

Fixes: 806528b8755f ("Add Yan Zheng's ext3->btrfs conversion program")
Reported-by: Peter Y. Chuang <peteryuchuang@gmail.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 ctree.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/ctree.h b/ctree.h
index 17cdac76c58c..0282deef339b 100644
--- a/ctree.h
+++ b/ctree.h
@@ -20,6 +20,7 @@
 #define __BTRFS_CTREE_H__
 
 #include <stdbool.h>
+#include "internal.h"
 
 #if BTRFS_FLAT_INCLUDES
 #include "list.h"
@@ -1195,8 +1196,14 @@ static inline u32 BTRFS_NODEPTRS_PER_BLOCK(const struct btrfs_fs_info *info)
 	(offsetof(struct btrfs_file_extent_item, disk_bytenr))
 static inline u32 BTRFS_MAX_INLINE_DATA_SIZE(const struct btrfs_fs_info *info)
 {
-	return BTRFS_MAX_ITEM_SIZE(info) -
-		BTRFS_FILE_EXTENT_INLINE_DATA_START;
+	/*
+	 * Inline extent larger than pagesize could lead to kernel unexpected
+	 * error when dropping extents, so we need to limit the inline extent
+	 * size to less than sectorsize.
+	 */
+	return min_t(u32, info->sectorsize - 1,
+		     BTRFS_MAX_ITEM_SIZE(info) -
+		     BTRFS_FILE_EXTENT_INLINE_DATA_START);
 }
 
 static inline u32 BTRFS_MAX_XATTR_SIZE(const struct btrfs_fs_info *info)
-- 
2.16.2

Re: [PATCH 1/4] btrfs-progs: Limit inline extent below page size

[Nikolay Borisov]

On  1.03.2018 04:47, Qu Wenruo wrote:
> Kernel doesn't support to drop extent inside an inlined extent.
> And kernel tends to limit inline extent just below sectorsize, so also
> limit it in btrfs-progs.
> 
> This fixes unexpected -EOPNOTSUPP error from __btrfs_drop_extents() on
> converted btrfs.
> 
> Fixes: 806528b8755f ("Add Yan Zheng's ext3->btrfs conversion program")
> Reported-by: Peter Y. Chuang <peteryuchuang@gmail.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  ctree.h | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/ctree.h b/ctree.h
> index 17cdac76c58c..0282deef339b 100644
> --- a/ctree.h
> +++ b/ctree.h
> @@ -20,6 +20,7 @@
>  #define __BTRFS_CTREE_H__
>  
>  #include <stdbool.h>
> +#include "internal.h"
>  
>  #if BTRFS_FLAT_INCLUDES
>  #include "list.h"
> @@ -1195,8 +1196,14 @@ static inline u32 BTRFS_NODEPTRS_PER_BLOCK(const struct btrfs_fs_info *info)
>  	(offsetof(struct btrfs_file_extent_item, disk_bytenr))
>  static inline u32 BTRFS_MAX_INLINE_DATA_SIZE(const struct btrfs_fs_info *info)
>  {
> -	return BTRFS_MAX_ITEM_SIZE(info) -
> -		BTRFS_FILE_EXTENT_INLINE_DATA_START;
> +	/*
> +	 * Inline extent larger than pagesize could lead to kernel unexpected
> +	 * error when dropping extents, so we need to limit the inline extent
> +	 * size to less than sectorsize.
> +	 */
> +	return min_t(u32, info->sectorsize - 1,
> +		     BTRFS_MAX_ITEM_SIZE(info) -
> +		     BTRFS_FILE_EXTENT_INLINE_DATA_START);
>  }

Isn't the same change required in the kernel as well ?

>  
>  static inline u32 BTRFS_MAX_XATTR_SIZE(const struct btrfs_fs_info *info)
> 

Re: [PATCH 1/4] btrfs-progs: Limit inline extent below page size

[Qu Wenruo]

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



On 2018年03月02日 01:47, Nikolay Borisov wrote:
> 
> 
> On  1.03.2018 04:47, Qu Wenruo wrote:
>> Kernel doesn't support to drop extent inside an inlined extent.
>> And kernel tends to limit inline extent just below sectorsize, so also
>> limit it in btrfs-progs.
>>
>> This fixes unexpected -EOPNOTSUPP error from __btrfs_drop_extents() on
>> converted btrfs.
>>
>> Fixes: 806528b8755f ("Add Yan Zheng's ext3->btrfs conversion program")
>> Reported-by: Peter Y. Chuang <peteryuchuang@gmail.com>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  ctree.h | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/ctree.h b/ctree.h
>> index 17cdac76c58c..0282deef339b 100644
>> --- a/ctree.h
>> +++ b/ctree.h
>> @@ -20,6 +20,7 @@
>>  #define __BTRFS_CTREE_H__
>>  
>>  #include <stdbool.h>
>> +#include "internal.h"
>>  
>>  #if BTRFS_FLAT_INCLUDES
>>  #include "list.h"
>> @@ -1195,8 +1196,14 @@ static inline u32 BTRFS_NODEPTRS_PER_BLOCK(const struct btrfs_fs_info *info)
>>  	(offsetof(struct btrfs_file_extent_item, disk_bytenr))
>>  static inline u32 BTRFS_MAX_INLINE_DATA_SIZE(const struct btrfs_fs_info *info)
>>  {
>> -	return BTRFS_MAX_ITEM_SIZE(info) -
>> -		BTRFS_FILE_EXTENT_INLINE_DATA_START;
>> +	/*
>> +	 * Inline extent larger than pagesize could lead to kernel unexpected
>> +	 * error when dropping extents, so we need to limit the inline extent
>> +	 * size to less than sectorsize.
>> +	 */
>> +	return min_t(u32, info->sectorsize - 1,
>> +		     BTRFS_MAX_ITEM_SIZE(info) -
>> +		     BTRFS_FILE_EXTENT_INLINE_DATA_START);
>>  }
> 
> Isn't the same change required in the kernel as well ?

Yep, kernel patch underway.

Although, kernel puts a lot of extra check into cow_file_range_inline()
instead of using the macro directly, I would like to enhance the check
in parse_options() so we could get a correct max_inline prompt.
(Currently we could pass max_inline=4096, and kernel prompt also shows
4096, while we can only inline 4095 bytes)

Thanks,
Qu

> 
>>  
>>  static inline u32 BTRFS_MAX_XATTR_SIZE(const struct btrfs_fs_info *info)
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


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

Re: [PATCH 1/4] btrfs-progs: Limit inline extent below page size

[David Sterba]

On Thu, Mar 01, 2018 at 10:47:44AM +0800, Qu Wenruo wrote:
> Kernel doesn't support to drop extent inside an inlined extent.
> And kernel tends to limit inline extent just below sectorsize, so also
> limit it in btrfs-progs.
> 
> This fixes unexpected -EOPNOTSUPP error from __btrfs_drop_extents() on
> converted btrfs.
> 
> Fixes: 806528b8755f ("Add Yan Zheng's ext3->btrfs conversion program")
> Reported-by: Peter Y. Chuang <peteryuchuang@gmail.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>

This patch breaks test fsck/020-extent-ref-cases for image
inline_regular_coexist.img .

[PATCH 2/4] btrfs-progs: check/original mode: Check inline extent size

[Qu Wenruo]

Kernel doesn't allow inline extent equal or larger than 4K.
And for inline extent larger than 4K, __btrfs_drop_extents() can return
-EOPNOTSUPP and cause unexpected error.

Check it in original mode.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c          | 4 ++++
 check/mode-original.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/check/main.c b/check/main.c
index 97baae583f04..ce41550ab16a 100644
--- a/check/main.c
+++ b/check/main.c
@@ -560,6 +560,8 @@ static void print_inode_error(struct btrfs_root *root, struct inode_record *rec)
 		fprintf(stderr, ", bad file extent");
 	if (errors & I_ERR_FILE_EXTENT_OVERLAP)
 		fprintf(stderr, ", file extent overlap");
+	if (errors & I_ERR_FILE_EXTENT_TOO_LARGE)
+		fprintf(stderr, ", inline file extent too large");
 	if (errors & I_ERR_FILE_EXTENT_DISCOUNT)
 		fprintf(stderr, ", file extent discount");
 	if (errors & I_ERR_DIR_ISIZE_WRONG)
@@ -1461,6 +1463,8 @@ static int process_file_extent(struct btrfs_root *root,
 		num_bytes = btrfs_file_extent_inline_len(eb, slot, fi);
 		if (num_bytes == 0)
 			rec->errors |= I_ERR_BAD_FILE_EXTENT;
+		if (num_bytes > BTRFS_MAX_INLINE_DATA_SIZE(root->fs_info))
+			rec->errors |= I_ERR_FILE_EXTENT_TOO_LARGE;
 		rec->found_size += num_bytes;
 		num_bytes = (num_bytes + mask) & ~mask;
 	} else if (extent_type == BTRFS_FILE_EXTENT_REG ||
diff --git a/check/mode-original.h b/check/mode-original.h
index f859af478f0f..368de692fdd1 100644
--- a/check/mode-original.h
+++ b/check/mode-original.h
@@ -185,6 +185,7 @@ struct file_extent_hole {
 #define I_ERR_SOME_CSUM_MISSING		(1 << 12)
 #define I_ERR_LINK_COUNT_WRONG		(1 << 13)
 #define I_ERR_FILE_EXTENT_ORPHAN	(1 << 14)
+#define I_ERR_FILE_EXTENT_TOO_LARGE	(1 << 15)
 
 struct inode_record {
 	struct list_head backrefs;
-- 
2.16.2

Re: [PATCH 2/4] btrfs-progs: check/original mode: Check inline extent size

[Su Yue]

On 03/01/2018 10:47 AM, Qu Wenruo wrote:
> Kernel doesn't allow inline extent equal or larger than 4K.
> And for inline extent larger than 4K, __btrfs_drop_extents() can return
> -EOPNOTSUPP and cause unexpected error.
> 
> Check it in original mode.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Looks good to me.

Reviewed-by: Su Yue <suy.fnst@cn.fujitsu.com>

> ---
>   check/main.c          | 4 ++++
>   check/mode-original.h | 1 +
>   2 files changed, 5 insertions(+)
> 
> diff --git a/check/main.c b/check/main.c
> index 97baae583f04..ce41550ab16a 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -560,6 +560,8 @@ static void print_inode_error(struct btrfs_root *root, struct inode_record *rec)
>   		fprintf(stderr, ", bad file extent");
>   	if (errors & I_ERR_FILE_EXTENT_OVERLAP)
>   		fprintf(stderr, ", file extent overlap");
> +	if (errors & I_ERR_FILE_EXTENT_TOO_LARGE)
> +		fprintf(stderr, ", inline file extent too large");
>   	if (errors & I_ERR_FILE_EXTENT_DISCOUNT)
>   		fprintf(stderr, ", file extent discount");
>   	if (errors & I_ERR_DIR_ISIZE_WRONG)
> @@ -1461,6 +1463,8 @@ static int process_file_extent(struct btrfs_root *root,
>   		num_bytes = btrfs_file_extent_inline_len(eb, slot, fi);
>   		if (num_bytes == 0)
>   			rec->errors |= I_ERR_BAD_FILE_EXTENT;
> +		if (num_bytes > BTRFS_MAX_INLINE_DATA_SIZE(root->fs_info))
> +			rec->errors |= I_ERR_FILE_EXTENT_TOO_LARGE;
>   		rec->found_size += num_bytes;
>   		num_bytes = (num_bytes + mask) & ~mask;
>   	} else if (extent_type == BTRFS_FILE_EXTENT_REG ||
> diff --git a/check/mode-original.h b/check/mode-original.h
> index f859af478f0f..368de692fdd1 100644
> --- a/check/mode-original.h
> +++ b/check/mode-original.h
> @@ -185,6 +185,7 @@ struct file_extent_hole {
>   #define I_ERR_SOME_CSUM_MISSING		(1 << 12)
>   #define I_ERR_LINK_COUNT_WRONG		(1 << 13)
>   #define I_ERR_FILE_EXTENT_ORPHAN	(1 << 14)
> +#define I_ERR_FILE_EXTENT_TOO_LARGE	(1 << 15)
>   
>   struct inode_record {
>   	struct list_head backrefs;
> 

[PATCH 3/4] btrfs-progs: check/lowmem mode: Check inline extent size

[Qu Wenruo]

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/mode-lowmem.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
index 62bcf3d2e126..44c58163f8f7 100644
--- a/check/mode-lowmem.c
+++ b/check/mode-lowmem.c
@@ -1417,6 +1417,7 @@ static int check_file_extent(struct btrfs_root *root, struct btrfs_key *fkey,
 	u64 csum_found;		/* In byte size, sectorsize aligned */
 	u64 search_start;	/* Logical range start we search for csum */
 	u64 search_len;		/* Logical range len we search for csum */
+	u32 max_inline_extent_size = BTRFS_MAX_INLINE_DATA_SIZE(root->fs_info);
 	unsigned int extent_type;
 	unsigned int is_hole;
 	int compressed = 0;
@@ -1440,6 +1441,13 @@ static int check_file_extent(struct btrfs_root *root, struct btrfs_key *fkey,
 				root->objectid, fkey->objectid, fkey->offset);
 			err |= FILE_EXTENT_ERROR;
 		}
+		if (extent_num_bytes > max_inline_extent_size) {
+			error(
+		"root %llu EXTENT_DATA[%llu %llu] too large inline extent size, have %llu, max: %u",
+				root->objectid, fkey->objectid, fkey->offset,
+				extent_num_bytes, max_inline_extent_size);
+			err |= FILE_EXTENT_ERROR;
+		}
 		if (!compressed && extent_num_bytes != item_inline_len) {
 			error(
 		"root %llu EXTENT_DATA[%llu %llu] wrong inline size, have: %llu, expected: %u",
-- 
2.16.2

Re: [PATCH 3/4] btrfs-progs: check/lowmem mode: Check inline extent size

[Su Yue]

On 03/01/2018 10:47 AM, Qu Wenruo wrote:
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Looks good to me.

Reviewed-by: Su Yue <suy.fnst@cn.fujitsu.com>
> ---
>   check/mode-lowmem.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
> index 62bcf3d2e126..44c58163f8f7 100644
> --- a/check/mode-lowmem.c
> +++ b/check/mode-lowmem.c
> @@ -1417,6 +1417,7 @@ static int check_file_extent(struct btrfs_root *root, struct btrfs_key *fkey,
>   	u64 csum_found;		/* In byte size, sectorsize aligned */
>   	u64 search_start;	/* Logical range start we search for csum */
>   	u64 search_len;		/* Logical range len we search for csum */
> +	u32 max_inline_extent_size = BTRFS_MAX_INLINE_DATA_SIZE(root->fs_info);
>   	unsigned int extent_type;
>   	unsigned int is_hole;
>   	int compressed = 0;
> @@ -1440,6 +1441,13 @@ static int check_file_extent(struct btrfs_root *root, struct btrfs_key *fkey,
>   				root->objectid, fkey->objectid, fkey->offset);
>   			err |= FILE_EXTENT_ERROR;
>   		}
> +		if (extent_num_bytes > max_inline_extent_size) {
> +			error(
> +		"root %llu EXTENT_DATA[%llu %llu] too large inline extent size, have %llu, max: %u",
> +				root->objectid, fkey->objectid, fkey->offset,
> +				extent_num_bytes, max_inline_extent_size);
> +			err |= FILE_EXTENT_ERROR;
> +		}
>   		if (!compressed && extent_num_bytes != item_inline_len) {
>   			error(
>   		"root %llu EXTENT_DATA[%llu %llu] wrong inline size, have: %llu, expected: %u",
> 

[PATCH 4/4] btrfs-progs: test/convert: Add test case for invalid large inline data extent

[Qu Wenruo]

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 .../016-invalid-large-inline-extent/test.sh        | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100755 tests/convert-tests/016-invalid-large-inline-extent/test.sh

diff --git a/tests/convert-tests/016-invalid-large-inline-extent/test.sh b/tests/convert-tests/016-invalid-large-inline-extent/test.sh
new file mode 100755
index 000000000000..f37c7c09d2e7
--- /dev/null
+++ b/tests/convert-tests/016-invalid-large-inline-extent/test.sh
@@ -0,0 +1,22 @@
+#!/bin/bash
+# Check if btrfs-convert refuses to rollback the filesystem, and leave the fs
+# and the convert image untouched
+
+source "$TEST_TOP/common"
+source "$TEST_TOP/common.convert"
+
+setup_root_helper
+prepare_test_dev
+check_prereq btrfs-convert
+check_global_prereq mke2fs
+
+convert_test_prep_fs ext4 mke2fs -t ext4 -b 4096
+
+# Create a 6K file, which should not be inlined
+run_check $SUDO_HELPER dd if=/dev/zero bs=2k count=3 of="$TEST_MNT/file1"
+
+run_check_umount_test_dev
+
+# convert_test_do_convert() will call btrfs check, which should expose any
+# invalid inline extent with too large size
+convert_test_do_convert
-- 
2.16.2

Re: [PATCH 0/4] Fix long standing -EOPNOTSUPP problem caused by

[David Sterba]

On Thu, Mar 01, 2018 at 10:47:43AM +0800, Qu Wenruo wrote:
> Kernel doesn't support dropping range inside inline extent, and prevents
> such thing happening by limiting max inline extent size to
> min(max_inline, sectorsize - 1) in cow_file_range_inline().
> 
> However btrfs-progs only inherit the BTRFS_MAX_INLINE_DATA_SIZE() macro,
> which doesn't have sectorsize check.
> And since btrfs-progs defaults to 16K nodesize, above macro allows large
> inline extent over 15K size.
> 
> This leads to unexpected kernel behavior.
> 
> The bug exists from the very beginning of btrfs-convert, dating back to
> 2008 when btrfs-convert is first introduced.
> 
> Qu Wenruo (4):
>   btrfs-progs: Limit inline extent below page size
>   btrfs-progs: check/original mode: Check inline extent size
>   btrfs-progs: check/lowmem mode: Check inline extent size
>   btrfs-progs: test/convert: Add test case for invalid large inline data
>     extent

Thanks, added to devel. Fixes will be added to 4.15.2.

Re: [PATCH 0/4] Fix long standing -EOPNOTSUPP problem caused by

[Qu Wenruo]

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



On 2018年03月01日 22:53, David Sterba wrote:
> On Thu, Mar 01, 2018 at 10:47:43AM +0800, Qu Wenruo wrote:
>> Kernel doesn't support dropping range inside inline extent, and prevents
>> such thing happening by limiting max inline extent size to
>> min(max_inline, sectorsize - 1) in cow_file_range_inline().
>>
>> However btrfs-progs only inherit the BTRFS_MAX_INLINE_DATA_SIZE() macro,
>> which doesn't have sectorsize check.
>> And since btrfs-progs defaults to 16K nodesize, above macro allows large
>> inline extent over 15K size.
>>
>> This leads to unexpected kernel behavior.
>>
>> The bug exists from the very beginning of btrfs-convert, dating back to
>> 2008 when btrfs-convert is first introduced.
>>
>> Qu Wenruo (4):
>>   btrfs-progs: Limit inline extent below page size
>>   btrfs-progs: check/original mode: Check inline extent size
>>   btrfs-progs: check/lowmem mode: Check inline extent size
>>   btrfs-progs: test/convert: Add test case for invalid large inline data
>>     extent
> 
> Thanks, added to devel. Fixes will be added to 4.15.2.

Just to mention, since we're checking inline extent size, and kernel can
still create such inline extent by symbol linking, I'm afraid we may get
some false alerts.
(Although it's should be less possible, as symbol link with over 4K size
is a little crazy)

Thanks,
Qu


> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


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