Re: [PATCH v2 5/6] ext4: rename xattr_find_entry() and __xattr_check_inode()

Re: [PATCH v2 5/6] ext4: rename xattr_find_entry() and __xattr_check_inode()

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 413 bytes --]

On Wed, Dec 07, 2022 at 03:40:42PM +0800, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
> 
> xattr_find_entry() and __xattr_check_inode() is in EXT4 xattr module. so
> add 'ext4' prefix to unify name style.

What about "Prepend ext4 prefix to xattr_find_entry() and
__xattr_check_inode(), since these functions are in ext4 xattr module."?

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 2/6] ext4: add primary check extended attribute inode in ext4_xattr_check_entries()

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 349 bytes --]

On Wed, Dec 07, 2022 at 03:40:39PM +0800, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
> 
> Add primary check for extended attribute inode, only do hash check when read
> ea_inode's data in ext4_xattr_inode_get().

"..., which is only perform hash checking when reading ..."

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v2 0/6] Fix two issue about ext4 extended attribute

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

Diff v2 vs v1:
1.Modify commit message about "ext4: fix WARNING in ext4_expand_extra_isize_ea"
2.Modify the indentation of arguments about "ext4: rename xattr_find_entry()
and __xattr_check_inode()"

This patchset fix two issues:
1. Patch [1]-[4] fix WARNING in ext4_expand_extra_isize_ea.
2. Patch [6] fix inode leak in 'ext4_xattr_inode_create()'.
3. Patch [5] is cleanup.

Ye Bin (6):
  ext4: fix WARNING in ext4_expand_extra_isize_ea
  ext4: add primary check extended attribute inode in
    ext4_xattr_check_entries()
  ext4: remove unnessary size check in ext4_xattr_inode_get()
  ext4: allocate extended attribute value in vmalloc area
  ext4: rename xattr_find_entry() and __xattr_check_inode()
  ext4: fix inode leak in 'ext4_xattr_inode_create()'

 fs/ext4/xattr.c | 91 ++++++++++++++++++++++++++++++++-----------------
 fs/ext4/xattr.h | 11 ++----
 2 files changed, 62 insertions(+), 40 deletions(-)

-- 
2.31.1

[PATCH v2 1/6] ext4: fix WARNING in ext4_expand_extra_isize_ea

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

