Fix long standing -EOPNOTSUPP problem caused by

PATCH Superseded Qu Wenruo 5 participants 12 messages Mar 1, 2018 – Mar 9, 2018 Btrfs file system

Share & tools

Qu Wenruo

author

2018-03-01 02:48 UTC #0 lore raw

cover Fix long standing -EOPNOTSUPP problem caused by

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

Signature · 1 line

2.16.2

Qu Wenruo

author

2018-03-01 02:48 UTC #1 lore raw

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

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 <[email protected]>

Signed-off-by Qu Wenruo <[email protected]>

---
 ctree.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

ctree.h +9 -2

index 17cdac76c58c..0282deef339b 100644
@@ -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)

Signature · 1 line

2.16.2

Nikolay Borisov

2018-03-01 17:47 UTC #2 lore raw

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

Quoted 31 lines · 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 <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>
---
 ctree.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
ctree.h +1 -1
index 17cdac76c58c..0282deef339b 100644
@@ -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) -

ctree.h static inline u32 BTRFS_NODEPTRS_PER_BLOCK(const struct btrfs_fs_info *info) · line 1207

Quoted 10 lines

ctree.h +8 -1

-		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 ?

Quoted 3 lines

ctree.h +0 -0

 static inline u32 BTRFS_MAX_XATTR_SIZE(const struct btrfs_fs_info *info)

Qu Wenruo

2018-03-01 23:44 UTC #3 lore raw

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

Quoted 38 lines · Nikolay Borisov

Quoted 35 lines · 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 <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>
---
 ctree.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
ctree.h +4 -2
index 17cdac76c58c..0282deef339b 100644
@@ -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

Quoted 8 lines

ctree.h static inline u32 BTRFS_NODEPTRS_PER_BLOCK(const struct btrfs_fs_info *info) · line 1207
Quoted 6 lines
ctree.h +5 -0
+	 * 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

Quoted 9 lines

ctree.h static inline u32 BTRFS_NODEPTRS_PER_BLOCK(const struct btrfs_fs_info *info) · line 1209
Quoted 4 lines
ctree.h +0 -0
 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

David Sterba

2018-03-09 16:53 UTC #4 lore raw

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

In reply to Qu Wenruo

Quoted 10 lines · 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 <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>

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

Qu Wenruo

author

2018-03-01 02:48 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---
 check/main.c          | 4 ++++
 check/mode-original.h | 1 +
 2 files changed, 5 insertions(+)

check/main.c +4 -0

index 97baae583f04..ce41550ab16a 100644
@@ -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 ||

check/mode-original.h +1 -0

index f859af478f0f..368de692fdd1 100644
@@ -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;

Signature · 1 line

2.16.2

Su Yue

Reviewed-by on v1 2/4

2018-03-01 02:54 UTC #6 lore raw

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

Quoted 7 lines · 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 <[email protected]>

Looks good to me.

Reviewed-by Su Yue <[email protected]>

Quoted 40 lines

---
  check/main.c          | 4 ++++
  check/mode-original.h | 1 +
  2 files changed, 5 insertions(+)

check/main.c +4 -0

index 97baae583f04..ce41550ab16a 100644
@@ -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 ||

check/mode-original.h +1 -0

index f859af478f0f..368de692fdd1 100644
@@ -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;

Qu Wenruo

author

2018-03-01 02:48 UTC #7 lore raw

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

In reply to Qu Wenruo

Signed-off-by Qu Wenruo <[email protected]>

---
 check/mode-lowmem.c | 8 ++++++++
 1 file changed, 8 insertions(+)

check/mode-lowmem.c +8 -0

index 62bcf3d2e126..44c58163f8f7 100644
@@ -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",

Signature · 1 line

2.16.2

Su Yue

Reviewed-by on v1 3/4

2018-03-01 02:55 UTC #8 lore raw

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

Quoted 1 line · Qu Wenruo

Signed-off-by Qu Wenruo <[email protected]>

Looks good to me.

Reviewed-by Su Yue <[email protected]>

Quoted 31 lines

---
  check/mode-lowmem.c | 8 ++++++++
  1 file changed, 8 insertions(+)

check/mode-lowmem.c +8 -0

index 62bcf3d2e126..44c58163f8f7 100644
@@ -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",

Qu Wenruo

author

2018-03-01 02:48 UTC #9 lore raw

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

In reply to Qu Wenruo

Signed-off-by Qu Wenruo <[email protected]>

---
 .../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

tests/convert-tests/016-invalid-large-inline-extent/test.sh new file +22 -0

new file mode 100755
index 000000000000..f37c7c09d2e7
@@ -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

Signature · 1 line

2.16.2

David Sterba

2018-03-01 14:55 UTC #10 lore raw

In reply to Qu Wenruo

Quoted 14 lines · 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.

Quoted 6 lines

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.

Qu Wenruo

2018-03-02 05:25 UTC #11 lore raw

Quoted 15 lines · David Sterba

Quoted 14 lines · 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.

Quoted 8 lines

Quoted 6 lines
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

Quoted 5 lines

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