[PATCH v3 0/6] Fix long standing -EOPNOTSUPP problem caused by large inline extent

[PATCH v3 0/6] Fix long standing -EOPNOTSUPP problem caused by large inline extent

[Qu Wenruo]

The patch is based on v4.15.1, and is designed to replace the old patch
in devel branch.

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 in several parts of btrfs-progs, any tool which creates
file extent is involved, including:
1) btrfs-convert
2) mkfs --rootdir

This patchset fixes the problems in convert (both ext2 and reiserfs),
mkfs --rootdir, then add check support for both original and lowmem
mode, and finally adds 2 test cases, one for mkfs and one for convert.

For mkfs test case, it can already be exposed by misc/002, but a
pin-point test case will be much better.

changelog:
v2:
  Don't modify BTRFS_MAX_INLINE_DATA_SIZE(), but add extra check to
  callers who create file extents.
v3:
  Merge fixes for convert.
  Add real commit message for convert fixes.
  Use $TEST_TOP to replace cooperate with stand alone test cases.
  Use for loops to make the new test case shorter.

Qu Wenruo (6):
  btrfs-progs: convert: Fix inline file extent creation condition
  btrfs-progs: mkfs/rootdir: Fix inline extent creation check
  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
  btrfs-progs: test/mkfs: Add test case for rootdir inline extent size

 check/main.c                                       | 16 ++++++++
 check/mode-lowmem.c                                | 28 +++++++++++++
 check/mode-original.h                              |  1 +
 convert/source-ext2.c                              |  2 +-
 convert/source-reiserfs.c                          |  3 +-
 mkfs/rootdir.c                                     |  6 ++-
 .../016-invalid-large-inline-extent/test.sh        | 22 ++++++++++
 tests/mkfs-tests/014-rootdir-inline-extent/test.sh | 47 ++++++++++++++++++++++
 8 files changed, 121 insertions(+), 4 deletions(-)
 create mode 100755 tests/convert-tests/016-invalid-large-inline-extent/test.sh
 create mode 100755 tests/mkfs-tests/014-rootdir-inline-extent/test.sh

-- 
2.16.2

[PATCH v3 1/6] btrfs-progs: convert: Fix inline file extent creation condition

[Qu Wenruo]

[Bug]
On btrfs converted from ext*, one user reported the following kernel
warning:
 ------------[ cut here ]------------
 BTRFS: Transaction aborted (error -95)
 WARNING: CPU: 0 PID: 324 at fs/btrfs/inode.c:3042 btrfs_finish_ordered_io+0x7ab/0x850 [btrfs]
 Workqueue: btrfs-endio-write btrfs_endio_write_helper [btrfs]
 RIP: 0010:btrfs_finish_ordered_io+0x7ab/0x850 [btrfs]
 ...
 Call Trace:
  normal_work_helper+0x39/0x370 [btrfs]
  process_one_work+0x1ce/0x410
  worker_thread+0x2b/0x3d0
  ? process_one_work+0x410/0x410
  kthread+0x113/0x130
  ? kthread_create_on_node+0x70/0x70
  ? do_syscall_64+0x74/0x190
  ? SyS_exit_group+0x10/0x10
  ret_from_fork+0x35/0x40
 ---[ end trace c8ed62ff6a525901 ]---
 BTRFS: error (device dm-2) in
btrfs_finish_ordered_io:3042: errno=-95 unknown
 BTRFS info (device dm-2): forced readonly
 BTRFS error (device dm-2): pending csums is 6447104

[Cause]
The call trace and the unique return value points to
__btrfs_drop_extents(), when we tries to drop pages of an inline extent,
we will trigger such -EOPNOTSUPP.

However kernel has limitation on the size of inline file extent
(sector size for ram size and sector size - 1for on-disk size),
btrfs-convert doesn't have the same limitation, resulting much larger
file extent.

The lack of correct inline extent size check dates back to 2008 when
btrfs-convert is added into btrfs-progs.

[Fix]
Fix the inline extent creation condition, not only using
BTRFS_MAX_INLINE_DATA_SIZE(), which is only the maximum size of inline
data according to nodesize, but also limit it against sector size.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 convert/source-ext2.c     | 2 +-
 convert/source-reiserfs.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/convert/source-ext2.c b/convert/source-ext2.c