Syzbot found the following issue:
------------[ cut here ]------------
WARNING: CPU: 1 PID: 3631 at mm/page_alloc.c:5534 __alloc_pages+0x30a/0x560 mm/page_alloc.c:5534
Modules linked in:
CPU: 1 PID: 3631 Comm: syz-executor261 Not tainted 6.1.0-rc6-syzkaller-00308-g644e9524388a #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/26/2022
RIP: 0010:__alloc_pages+0x30a/0x560 mm/page_alloc.c:5534
RSP: 0018:ffffc90003ccf080 EFLAGS: 00010246
RAX: ffffc90003ccf0e0 RBX: 000000000000000c RCX: 0000000000000000
RDX: 0000000000000028 RSI: 0000000000000000 RDI: ffffc90003ccf108
RBP: ffffc90003ccf198 R08: dffffc0000000000 R09: ffffc90003ccf0e0
R10: fffff52000799e21 R11: 1ffff92000799e1c R12: 0000000000040c40
R13: 1ffff92000799e18 R14: dffffc0000000000 R15: 1ffff92000799e14
FS:  0000555555c10300(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007ffc36f70000 CR3: 00000000744ad000 CR4: 00000000003506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
 __alloc_pages_node include/linux/gfp.h:223 [inline]
 alloc_pages_node include/linux/gfp.h:246 [inline]
 __kmalloc_large_node+0x8a/0x1a0 mm/slab_common.c:1096
 __do_kmalloc_node mm/slab_common.c:943 [inline]
 __kmalloc+0xfe/0x1a0 mm/slab_common.c:968
 kmalloc include/linux/slab.h:558 [inline]
 ext4_xattr_move_to_block fs/ext4/xattr.c:2558 [inline]
 ext4_xattr_make_inode_space fs/ext4/xattr.c:2673 [inline]
 ext4_expand_extra_isize_ea+0xe3f/0x1cd0 fs/ext4/xattr.c:2765
 __ext4_expand_extra_isize+0x2b8/0x3f0 fs/ext4/inode.c:5857
 ext4_try_to_expand_extra_isize fs/ext4/inode.c:5900 [inline]
 __ext4_mark_inode_dirty+0x51a/0x670 fs/ext4/inode.c:5978
 ext4_inline_data_truncate+0x548/0xd00 fs/ext4/inline.c:2021
 ext4_truncate+0x341/0xeb0 fs/ext4/inode.c:4221
 ext4_process_orphan+0x1aa/0x2d0 fs/ext4/orphan.c:339
 ext4_orphan_cleanup+0xb60/0x1340 fs/ext4/orphan.c:474
 __ext4_fill_super fs/ext4/super.c:5515 [inline]
 ext4_fill_super+0x80ed/0x8610 fs/ext4/super.c:5643
 get_tree_bdev+0x400/0x620 fs/super.c:1324
 vfs_get_tree+0x88/0x270 fs/super.c:1531
 do_new_mount+0x289/0xad0 fs/namespace.c:3040
 do_mount fs/namespace.c:3383 [inline]
 __do_sys_mount fs/namespace.c:3591 [inline]
 __se_sys_mount+0x2d3/0x3c0 fs/namespace.c:3568
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd
 </TASK>

Reason is allocate 16M memory by kmalloc, but MAX_ORDER is 11, kmalloc
can allocate maxium size memory is 4M.
XATTR_SIZE_MAX is currently 64k, but EXT4_XATTR_SIZE_MAX is '(1 << 24)',
so 'ext4_xattr_check_entries()' regards this length as legal. Then trigger
warning in 'ext4_xattr_move_to_block()'.
To solve above issue, change EXT4_XATTR_SIZE_MAX from '(1 << 24)' to
XATTR_SIZE_MAX. As VFS limit extended attribute maxium size to XATTR_SIZE_MAX.
So we can assume that there will be no extended attribute with a length greater
than XATTR_SIZE_MAX.

Reported-by: syzbot+4d99a966fd74bdeeec36@syzkaller.appspotmail.com
Fixes: 54dd0e0a1b25 ("ext4: add extra checks to ext4_xattr_block_get()")
Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 fs/ext4/xattr.h | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/fs/ext4/xattr.h b/fs/ext4/xattr.h
index 824faf0b15a8..c71e582b1007 100644
--- a/fs/ext4/xattr.h
+++ b/fs/ext4/xattr.h
@@ -71,15 +71,10 @@ struct ext4_xattr_entry {
 #define IFIRST(hdr) ((struct ext4_xattr_entry *)((hdr)+1))
 
 /*
- * XATTR_SIZE_MAX is currently 64k, but for the purposes of checking
- * for file system consistency errors, we use a somewhat bigger value.
- * This allows XATTR_SIZE_MAX to grow in the future, but by using this
- * instead of INT_MAX for certain consistency checks, we don't need to
- * worry about arithmetic overflows.  (Actually XATTR_SIZE_MAX is
- * defined in include/uapi/linux/limits.h, so changing it is going
- * not going to be trivial....)
+ * Use XATTR_SIZE_MAX to checking for file system consistency errors. Extended
+ * attribute length exceed XATTR_SIZE_MAX is illegal.
  */
-#define EXT4_XATTR_SIZE_MAX (1 << 24)
+#define EXT4_XATTR_SIZE_MAX XATTR_SIZE_MAX
 
 /*
  * The minimum size of EA value when you start storing it in an external inode
-- 
2.31.1

[PATCH v2 2/6] ext4: add primary check extended attribute inode in ext4_xattr_check_entries()

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

Add primary check for extended attribute inode, only do hash check when read
ea_inode's data in ext4_xattr_inode_get().

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 fs/ext4/xattr.c | 41 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 718ef3987f94..eed001eee3ec 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -83,6 +83,9 @@ static __le32 ext4_xattr_hash_entry(char *name, size_t name_len, __le32 *value,
 				    size_t value_count);
 static void ext4_xattr_rehash(struct ext4_xattr_header *);
 
+static int ext4_xattr_inode_iget(struct inode *parent, unsigned long ea_ino,
+				 u32 ea_inode_hash, struct inode **ea_inode);
+
 static const struct xattr_handler * const ext4_xattr_handler_map[] = {
 	[EXT4_XATTR_INDEX_USER]		     = &ext4_xattr_user_handler,
 #ifdef CONFIG_EXT4_FS_POSIX_ACL
@@ -181,9 +184,32 @@ ext4_xattr_handler(int name_index)
 	return handler;
 }
 
+static inline int ext4_xattr_check_extra_inode(struct inode *inode,
+					       struct ext4_xattr_entry *entry)
+{
+	int err;
+	struct inode *ea_inode;
+
+	err = ext4_xattr_inode_iget(inode, le32_to_cpu(entry->e_value_inum),
+				    le32_to_cpu(entry->e_hash), &ea_inode);
+	if (err)
+		return err;
+
+	if (i_size_read(ea_inode) != le32_to_cpu(entry->e_value_size)) {
+		ext4_warning_inode(ea_inode,
+                           "ea_inode file size=%llu entry size=%u",
+                           i_size_read(ea_inode),
+			   le32_to_cpu(entry->e_value_size));
+		err = -EFSCORRUPTED;
+	}
+	iput(ea_inode);
+
+	return err;
+}
+
 static int
-ext4_xattr_check_entries(struct ext4_xattr_entry *entry, void *end,
-			 void *value_start)
+ext4_xattr_check_entries(struct inode *inode, struct ext4_xattr_entry *entry,
+			 void *end, void *value_start)
 {
 	struct ext4_xattr_entry *e = entry;
 
@@ -221,6 +247,10 @@ ext4_xattr_check_entries(struct ext4_xattr_entry *entry, void *end,
 			    size > end - value ||
 			    EXT4_XATTR_SIZE(size) > end - value)
 				return -EFSCORRUPTED;
+		} else if (entry->e_value_inum) {
+			int err = ext4_xattr_check_extra_inode(inode, entry);
+			if (err)
+				return err;
 		}
 		entry = EXT4_XATTR_NEXT(entry);
 	}
@@ -243,8 +273,8 @@ __ext4_xattr_check_block(struct inode *inode, struct buffer_head *bh,
 	error = -EFSBADCRC;
 	if (!ext4_xattr_block_csum_verify(inode, bh))
 		goto errout;
-	error = ext4_xattr_check_entries(BFIRST(bh), bh->b_data + bh->b_size,
-					 bh->b_data);
+	error = ext4_xattr_check_entries(inode, BFIRST(bh),
+					 bh->b_data + bh->b_size, bh->b_data);
 errout:
 	if (error)
 		__ext4_error_inode(inode, function, line, 0, -error,
@@ -268,7 +298,8 @@ __xattr_check_inode(struct inode *inode, struct ext4_xattr_ibody_header *header,
 	if (end - (void *)header < sizeof(*header) + sizeof(u32) ||
 	    (header->h_magic != cpu_to_le32(EXT4_XATTR_MAGIC)))
 		goto errout;
-	error = ext4_xattr_check_entries(IFIRST(header), end, IFIRST(header));
+	error = ext4_xattr_check_entries(inode, IFIRST(header), end,
+					 IFIRST(header));
 errout:
 	if (error)
 		__ext4_error_inode(inode, function, line, 0, -error,
-- 
2.31.1

[PATCH v2 3/6] ext4: remove unnessary size check in ext4_xattr_inode_get()

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

As previous patch add check in ext4_xattr_check_entries(), before call
ext4_xattr_inode_get() will already do xattr entries check.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 fs/ext4/xattr.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index eed001eee3ec..75287422c36c 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -525,14 +525,6 @@ ext4_xattr_inode_get(struct inode *inode, struct ext4_xattr_entry *entry,
 		goto out;
 	}
 
-	if (i_size_read(ea_inode) != size) {
-		ext4_warning_inode(ea_inode,
-				   "ea_inode file size=%llu entry size=%zu",
-				   i_size_read(ea_inode), size);
-		err = -EFSCORRUPTED;
-		goto out;
-	}
-
 	err = ext4_xattr_inode_read(ea_inode, buffer, size);
 	if (err)
 		goto out;
-- 
2.31.1

[PATCH v2 4/6] ext4: allocate extended attribute value in vmalloc area

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

Now, extended attribute value maxium length is 64K. The memory requested here
does not need continuous physical addresses, so it is appropriate to use
kvmalloc to request memory. At the same time, it can also cope with the
situation that the extended attribute will become longer in the future.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 fs/ext4/xattr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 75287422c36c..efa623658c12 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -2579,7 +2579,7 @@ static int ext4_xattr_move_to_block(handle_t *handle, struct inode *inode,
 
 	is = kzalloc(sizeof(struct ext4_xattr_ibody_find), GFP_NOFS);
 	bs = kzalloc(sizeof(struct ext4_xattr_block_find), GFP_NOFS);
-	buffer = kmalloc(value_size, GFP_NOFS);
+	buffer = kvmalloc(value_size, GFP_NOFS);
 	b_entry_name = kmalloc(entry->e_name_len + 1, GFP_NOFS);
 	if (!is || !bs || !buffer || !b_entry_name) {
 		error = -ENOMEM;
@@ -2631,7 +2631,7 @@ static int ext4_xattr_move_to_block(handle_t *handle, struct inode *inode,
 	error = 0;
 out:
 	kfree(b_entry_name);
-	kfree(buffer);
+	kvfree(buffer);
 	if (is)
 		brelse(is->iloc.bh);
 	if (bs)
-- 
2.31.1

[PATCH v2 5/6] ext4: rename xattr_find_entry() and __xattr_check_inode()

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

xattr_find_entry() and __xattr_check_inode() is in EXT4 xattr module. so
add 'ext4' prefix to unify name style.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 fs/ext4/xattr.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index efa623658c12..5c0476ff62c8 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -290,8 +290,9 @@ __ext4_xattr_check_block(struct inode *inode, struct buffer_head *bh,
 
 
 static int
-__xattr_check_inode(struct inode *inode, struct ext4_xattr_ibody_header *header,
-			 void *end, const char *function, unsigned int line)
+__ext4_xattr_check_inode(struct inode *inode,
+			 struct ext4_xattr_ibody_header *header, void *end,
+			 const char *function, unsigned int line)
 {
 	int error = -EFSCORRUPTED;
 
@@ -307,12 +308,12 @@ __xattr_check_inode(struct inode *inode, struct ext4_xattr_ibody_header *header,
 	return error;
 }
 
-#define xattr_check_inode(inode, header, end) \
-	__xattr_check_inode((inode), (header), (end), __func__, __LINE__)
+#define ext4_xattr_check_inode(inode, header, end) \
+	__ext4_xattr_check_inode((inode), (header), (end), __func__, __LINE__)
 
 static int
-xattr_find_entry(struct inode *inode, struct ext4_xattr_entry **pentry,
-		 void *end, int name_index, const char *name, int sorted)
+ext4_xattr_find_entry(struct inode *inode, struct ext4_xattr_entry **pentry,
+		      void *end, int name_index, const char *name, int sorted)
 {
 	struct ext4_xattr_entry *entry, *next;
 	size_t name_len;
@@ -577,7 +578,7 @@ ext4_xattr_block_get(struct inode *inode, int name_index, const char *name,
 	ext4_xattr_block_cache_insert(ea_block_cache, bh);
 	entry = BFIRST(bh);
 	end = bh->b_data + bh->b_size;
-	error = xattr_find_entry(inode, &entry, end, name_index, name, 1);
+	error = ext4_xattr_find_entry(inode, &entry, end, name_index, name, 1);
 	if (error)
 		goto cleanup;
 	size = le32_to_cpu(entry->e_value_size);
@@ -628,11 +629,11 @@ ext4_xattr_ibody_get(struct inode *inode, int name_index, const char *name,
 	raw_inode = ext4_raw_inode(&iloc);
 	header = IHDR(inode, raw_inode);
 	end = (void *)raw_inode + EXT4_SB(inode->i_sb)->s_inode_size;
-	error = xattr_check_inode(inode, header, end);
+	error = ext4_xattr_check_inode(inode, header, end);
 	if (error)
 		goto cleanup;
 	entry = IFIRST(header);
-	error = xattr_find_entry(inode, &entry, end, name_index, name, 0);
+	error = ext4_xattr_find_entry(inode, &entry, end, name_index, name, 0);
 	if (error)
 		goto cleanup;
 	size = le32_to_cpu(entry->e_value_size);
@@ -773,7 +774,7 @@ ext4_xattr_ibody_list(struct dentry *dentry, char *buffer, size_t buffer_size)
 	raw_inode = ext4_raw_inode(&iloc);
 	header = IHDR(inode, raw_inode);
 	end = (void *)raw_inode + EXT4_SB(inode->i_sb)->s_inode_size;
-	error = xattr_check_inode(inode, header, end);
+	error = ext4_xattr_check_inode(inode, header, end);
 	if (error)
 		goto cleanup;
 	error = ext4_xattr_list_entries(dentry, IFIRST(header),
@@ -859,7 +860,7 @@ int ext4_get_inode_usage(struct inode *inode, qsize_t *usage)
 		raw_inode = ext4_raw_inode(&iloc);
 		header = IHDR(inode, raw_inode);
 		end = (void *)raw_inode + EXT4_SB(inode->i_sb)->s_inode_size;
-		ret = xattr_check_inode(inode, header, end);
+		ret = ext4_xattr_check_inode(inode, header, end);
 		if (ret)
 			goto out;
 
@@ -1862,8 +1863,8 @@ ext4_xattr_block_find(struct inode *inode, struct ext4_xattr_info *i,
 		bs->s.first = BFIRST(bs->bh);
 		bs->s.end = bs->bh->b_data + bs->bh->b_size;
 		bs->s.here = bs->s.first;
-		error = xattr_find_entry(inode, &bs->s.here, bs->s.end,
-					 i->name_index, i->name, 1);
+		error = ext4_xattr_find_entry(inode, &bs->s.here, bs->s.end,
+					      i->name_index, i->name, 1);
 		if (error && error != -ENODATA)
 			return error;
 		bs->s.not_found = error;
@@ -2222,12 +2223,12 @@ int ext4_xattr_ibody_find(struct inode *inode, struct ext4_xattr_info *i,
 	is->s.here = is->s.first;
 	is->s.end = (void *)raw_inode + EXT4_SB(inode->i_sb)->s_inode_size;
 	if (ext4_test_inode_state(inode, EXT4_STATE_XATTR)) {
-		error = xattr_check_inode(inode, header, is->s.end);
+		error = ext4_xattr_check_inode(inode, header, is->s.end);
 		if (error)
 			return error;
 		/* Find the named attribute. */
-		error = xattr_find_entry(inode, &is->s.here, is->s.end,
-					 i->name_index, i->name, 0);
+		error = ext4_xattr_find_entry(inode, &is->s.here, is->s.end,
+					      i->name_index, i->name, 0);
 		if (error && error != -ENODATA)
 			return error;
 		is->s.not_found = error;
@@ -2742,7 +2743,7 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
 	min_offs = end - base;
 	total_ino = sizeof(struct ext4_xattr_ibody_header) + sizeof(u32);
 
-	error = xattr_check_inode(inode, header, end);
+	error = ext4_xattr_check_inode(inode, header, end);
 	if (error)
 		goto cleanup;
 
-- 
2.31.1

[PATCH v2 6/6] ext4: fix inode leak in 'ext4_xattr_inode_create()'

[Ye Bin]

From: Ye Bin <yebin10@huawei.com>

There is issue as follows when do setxattr with inject fault:
[localhost]#fsck.ext4  -fn  /dev/sda
e2fsck 1.46.6-rc1 (12-Sep-2022)
Pass 1: Checking inodes, blocks, and sizes
Pass 2: Checking directory structure
Pass 3: Checking directory connectivity
Pass 4: Checking reference counts
Unattached zero-length inode 15.  Clear? no

Unattached inode 15
Connect to /lost+found? no

Pass 5: Checking group summary information

/dev/sda: ********** WARNING: Filesystem still has errors **********

/dev/sda: 15/655360 files (0.0% non-contiguous), 66755/2621440 blocks

Above issue occur in 'ext4_xattr_inode_create()', if 'ext4_mark_inode_dirty()'
failed need to drop inode's i_nlink. Or will lead to inode leak.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 fs/ext4/xattr.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 5c0476ff62c8..6c19d01ba261 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1465,6 +1465,9 @@ static struct inode *ext4_xattr_inode_create(handle_t *handle,
 		if (!err)
 			err = ext4_inode_attach_jinode(ea_inode);
 		if (err) {
+			if (ext4_xattr_inode_dec_ref(handle, ea_inode))
+				ext4_warning_inode(ea_inode,
+					"cleanup dec ref error %d", err);
 			iput(ea_inode);
 			return ERR_PTR(err);
 		}
-- 
2.31.1

Re: [PATCH v2 5/6] ext4: rename xattr_find_entry() and __xattr_check_inode()

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 690 bytes --]

On Wed, Dec 07, 2022 at 02:35:02PM +0700, Bagas Sanjaya wrote:
> On Wed, Dec 07, 2022 at 03:40:42PM +0800, Ye Bin wrote:
> > From: Ye Bin <yebin10@huawei.com>
> > 
> > xattr_find_entry() and __xattr_check_inode() is in EXT4 xattr module. so
> > add 'ext4' prefix to unify name style.
> 
> What about "Prepend ext4 prefix to xattr_find_entry() and
> __xattr_check_inode(), since these functions are in ext4 xattr module."?
> 

Oops, I mean "Prepend ext4 prefix to function names of
xattr_find_entry() and __xattr_check_inode() for consistency with other
functions in ext4 xattr module."

Sorry for inconvenience.

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 6/6] ext4: fix inode leak in 'ext4_xattr_inode_create()'

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 399 bytes --]

