Re: [PATCH v16 05/15] iov_iter: Add a bvecq kunit test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] iov_iter: kunit_bvecq marks test functions __init with
  kunit_test_suites()
- [Low] iov_iter: folio leak and unchecked action registration in
  kunit_bvecq
- [Low] iov_iter: bvecq_kunit_advance() bounds revert steps by left
  instead of pos
--

commit 078e4c29ad244b5b9e915f77ff2e3db4692905e6
Author: David Howells <dhowells@redhat.com>

iov_iter: Add a bvecq kunit test

This commit adds a KUnit test suite for the bvecq iterator covering advance,
revert, copy-out, iov_iter_npages(), and iov_iter_alignment() across various
bvecq chain recipes and partial folio configurations.

> diff --git a/lib/tests/kunit_bvecq.c b/lib/tests/kunit_bvecq.c
> new file mode 100644
> index 0000000000000..7cdd455df6929
> --- /dev/null
> +++ b/lib/tests/kunit_bvecq.c
> @@ -0,0 +1,599 @@

[ ... ]

> +static void __init iov_bvecq_alloc_folios(struct kunit *test)
> +{
> +	struct folio *folio, **folios = bvecq_kunit_folios;
> +	const int nr = ARRAY_SIZE(bvecq_kunit_folios);
> +
> +	bvecq_kunit_pagemap_nr = 0;
> +	memset(bvecq_kunit_pagemap, 0, sizeof(bvecq_kunit_pagemap));
> +	memset(bvecq_kunit_folios, 0, sizeof(bvecq_kunit_folios));
> +
> +	for (int i = 0; i < nr; i++) {
> +		folio = folio_alloc(GFP_KERNEL, bvecq_kunit_orders[i]);
> +		KUNIT_ASSERT_NOT_NULL(test, folio);
> +		folios[i] = folio;
> +
> +		/* Fill each subpage with a different but predictable pattern. */
> +		for (int j = 0; j < folio_nr_pages(folio); j++) {
> +			u8 *p = kmap_local_folio(folio, PAGE_SIZE * j);
> +
> +			for (int k = 0; k < PAGE_SIZE; k++)
> +				p[k] = bvecq_kunit_pattern(i, j, k);
> +			kunmap_local(p);
> +		}
> +	}
> +
> +	kunit_add_action_or_reset(test, bvecq_kunit_release_folios, folios);
> +}
> +
> +/*
> + * Follow the specified recipe to allocate a bvecq chain and stick the folios
> + * and empty gaps into it.  Further, the offset into and length of each folio
> + * is adjusted from the partials list.  Each ALLOC command will append a new
> + * bvecq to the chain and move to filling that.
> + */
> +static struct bvecq *__init bvecq_kunit_load_chain(
> +	struct kunit *test,
> +	const struct bvecq_test_recipe *rec,
> +	const struct bvecq_test_partials *parts,
> +	size_t *size)
> +{
> +	struct bvecq *head = NULL, *bq = NULL, *p;
> +	size_t pos = 0;
> +	int folio_index = 0;
> +
> +	*size = 0;
> +
> +	for (; rec->cmd != BVECQ_KUNIT_STOP; rec++) {
> +		if (!head)
> +			KUNIT_ASSERT_EQ(test, rec->cmd, BVECQ_KUNIT_ALLOC);
> +
> +		switch (rec->cmd) {
> +		case BVECQ_KUNIT_STOP:
> +			break;
> +
> +		case BVECQ_KUNIT_ALLOC:
> +			p = bvecq_alloc_one(rec->count, GFP_KERNEL, false);
> +			KUNIT_ASSERT_NOT_NULL(test, p);
> +
> +			if (!head) {
> +				head = p;
> +				kunit_add_action_or_reset(test, bvecq_kunit_bvecq_put, head);
> +				bq = p;
> +			} else {

[Severity: Low]
Does iov_bvecq_alloc_folios() leak already-allocated folios if folio_alloc()
fails after the first iteration?  KUNIT_ASSERT_NOT_NULL() aborts the test
before kunit_add_action_or_reset() is reached, and
bvecq_kunit_release_folios() passes the full array size to release_pages(),
which cannot handle NULL entries in a partially populated array.

Also, neither iov_bvecq_alloc_folios() nor bvecq_kunit_load_chain() checks
the return value of kunit_add_action_or_reset().  If action allocation fails,
the cleanup callback runs immediately to free the folios or head bvecq while
the test continues, causing a NULL pointer dereference or use-after-free.

[ ... ]

> +		/* 3 steps forward. */
> +		part = min(left, 3);
> +		iov_iter_advance(&iter, 3); /* May be short. */
> +		pos += part;
> +		left -= part;
> +		bvecq_kunit_check_next(test, &iter, &pos, &left, 3);
> +
> +		/* 2 steps back */
> +		part = umin(2, left);
> +		iov_iter_revert(&iter, part);
> +		pos -= part;
> +		left += part;
> +		bvecq_kunit_check_next(test, &iter, &pos, &left, 2);
> +
> +		/* 1 more step back */
> +		part = umin(1, left);
> +		iov_iter_revert(&iter, part);
> +		pos -= part;
> +		left += part;
> +		bvecq_kunit_check_next(test, &iter, &pos, &left, 1);

[Severity: Low]
Should these two revert steps clamp against pos (or size - left) rather than
left?  left is the unread byte count remaining ahead, so when the forward
advance and check reach the end of the iterator (left == 0), both reverts
become 0-byte no-ops instead of rewinding.

[ ... ]

> +#define TEST(CHECK, PARTIALS, RECIPE) \
> +static void __init bvecq_kunit_##CHECK##_##PARTIALS##_##RECIPE(struct kunit *test) \
> +{ \
> +	bvecq_kunit_##CHECK(test, \
> +			    bvecq_test_recipe_##RECIPE,	      \
> +			    bvecq_test_partials_##PARTIALS);  \
> +}
> +
> +/* Emit test stub functions. */
> +ADVANCE_TESTS(advance);
> +ADVANCE_TESTS(jump);
> +#undef TEST
> +
> +#define TEST(CHECK, PARTIALS, RECIPE)				\
> +	KUNIT_CASE(bvecq_kunit_##CHECK##_##PARTIALS##_##RECIPE),
> +
> +static struct kunit_case __refdata bvecq_kunit_cases[] = {
> +	ADVANCE_TESTS(advance)
> +	ADVANCE_TESTS(jump)
> +	{}
> +};
> +
> +static struct kunit_suite bvecq_kunit_suite = {
> +	.name = "bvecq",
> +	.test_cases = bvecq_kunit_cases,
> +};
> +
> +kunit_test_suites(&bvecq_kunit_suite);

[Severity: Medium]
Can triggering this suite via /sys/kernel/debug/kunit/bvecq/run after boot or
module load jump into freed .init.text memory?

All test functions and helpers in this file are marked __init, while
kunit_test_suites() registers bvecq_kunit_suite with is_init = false, which
causes kunit_debugfs_create_suite() to expose the debugfs run file.  Should
this either drop __init and __refdata or register the suite with
kunit_test_init_section_suites()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107160823.1465553-1-dhowells@redhat.com?part=5