index b1492c78693d..e13fbe00415a 100644
--- a/convert/source-ext2.c
+++ b/convert/source-ext2.c
@@ -310,7 +310,7 @@ static int ext2_create_file_extents(struct btrfs_trans_handle *trans,
 	if (ret)
 		goto fail;
 	if ((convert_flags & CONVERT_FLAG_INLINE_DATA) && data.first_block == 0
-	    && data.num_blocks > 0
+	    && data.num_blocks > 0 && inode_size < sectorsize
 	    && inode_size <= BTRFS_MAX_INLINE_DATA_SIZE(root->fs_info)) {
 		u64 num_bytes = data.num_blocks * sectorsize;
 		u64 disk_bytenr = data.disk_block * sectorsize;
diff --git a/convert/source-reiserfs.c b/convert/source-reiserfs.c
index 39d6f0728bd3..eeb68d962c5d 100644
--- a/convert/source-reiserfs.c
+++ b/convert/source-reiserfs.c
@@ -376,7 +376,8 @@ static int reiserfs_convert_tail(struct btrfs_trans_handle *trans,
 	u64 isize;
 	int ret;
 
-	if (length >= BTRFS_MAX_INLINE_DATA_SIZE(root->fs_info))
+	if (length >= BTRFS_MAX_INLINE_DATA_SIZE(root->fs_info) ||
+	    length >= root->fs_info->sectorsize)
 		return convert_direct(trans, root, objectid, inode, body,
 				      length, offset, convert_flags);
 
-- 
2.16.2

[PATCH v3 2/6] btrfs-progs: mkfs/rootdir: Fix inline extent creation check

[Qu Wenruo]

Just like convert, we need extra check against sector size for creating
inline extent.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 mkfs/rootdir.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mkfs/rootdir.c b/mkfs/rootdir.c
index e06b65ac13e4..a1d223a2408a 100644
--- a/mkfs/rootdir.c
+++ b/mkfs/rootdir.c
@@ -139,7 +139,8 @@ static int fill_inode_item(struct btrfs_trans_handle *trans,
 	}
 	if (S_ISREG(src->st_mode)) {
 		btrfs_set_stack_inode_size(dst, (u64)src->st_size);
-		if (src->st_size <= BTRFS_MAX_INLINE_DATA_SIZE(root->fs_info))
+		if (src->st_size <= BTRFS_MAX_INLINE_DATA_SIZE(root->fs_info) &&
+		    src->st_size < sectorsize)
 			btrfs_set_stack_inode_nbytes(dst, src->st_size);
 		else {
 			blocks = src->st_size / sectorsize;
@@ -327,7 +328,8 @@ static int add_file_items(struct btrfs_trans_handle *trans,
 	if (st->st_size % sectorsize)
 		blocks += 1;
 
-	if (st->st_size <= BTRFS_MAX_INLINE_DATA_SIZE(root->fs_info)) {
+	if (st->st_size <= BTRFS_MAX_INLINE_DATA_SIZE(root->fs_info) &&
+	    st->st_size < sectorsize) {
 		char *buffer = malloc(st->st_size);
 
 		if (!buffer) {
-- 
2.16.2

[PATCH v3 3/6] btrfs-progs: check/original mode: Check inline extent size

[Qu Wenruo]

For inline compressed file extent, kernel doesn't allow inline extent
ram size larger than sector size and on-disk inline extent size should
not exceed BTRFS_MAX_INLINE_DATA_SIZE().

For inline uncompressed file extent, kernel doesn't allow inline extent
ram and on-disk size larger than either BTRFS_MAX_INLINE_DATA_SIZE() or
sector size.

Check it in original mode.

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

diff --git a/check/main.c b/check/main.c
index 97baae583f04..7821faa929a3 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)
@@ -1433,6 +1435,8 @@ static int process_file_extent(struct btrfs_root *root,
 	u64 disk_bytenr = 0;
 	u64 extent_offset = 0;
 	u64 mask = root->fs_info->sectorsize - 1;
+	u32 max_inline_size = min_t(u32, mask,
+				BTRFS_MAX_INLINE_DATA_SIZE(root->fs_info));
 	int extent_type;
 	int ret;
 
@@ -1458,9 +1462,21 @@ static int process_file_extent(struct btrfs_root *root,
 	extent_type = btrfs_file_extent_type(eb, fi);
 
 	if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
+		u8 compression = btrfs_file_extent_compression(eb, fi);
+		struct btrfs_item *item = btrfs_item_nr(slot);
+
 		num_bytes = btrfs_file_extent_inline_len(eb, slot, fi);
 		if (num_bytes == 0)
 			rec->errors |= I_ERR_BAD_FILE_EXTENT;
+		if (compression) {
+			if (btrfs_file_extent_inline_item_len(eb, item) >
+			    max_inline_size ||
+			    num_bytes > root->fs_info->sectorsize)
+				rec->errors |= I_ERR_FILE_EXTENT_TOO_LARGE;
+		} else {
+			if (num_bytes > max_inline_size)
+				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

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

[Qu Wenruo]

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

diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
index 62bcf3d2e126..e5446f9c7280 100644
--- a/check/mode-lowmem.c
+++ b/check/mode-lowmem.c
@@ -1417,6 +1417,8 @@ 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 = min_t(u32, root->fs_info->sectorsize - 1,
+				BTRFS_MAX_INLINE_DATA_SIZE(root->fs_info));
 	unsigned int extent_type;
 	unsigned int is_hole;
 	int compressed = 0;
@@ -1440,6 +1442,32 @@ static int check_file_extent(struct btrfs_root *root, struct btrfs_key *fkey,
 				root->objectid, fkey->objectid, fkey->offset);
 			err |= FILE_EXTENT_ERROR;
 		}
+		if (compressed) {
+			if (extent_num_bytes > root->fs_info->sectorsize) {
+				error(
+"root %llu EXTENT_DATA[%llu %llu] too large inline extent ram size, have %llu, max: %u",
+					root->objectid, fkey->objectid,
+					fkey->offset, extent_num_bytes,
+					root->fs_info->sectorsize - 1);
+				err |= FILE_EXTENT_ERROR;
+			}
+			if (item_inline_len > max_inline_extent_size) {
+				error(
+"root %llu EXTENT_DATA[%llu %llu] too large inline extent on-disk size, have %u, max: %u",
+					root->objectid, fkey->objectid,
+					fkey->offset, item_inline_len,
+					max_inline_extent_size);
+				err |= FILE_EXTENT_ERROR;
+			}
+		} else {
+			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

[PATCH v3 5/6] 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

[PATCH v3 6/6] btrfs-progs: test/mkfs: Add test case for rootdir inline extent size

[Qu Wenruo]

Add a test case for mkfs --rootdir, using files with different file
sizes to check if invalid large inline extent could exist.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/mkfs-tests/014-rootdir-inline-extent/test.sh | 47 ++++++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100755 tests/mkfs-tests/014-rootdir-inline-extent/test.sh

diff --git a/tests/mkfs-tests/014-rootdir-inline-extent/test.sh b/tests/mkfs-tests/014-rootdir-inline-extent/test.sh
new file mode 100755
index 000000000000..167ced1e4987
--- /dev/null
+++ b/tests/mkfs-tests/014-rootdir-inline-extent/test.sh
@@ -0,0 +1,47 @@
+#!/bin/bash
+# Regression test for mkfs.btrfs --rootdir with inline file extents
+# For any large inline file extent, btrfs check could already report it
+
+source "$TEST_TOP/common"
+
+check_global_prereq fallocate
+check_prereq mkfs.btrfs
+
+prepare_test_dev
+
+tmp=$(mktemp -d --tmpdir btrfs-progs-mkfs.rootdirXXXXXXX)
+
+pagesize=$(getconf PAGESIZE)
+create_file()
+{
+	local size=$1
+	# Reuse size as filename
+	run_check fallocate -l $size "$tmp/$size"
+}
+
+test_mkfs_rootdir()
+{
+	nodesize=$1
+	run_check "$TOP/mkfs.btrfs" --nodesize $nodesize -f --rootdir "$tmp" \
+		"$TEST_DEV"
+	run_check $SUDO_HELPER "$TOP/btrfs" check "$TEST_DEV"
+}
+
+# Use power of 2 from 512 to 64K (maximum node size) as base file size
+for i in 512 1024 2048 4096 8192 16384 32768; do
+	create_file $(($i - 102))
+	# 101 is the overhead size for max inline extent
+	create_file $(($i - 101))
+	create_file $(($i - 100))
+
+	create_file $(($i - 1))
+	create_file $i
+	create_file $(($i + 1))
+done
+
+for nodesize in 4096 8192 16384 32768 65536; do
+	if [ $nodesize -ge $pagesize ]; then
+		test_mkfs_rootdir $nodesize
+	fi
+done
+rm -rf -- "$tmp"
-- 
2.16.2

Re: [PATCH v3 0/6] Fix long standing -EOPNOTSUPP problem caused by large inline extent

[David Sterba]

On Tue, Mar 20, 2018 at 02:42:23PM +0800, Qu Wenruo wrote:
> The patch is based on v4.15.1, and is designed to replace the old patch
> in devel branch.
> 
> 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 in several parts of btrfs-progs, any tool which creates
> file extent is involved, including:
> 1) btrfs-convert
> 2) mkfs --rootdir
> 
> This patchset fixes the problems in convert (both ext2 and reiserfs),
> mkfs --rootdir, then add check support for both original and lowmem
> mode, and finally adds 2 test cases, one for mkfs and one for convert.
> 
> For mkfs test case, it can already be exposed by misc/002, but a
> pin-point test case will be much better.
> 
> changelog:
> v2:
>   Don't modify BTRFS_MAX_INLINE_DATA_SIZE(), but add extra check to
>   callers who create file extents.
> v3:
>   Merge fixes for convert.
>   Add real commit message for convert fixes.
>   Use $TEST_TOP to replace cooperate with stand alone test cases.
>   Use for loops to make the new test case shorter.
> 
> Qu Wenruo (6):
>   btrfs-progs: convert: Fix inline file extent creation condition
>   btrfs-progs: mkfs/rootdir: Fix inline extent creation check
>   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
>   btrfs-progs: test/mkfs: Add test case for rootdir inline extent size

Applied, thanks. I had to fix the test, fallocate may fail, so a file of
given size is created directly.

I hope this bug is fixed. My plan was to release 4.15.2 with just
bugfixes but as this took some time to fix, there likely will be no
minor release before 4.16 as kernel is about to be released next week.

Re: [PATCH v3 0/6] Fix long standing -EOPNOTSUPP problem caused by large inline extent

[Qu Wenruo]

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



On 2018年03月21日 23:51, David Sterba wrote:
> On Tue, Mar 20, 2018 at 02:42:23PM +0800, Qu Wenruo wrote:
>> The patch is based on v4.15.1, and is designed to replace the old patch
>> in devel branch.
>>
>> 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 in several parts of btrfs-progs, any tool which creates
>> file extent is involved, including:
>> 1) btrfs-convert
>> 2) mkfs --rootdir
>>
>> This patchset fixes the problems in convert (both ext2 and reiserfs),
>> mkfs --rootdir, then add check support for both original and lowmem
>> mode, and finally adds 2 test cases, one for mkfs and one for convert.
>>
>> For mkfs test case, it can already be exposed by misc/002, but a
>> pin-point test case will be much better.
>>
>> changelog:
>> v2:
>>   Don't modify BTRFS_MAX_INLINE_DATA_SIZE(), but add extra check to
>>   callers who create file extents.
>> v3:
>>   Merge fixes for convert.
>>   Add real commit message for convert fixes.
>>   Use $TEST_TOP to replace cooperate with stand alone test cases.
>>   Use for loops to make the new test case shorter.
>>
>> Qu Wenruo (6):
>>   btrfs-progs: convert: Fix inline file extent creation condition
>>   btrfs-progs: mkfs/rootdir: Fix inline extent creation check
>>   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
>>   btrfs-progs: test/mkfs: Add test case for rootdir inline extent size
> 
> Applied, thanks. I had to fix the test, fallocate may fail, so a file of
> given size is created directly.

The fix looks good, and I learn a new trick.

But I'm wondering how could it fail.

Nowadays /tmp should be tmpfs created by systemd, which supports fallocate.
Or is the CI things doesn't follow this?

Thanks,
Qu

> 
> I hope this bug is fixed. My plan was to release 4.15.2 with just
> bugfixes but as this took some time to fix, there likely will be no
> minor release before 4.16 as kernel is about to be released next week.
> --
> 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 v3 0/6] Fix long standing -EOPNOTSUPP problem caused by large inline extent

[David Sterba]

On Thu, Mar 22, 2018 at 08:12:31AM +0800, Qu Wenruo wrote:
> > Applied, thanks. I had to fix the test, fallocate may fail, so a file of
> > given size is created directly.
> 
> The fix looks good, and I learn a new trick.
> 
> But I'm wondering how could it fail.
> 
> Nowadays /tmp should be tmpfs created by systemd, which supports fallocate.
> Or is the CI things doesn't follow this?

Unless /tmp is not on tmpfs because this is not always the smartest
thing to do and also when /tmp is on ext3 for historical reasons.