On Wed, Dec 07, 2022 at 03:40:43PM +0800, Ye Bin wrote:
> Above issue occur in 'ext4_xattr_inode_create()', if 'ext4_mark_inode_dirty()'
> failed need to drop inode's i_nlink. Or will lead to inode leak.
> 

What about "This occurs in ... . If ... fails, dropping i_nlink of the
inode is needed, otherwise inode leak can occur."?

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/6] ext4: fix WARNING in ext4_expand_extra_isize_ea

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 3589 bytes --]

On Wed, Dec 07, 2022 at 03:40:38PM +0800, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
> 
> Syzbot found the following issue:
> ------------[ cut here ]------------
> WARNING: CPU: 1 PID: 3631 at mm/page_alloc.c:5534 __alloc_pages+0x30a/0x560 mm/page_alloc.c:5534
> Modules linked in:
> CPU: 1 PID: 3631 Comm: syz-executor261 Not tainted 6.1.0-rc6-syzkaller-00308-g644e9524388a #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/26/2022
> RIP: 0010:__alloc_pages+0x30a/0x560 mm/page_alloc.c:5534
> RSP: 0018:ffffc90003ccf080 EFLAGS: 00010246
> RAX: ffffc90003ccf0e0 RBX: 000000000000000c RCX: 0000000000000000
> RDX: 0000000000000028 RSI: 0000000000000000 RDI: ffffc90003ccf108
> RBP: ffffc90003ccf198 R08: dffffc0000000000 R09: ffffc90003ccf0e0
> R10: fffff52000799e21 R11: 1ffff92000799e1c R12: 0000000000040c40
> R13: 1ffff92000799e18 R14: dffffc0000000000 R15: 1ffff92000799e14
> FS:  0000555555c10300(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007ffc36f70000 CR3: 00000000744ad000 CR4: 00000000003506e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  <TASK>
>  __alloc_pages_node include/linux/gfp.h:223 [inline]
>  alloc_pages_node include/linux/gfp.h:246 [inline]
>  __kmalloc_large_node+0x8a/0x1a0 mm/slab_common.c:1096
>  __do_kmalloc_node mm/slab_common.c:943 [inline]
>  __kmalloc+0xfe/0x1a0 mm/slab_common.c:968
>  kmalloc include/linux/slab.h:558 [inline]
>  ext4_xattr_move_to_block fs/ext4/xattr.c:2558 [inline]
>  ext4_xattr_make_inode_space fs/ext4/xattr.c:2673 [inline]
>  ext4_expand_extra_isize_ea+0xe3f/0x1cd0 fs/ext4/xattr.c:2765
>  __ext4_expand_extra_isize+0x2b8/0x3f0 fs/ext4/inode.c:5857
>  ext4_try_to_expand_extra_isize fs/ext4/inode.c:5900 [inline]
>  __ext4_mark_inode_dirty+0x51a/0x670 fs/ext4/inode.c:5978
>  ext4_inline_data_truncate+0x548/0xd00 fs/ext4/inline.c:2021
>  ext4_truncate+0x341/0xeb0 fs/ext4/inode.c:4221
>  ext4_process_orphan+0x1aa/0x2d0 fs/ext4/orphan.c:339
>  ext4_orphan_cleanup+0xb60/0x1340 fs/ext4/orphan.c:474
>  __ext4_fill_super fs/ext4/super.c:5515 [inline]
>  ext4_fill_super+0x80ed/0x8610 fs/ext4/super.c:5643
>  get_tree_bdev+0x400/0x620 fs/super.c:1324
>  vfs_get_tree+0x88/0x270 fs/super.c:1531
>  do_new_mount+0x289/0xad0 fs/namespace.c:3040
>  do_mount fs/namespace.c:3383 [inline]
>  __do_sys_mount fs/namespace.c:3591 [inline]
>  __se_sys_mount+0x2d3/0x3c0 fs/namespace.c:3568
>  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>  do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
>  entry_SYSCALL_64_after_hwframe+0x63/0xcd
>  </TASK>
> 
> Reason is allocate 16M memory by kmalloc, but MAX_ORDER is 11, kmalloc
> can allocate maxium size memory is 4M.
> XATTR_SIZE_MAX is currently 64k, but EXT4_XATTR_SIZE_MAX is '(1 << 24)',
> so 'ext4_xattr_check_entries()' regards this length as legal. Then trigger
> warning in 'ext4_xattr_move_to_block()'.
> To solve above issue, change EXT4_XATTR_SIZE_MAX from '(1 << 24)' to
> XATTR_SIZE_MAX. As VFS limit extended attribute maxium size to XATTR_SIZE_MAX.
> So we can assume that there will be no extended attribute with a length greater
> than XATTR_SIZE_MAX.

What is the value of (1 << 24)?

The prose reads a rather confused. kmalloc allocates 16M memory, but the
maximum memory size that kmalloc can allocate is 4M, right?

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 4/6] ext4: allocate extended attribute value in vmalloc area

