Re: [PATCH 04/27] ext4: add KUnit tests for fs/ext4/xattr.c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] ext4: uninitialized e_name in test_xattr_check_inode()
  bypasses value checks
- [Low] ext4: stack frame size warning in
  test_xattr_credits_usage_expand_and_delete()
- [Low] ext4: missing i_size_ordered_init() in init_mock_inode_slot()
--

commit b81033fe08ff323ec6a5c24275644912bdfe1108
Author: Theodore Ts'o <tytso@mit.edu>

ext4: add KUnit tests for fs/ext4/xattr.c

This commit adds a KUnit test suite in fs/ext4/xattr-test.c to exercise
extended attribute operations in fs/ext4/xattr.c using static stubs and
exports.

> diff --git a/fs/ext4/xattr-test.c b/fs/ext4/xattr-test.c
> new file mode 100644
> index 0000000000000..35be150353a20
> --- /dev/null
> +++ b/fs/ext4/xattr-test.c

[ ... ]

> +static void init_mock_inode_slot(struct xattr_test_ctx *ctx,
> +				 struct mock_inode_slot *slot,
> +				 unsigned long ino)
> +{
> +	struct inode *inode = &slot->ei.vfs_inode;
> +	struct ext4_inode *raw_inode;
> +
> +	memset(slot, 0, sizeof(*slot));
> +	slot->allocated = true;
> +
> +	inode->i_sb = &ctx->sb;
> +	inode->i_ino = ino;
> +	inode->i_mode = S_IFREG | 0644;
> +	inode->i_blkbits = TEST_BLK_BITS;
> +	inode->i_mapping = &inode->i_data;
> +	set_nlink(inode, 1);
> +	atomic_set(&inode->i_count, 1);
> +
> +	spin_lock_init(&inode->i_lock);
> +	init_rwsem(&inode->i_rwsem);

[Severity: Low]
Should init_mock_inode_slot() also call i_size_ordered_init(inode)?

On 32-bit SMP builds with CONFIG_DEBUG_LOCK_ALLOC=y, struct inode embeds
i_size_seqcount, so calling i_size_write() or i_size_read() on the mock
EA inodes triggers a lockdep warning for an uninitialized key and
disables lockdep.

[ ... ]

> +static void test_xattr_check_inode(struct kunit *test)
> +{
> +	const struct xattr_check_param *p = test->param_value;
> +	struct xattr_test_ctx *ctx = test->priv;
> +	struct inode *inode = &ctx->primary.ei.vfs_inode;
> +	struct ext4_inode *raw_inode = (struct ext4_inode *)ctx->primary.raw_buf;
> +	struct ext4_xattr_ibody_header *hdr = IHDR(inode, raw_inode);
> +	struct ext4_xattr_entry *e1, *e2;
> +	void *end = ITAIL(inode, raw_inode);
> +	size_t total_size = (char *)end - (char *)IFIRST(hdr);
> +	int ret;
> +
> +	memset(hdr, 0, (char *)end - (char *)hdr);
> +	hdr->h_magic = cpu_to_le32(EXT4_XATTR_MAGIC);
> +	e1 = IFIRST(hdr);
> +
> +	switch (p->id) {

[ ... ]

> +	case CHECK_NONZERO_OFFS_WITH_INUM:
> +		e1->e_name_len = 1;
> +		e1->e_value_inum = cpu_to_le32(20);
> +		e1->e_value_offs = cpu_to_le16(total_size - 4);
> +		break;
> +	case CHECK_VALUE_SIZE_GT_TOTAL:
> +		e1->e_name_len = 1;
> +		e1->e_value_size = cpu_to_le32(total_size + 4);
> +		e1->e_value_offs = cpu_to_le16(total_size - 4);
> +		break;
> +	case CHECK_VALUE_OFFS_OVERLAPS_ENTRY:
> +		e1->e_name_len = 1;
> +		e1->e_value_size = cpu_to_le32(4);
> +		/* Offset points inside e1 + 4-byte terminator */
> +		e1->e_value_offs = cpu_to_le16(EXT4_XATTR_LEN(1));
> +		break;
> +	case CHECK_VALUE_END_EXCEEDS_TOTAL:
> +		e1->e_name_len = 1;
> +		e1->e_value_size = cpu_to_le32(8);
> +		e1->e_value_offs = cpu_to_le16(total_size - 4);
> +		break;
> +	}

[Severity: Medium]
Do these four cases reach the value checks in check_xattrs()?

Because hdr is zeroed with memset() and e1->e_name[0] is left as NUL
while e1->e_name_len is 1, check_xattrs() rejects all four cases in the
entry-name loop before reaching the value-validation loop:

fs/ext4/xattr.c:check_xattrs() {
    ...
	while (!IS_LAST_ENTRY(e)) {
		...
		if (strnlen(e->e_name, e->e_name_len) != e->e_name_len) {
			err_str = "bad e_name length";
			goto errout;
		}
		e = next;
	}

	/* Check the values */
	while (!IS_LAST_ENTRY(entry)) {
    ...
}

In addition, check_xattrs() only inspects e_value_offs when
entry->e_value_inum == 0, so CHECK_NONZERO_OFFS_WITH_INUM does not test a
check that check_xattrs() performs; even with e1->e_name[0] set, it would
fail on the missing EXT4_FEATURE_INCOMPAT_EA_INODE feature or zero
e_value_size instead.

[ ... ]

> +static void test_xattr_credits_usage_expand_and_delete(struct kunit *test)
> +{
> +	struct xattr_test_ctx *ctx = test->priv;
> +	struct inode *inode = &ctx->primary.ei.vfs_inode;
> +	struct ext4_inode *raw = (struct ext4_inode *)ctx->primary.raw_buf;
> +	journal_t fake_journal = {
> +		.j_revoke_records_per_block = 64,
> +	};

[Severity: Low]
Can placing fake_journal on the stack exceed CONFIG_FRAME_WARN?

journal_t (struct journal_s) is ~1.3 KB on 64-bit builds and over 2.2 KB
when lock debugging is enabled, which triggers -Wframe-larger-than=
warnings on 32-bit builds or 64-bit debug configurations.  Moving
fake_journal into struct xattr_test_ctx or allocating it with
kunit_kzalloc() would avoid the large stack frame.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112040638.1576836-1-tytso@mit.edu?part=4