[Jan Kara]

On Wed 07-12-22 15:40:41, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
> 
> Now, extended attribute value maxium length is 64K. The memory requested here
> does not need continuous physical addresses, so it is appropriate to use
> kvmalloc to request memory. At the same time, it can also cope with the
> situation that the extended attribute will become longer in the future.
> 
> Signed-off-by: Ye Bin <yebin10@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

Thanks!

								Honza

> ---
>  fs/ext4/xattr.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 75287422c36c..efa623658c12 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -2579,7 +2579,7 @@ static int ext4_xattr_move_to_block(handle_t *handle, struct inode *inode,
>  
>  	is = kzalloc(sizeof(struct ext4_xattr_ibody_find), GFP_NOFS);
>  	bs = kzalloc(sizeof(struct ext4_xattr_block_find), GFP_NOFS);
> -	buffer = kmalloc(value_size, GFP_NOFS);
> +	buffer = kvmalloc(value_size, GFP_NOFS);
>  	b_entry_name = kmalloc(entry->e_name_len + 1, GFP_NOFS);
>  	if (!is || !bs || !buffer || !b_entry_name) {
>  		error = -ENOMEM;
> @@ -2631,7 +2631,7 @@ static int ext4_xattr_move_to_block(handle_t *handle, struct inode *inode,
>  	error = 0;
>  out:
>  	kfree(b_entry_name);
> -	kfree(buffer);
> +	kvfree(buffer);
>  	if (is)
>  		brelse(is->iloc.bh);
>  	if (bs)
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 5/6] ext4: rename xattr_find_entry() and __xattr_check_inode()

[Jan Kara]

On Wed 07-12-22 15:40:42, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
> 
> xattr_find_entry() and __xattr_check_inode() is in EXT4 xattr module. so
> add 'ext4' prefix to unify name style.
> 
> Signed-off-by: Ye Bin <yebin10@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/xattr.c | 35 ++++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index efa623658c12..5c0476ff62c8 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -290,8 +290,9 @@ __ext4_xattr_check_block(struct inode *inode, struct buffer_head *bh,
>  
>  
>  static int
> -__xattr_check_inode(struct inode *inode, struct ext4_xattr_ibody_header *header,
> -			 void *end, const char *function, unsigned int line)
> +__ext4_xattr_check_inode(struct inode *inode,
> +			 struct ext4_xattr_ibody_header *header, void *end,
> +			 const char *function, unsigned int line)
>  {
>  	int error = -EFSCORRUPTED;
>  
> @@ -307,12 +308,12 @@ __xattr_check_inode(struct inode *inode, struct ext4_xattr_ibody_header *header,
>  	return error;
>  }
>  
> -#define xattr_check_inode(inode, header, end) \
> -	__xattr_check_inode((inode), (header), (end), __func__, __LINE__)
> +#define ext4_xattr_check_inode(inode, header, end) \
> +	__ext4_xattr_check_inode((inode), (header), (end), __func__, __LINE__)
>  
>  static int
> -xattr_find_entry(struct inode *inode, struct ext4_xattr_entry **pentry,
> -		 void *end, int name_index, const char *name, int sorted)
> +ext4_xattr_find_entry(struct inode *inode, struct ext4_xattr_entry **pentry,
> +		      void *end, int name_index, const char *name, int sorted)
>  {
>  	struct ext4_xattr_entry *entry, *next;
>  	size_t name_len;
> @@ -577,7 +578,7 @@ ext4_xattr_block_get(struct inode *inode, int name_index, const char *name,
>  	ext4_xattr_block_cache_insert(ea_block_cache, bh);
>  	entry = BFIRST(bh);
>  	end = bh->b_data + bh->b_size;
> -	error = xattr_find_entry(inode, &entry, end, name_index, name, 1);
> +	error = ext4_xattr_find_entry(inode, &entry, end, name_index, name, 1);
>  	if (error)
>  		goto cleanup;
>  	size = le32_to_cpu(entry->e_value_size);
> @@ -628,11 +629,11 @@ ext4_xattr_ibody_get(struct inode *inode, int name_index, const char *name,
>  	raw_inode = ext4_raw_inode(&iloc);
>  	header = IHDR(inode, raw_inode);
>  	end = (void *)raw_inode + EXT4_SB(inode->i_sb)->s_inode_size;
> -	error = xattr_check_inode(inode, header, end);
> +	error = ext4_xattr_check_inode(inode, header, end);
>  	if (error)
>  		goto cleanup;
>  	entry = IFIRST(header);
> -	error = xattr_find_entry(inode, &entry, end, name_index, name, 0);
> +	error = ext4_xattr_find_entry(inode, &entry, end, name_index, name, 0);
>  	if (error)
>  		goto cleanup;
>  	size = le32_to_cpu(entry->e_value_size);
> @@ -773,7 +774,7 @@ ext4_xattr_ibody_list(struct dentry *dentry, char *buffer, size_t buffer_size)
>  	raw_inode = ext4_raw_inode(&iloc);
>  	header = IHDR(inode, raw_inode);
>  	end = (void *)raw_inode + EXT4_SB(inode->i_sb)->s_inode_size;
> -	error = xattr_check_inode(inode, header, end);
> +	error = ext4_xattr_check_inode(inode, header, end);
>  	if (error)
>  		goto cleanup;
>  	error = ext4_xattr_list_entries(dentry, IFIRST(header),
> @@ -859,7 +860,7 @@ int ext4_get_inode_usage(struct inode *inode, qsize_t *usage)
>  		raw_inode = ext4_raw_inode(&iloc);
>  		header = IHDR(inode, raw_inode);
>  		end = (void *)raw_inode + EXT4_SB(inode->i_sb)->s_inode_size;
> -		ret = xattr_check_inode(inode, header, end);
> +		ret = ext4_xattr_check_inode(inode, header, end);
>  		if (ret)
>  			goto out;
>  
> @@ -1862,8 +1863,8 @@ ext4_xattr_block_find(struct inode *inode, struct ext4_xattr_info *i,
>  		bs->s.first = BFIRST(bs->bh);
>  		bs->s.end = bs->bh->b_data + bs->bh->b_size;
>  		bs->s.here = bs->s.first;
> -		error = xattr_find_entry(inode, &bs->s.here, bs->s.end,
> -					 i->name_index, i->name, 1);
> +		error = ext4_xattr_find_entry(inode, &bs->s.here, bs->s.end,
> +					      i->name_index, i->name, 1);
>  		if (error && error != -ENODATA)
>  			return error;
>  		bs->s.not_found = error;
> @@ -2222,12 +2223,12 @@ int ext4_xattr_ibody_find(struct inode *inode, struct ext4_xattr_info *i,
>  	is->s.here = is->s.first;
>  	is->s.end = (void *)raw_inode + EXT4_SB(inode->i_sb)->s_inode_size;
>  	if (ext4_test_inode_state(inode, EXT4_STATE_XATTR)) {
> -		error = xattr_check_inode(inode, header, is->s.end);
> +		error = ext4_xattr_check_inode(inode, header, is->s.end);
>  		if (error)
>  			return error;
>  		/* Find the named attribute. */
> -		error = xattr_find_entry(inode, &is->s.here, is->s.end,
> -					 i->name_index, i->name, 0);
> +		error = ext4_xattr_find_entry(inode, &is->s.here, is->s.end,
> +					      i->name_index, i->name, 0);
>  		if (error && error != -ENODATA)
>  			return error;
>  		is->s.not_found = error;
> @@ -2742,7 +2743,7 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
>  	min_offs = end - base;
>  	total_ino = sizeof(struct ext4_xattr_ibody_header) + sizeof(u32);
>  
> -	error = xattr_check_inode(inode, header, end);
> +	error = ext4_xattr_check_inode(inode, header, end);
>  	if (error)
>  		goto cleanup;
>  
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 6/6] ext4: fix inode leak in 'ext4_xattr_inode_create()'

[Jan Kara]

On Wed 07-12-22 15:40:43, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
> 
> There is issue as follows when do setxattr with inject fault:
> [localhost]#fsck.ext4  -fn  /dev/sda
> e2fsck 1.46.6-rc1 (12-Sep-2022)
> Pass 1: Checking inodes, blocks, and sizes
> Pass 2: Checking directory structure
> Pass 3: Checking directory connectivity
> Pass 4: Checking reference counts
> Unattached zero-length inode 15.  Clear? no
> 
> Unattached inode 15
> Connect to /lost+found? no
> 
> Pass 5: Checking group summary information
> 
> /dev/sda: ********** WARNING: Filesystem still has errors **********
> 
> /dev/sda: 15/655360 files (0.0% non-contiguous), 66755/2621440 blocks
> 
> Above issue occur in 'ext4_xattr_inode_create()', if 'ext4_mark_inode_dirty()'
> failed need to drop inode's i_nlink. Or will lead to inode leak.
> 
> Signed-off-by: Ye Bin <yebin10@huawei.com>

I think I've already given my Reviewed-by on this :). Anyway, the patch
looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/xattr.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 5c0476ff62c8..6c19d01ba261 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -1465,6 +1465,9 @@ static struct inode *ext4_xattr_inode_create(handle_t *handle,
>  		if (!err)
>  			err = ext4_inode_attach_jinode(ea_inode);
>  		if (err) {
> +			if (ext4_xattr_inode_dec_ref(handle, ea_inode))
> +				ext4_warning_inode(ea_inode,
> +					"cleanup dec ref error %d", err);
>  			iput(ea_inode);
>  			return ERR_PTR(err);
>  		}
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 2/6] ext4: add primary check extended attribute inode in ext4_xattr_check_entries()

[Jan Kara]

On Wed 07-12-22 15:40:39, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
> 
> Add primary check for extended attribute inode, only do hash check when read
> ea_inode's data in ext4_xattr_inode_get().
> 
> Signed-off-by: Ye Bin <yebin10@huawei.com>

...

> +static inline int ext4_xattr_check_extra_inode(struct inode *inode,
> +					       struct ext4_xattr_entry *entry)
> +{
> +	int err;
> +	struct inode *ea_inode;
> +
> +	err = ext4_xattr_inode_iget(inode, le32_to_cpu(entry->e_value_inum),
> +				    le32_to_cpu(entry->e_hash), &ea_inode);
> +	if (err)
> +		return err;
> +
> +	if (i_size_read(ea_inode) != le32_to_cpu(entry->e_value_size)) {
> +		ext4_warning_inode(ea_inode,
> +                           "ea_inode file size=%llu entry size=%u",
> +                           i_size_read(ea_inode),
> +			   le32_to_cpu(entry->e_value_size));
> +		err = -EFSCORRUPTED;
> +	}
> +	iput(ea_inode);
> +
> +	return err;
> +}
> +
>  static int
> -ext4_xattr_check_entries(struct ext4_xattr_entry *entry, void *end,
> -			 void *value_start)
> +ext4_xattr_check_entries(struct inode *inode, struct ext4_xattr_entry *entry,
> +			 void *end, void *value_start)
>  {
>  	struct ext4_xattr_entry *e = entry;
>  
> @@ -221,6 +247,10 @@ ext4_xattr_check_entries(struct ext4_xattr_entry *entry, void *end,
>  			    size > end - value ||
>  			    EXT4_XATTR_SIZE(size) > end - value)
>  				return -EFSCORRUPTED;
> +		} else if (entry->e_value_inum) {
> +			int err = ext4_xattr_check_extra_inode(inode, entry);
> +			if (err)
> +				return err;
>  		}
>  		entry = EXT4_XATTR_NEXT(entry);
>  	}

So I was thinking about this. It is nice to have the inode references
checked but OTOH this is rather expensive for a filesystem with EA inodes -
we have to lookup and possibly load EA inodes from the disk although they
won't be needed for anything else than the check. Also as you have noticed
we do check whether i_size and xattr size as recorded in xattr entry match
in ext4_xattr_inode_iget() which gets called once we need to do anything
with the EA inode.

Also I've checked and we do call ext4_xattr_check_block() and
xattr_check_inode() in ext4_expand_extra_isize_ea() so Ted's suspicion that
the problem comes from not checking the xattr entries before moving them
from the inode was not correct.

So to summarize, I don't think this and the following patch is actually
needed and brings benefit that would outweight the performance cost.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 2/6] ext4: add primary check extended attribute inode in ext4_xattr_check_entries()

[yebin (H)]

On 2022/12/7 19:14, Jan Kara wrote:
> On Wed 07-12-22 15:40:39, Ye Bin wrote:
>> From: Ye Bin <yebin10@huawei.com>
>>
>> Add primary check for extended attribute inode, only do hash check when read
>> ea_inode's data in ext4_xattr_inode_get().
>>
>> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ...
>
>> +static inline int ext4_xattr_check_extra_inode(struct inode *inode,
>> +					       struct ext4_xattr_entry *entry)
>> +{
>> +	int err;
>> +	struct inode *ea_inode;
>> +
>> +	err = ext4_xattr_inode_iget(inode, le32_to_cpu(entry->e_value_inum),
>> +				    le32_to_cpu(entry->e_hash), &ea_inode);
>> +	if (err)
>> +		return err;
>> +
>> +	if (i_size_read(ea_inode) != le32_to_cpu(entry->e_value_size)) {
>> +		ext4_warning_inode(ea_inode,
>> +                           "ea_inode file size=%llu entry size=%u",
>> +                           i_size_read(ea_inode),
>> +			   le32_to_cpu(entry->e_value_size));
>> +		err = -EFSCORRUPTED;
>> +	}
>> +	iput(ea_inode);
>> +
>> +	return err;
>> +}
>> +
>>   static int
>> -ext4_xattr_check_entries(struct ext4_xattr_entry *entry, void *end,
>> -			 void *value_start)
>> +ext4_xattr_check_entries(struct inode *inode, struct ext4_xattr_entry *entry,
>> +			 void *end, void *value_start)
>>   {
>>   	struct ext4_xattr_entry *e = entry;
>>   
>> @@ -221,6 +247,10 @@ ext4_xattr_check_entries(struct ext4_xattr_entry *entry, void *end,
>>   			    size > end - value ||
>>   			    EXT4_XATTR_SIZE(size) > end - value)
>>   				return -EFSCORRUPTED;
>> +		} else if (entry->e_value_inum) {
>> +			int err = ext4_xattr_check_extra_inode(inode, entry);
>> +			if (err)
>> +				return err;
>>   		}
>>   		entry = EXT4_XATTR_NEXT(entry);
>>   	}
> So I was thinking about this. It is nice to have the inode references
> checked but OTOH this is rather expensive for a filesystem with EA inodes -
> we have to lookup and possibly load EA inodes from the disk although they
> won't be needed for anything else than the check. Also as you have noticed
> we do check whether i_size and xattr size as recorded in xattr entry match
> in ext4_xattr_inode_iget() which gets called once we need to do anything
> with the EA inode.
>
> Also I've checked and we do call ext4_xattr_check_block() and
> xattr_check_inode() in ext4_expand_extra_isize_ea() so Ted's suspicion that
> the problem comes from not checking the xattr entries before moving them
> from the inode was not correct.
>
> So to summarize, I don't think this and the following patch is actually
> needed and brings benefit that would outweight the performance cost.
>
> 								Honza

Yes, I agree with you.
In ext4_ xattr_ check_ Entries () simply verifies the length of the 
extended attribute with
ea_inode. If the previous patch is not merged, EXT4_ XATTR_ SIZE_ MAX is 
much larger
than the actual constraint value. Data verification can only be 
postponed until the ea_inode
is read.
So your suggestion is to modify EXT4_ XATTR_ SIZE_ MAX Or defer data 
verification until
the ea_inode is read?

Re: [PATCH v2 2/6] ext4: add primary check extended attribute inode in ext4_xattr_check_entries()

[Jan Kara]

On Wed 07-12-22 19:39:54, yebin (H) wrote:
> 
> 
> On 2022/12/7 19:14, Jan Kara wrote:
> > On Wed 07-12-22 15:40:39, Ye Bin wrote:
> > > From: Ye Bin <yebin10@huawei.com>
> > > 
> > > Add primary check for extended attribute inode, only do hash check when read
> > > ea_inode's data in ext4_xattr_inode_get().
> > > 
> > > Signed-off-by: Ye Bin <yebin10@huawei.com>
> > ...
> > 
> > > +static inline int ext4_xattr_check_extra_inode(struct inode *inode,
> > > +					       struct ext4_xattr_entry *entry)
> > > +{
> > > +	int err;
> > > +	struct inode *ea_inode;
> > > +
> > > +	err = ext4_xattr_inode_iget(inode, le32_to_cpu(entry->e_value_inum),
> > > +				    le32_to_cpu(entry->e_hash), &ea_inode);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	if (i_size_read(ea_inode) != le32_to_cpu(entry->e_value_size)) {
> > > +		ext4_warning_inode(ea_inode,
> > > +                           "ea_inode file size=%llu entry size=%u",
> > > +                           i_size_read(ea_inode),
> > > +			   le32_to_cpu(entry->e_value_size));
> > > +		err = -EFSCORRUPTED;
> > > +	}
> > > +	iput(ea_inode);
> > > +
> > > +	return err;
> > > +}
> > > +
> > >   static int
> > > -ext4_xattr_check_entries(struct ext4_xattr_entry *entry, void *end,
> > > -			 void *value_start)
> > > +ext4_xattr_check_entries(struct inode *inode, struct ext4_xattr_entry *entry,
> > > +			 void *end, void *value_start)
> > >   {
> > >   	struct ext4_xattr_entry *e = entry;
> > > @@ -221,6 +247,10 @@ ext4_xattr_check_entries(struct ext4_xattr_entry *entry, void *end,
> > >   			    size > end - value ||
> > >   			    EXT4_XATTR_SIZE(size) > end - value)
> > >   				return -EFSCORRUPTED;
> > > +		} else if (entry->e_value_inum) {
> > > +			int err = ext4_xattr_check_extra_inode(inode, entry);
> > > +			if (err)
> > > +				return err;
> > >   		}
> > >   		entry = EXT4_XATTR_NEXT(entry);
> > >   	}
> > So I was thinking about this. It is nice to have the inode references
> > checked but OTOH this is rather expensive for a filesystem with EA inodes -
> > we have to lookup and possibly load EA inodes from the disk although they
> > won't be needed for anything else than the check. Also as you have noticed
> > we do check whether i_size and xattr size as recorded in xattr entry match
> > in ext4_xattr_inode_iget() which gets called once we need to do anything
> > with the EA inode.
> > 
> > Also I've checked and we do call ext4_xattr_check_block() and
> > xattr_check_inode() in ext4_expand_extra_isize_ea() so Ted's suspicion that
> > the problem comes from not checking the xattr entries before moving them
> > from the inode was not correct.
> > 
> > So to summarize, I don't think this and the following patch is actually
> > needed and brings benefit that would outweight the performance cost.
> > 
> > 								Honza
> 
> Yes, I agree with you.
> In ext4_ xattr_ check_ Entries () simply verifies the length of the extended
> attribute with
> ea_inode. If the previous patch is not merged, EXT4_ XATTR_ SIZE_ MAX is
> much larger
> than the actual constraint value. Data verification can only be postponed
> until the ea_inode
> is read.
>
> So your suggestion is to modify EXT4_ XATTR_ SIZE_ MAX Or defer data
> verification until the ea_inode is read?

My suggestion would be to take patches 1,4,5,6 from your series. So reduce
EXT4_XATTR_SIZE_MAX (if Ted agrees), use kvmalloc() instead of kmalloc(),
do the cleanup of funtion names, and fix the inode refcount leak.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR