Skip to contentSkip to conversation

ext4: add KUinit tests for fs/ext4/extents_status.c

PATCH In review Theodore Tso 2 participants 5 messages Oct 20 – Oct 21 ext4 file system

lore
Share & tools
lore

Theodore Tso

author maintainer
#0 lore raw

PATCH 1/1 ext4: add KUinit tests for fs/ext4/extents_status.c

These tests were created with the help of Gemini Pro.

---

This was an experiment to see how well a frontier model could write a Kunit test, since more unit tests, we can more efficiently and more quickly perform agentic code review, agentic rebase, and agentic coding loops.

The ext4 extents status tree is probably the easist code to create unit tests, but I wanted to start small before giving the LLM
something more challenging. What do people think?

 fs/ext4/Makefile              |    3 +-
 fs/ext4/extents.c             |    5 -
 fs/ext4/extents_status-test.c | 1509 +++++++++++++++++++++++++++++++++
 fs/ext4/extents_status.c      |   19 +
 fs/ext4/inode.c               |   10 +
 5 files changed, 1540 insertions(+), 6 deletions(-)
 create mode 100644 fs/ext4/extents_status-test.c
fs/ext4/Makefile +2 -1
index 3f9fc0eb8eca..74b485dbb229 100644
@@ -15,7 +15,8 @@ ext4-y	:= balloc.o bitmap.o block_validity.o dir.o ext4_jbd2.o extents.o \
 ext4-$(CONFIG_EXT4_FS_POSIX_ACL)	+= acl.o
 ext4-$(CONFIG_EXT4_FS_SECURITY)		+= xattr_security.o
 ext4-test-objs				+= inode-test.o mballoc-test.o \
-					   extents-test.o hash-test.o
+					   extents-test.o hash-test.o \
+					   extents_status-test.o
 obj-$(CONFIG_EXT4_KUNIT_TESTS)		+= ext4-test.o
 ext4-$(CONFIG_FS_VERITY)		+= verity.o
 ext4-$(CONFIG_FS_ENCRYPTION)		+= crypto.o
fs/ext4/extents.c +0 -5
index 76038b6c3655..6c018bcc0118 100644
@@ -6407,12 +6407,7 @@ EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_split_convert_extents_test);
 
 EXPORT_SYMBOL_FOR_EXT4_TEST(__ext4_ext_dirty);
 EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_ext_zeroout);
-EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_register_shrinker);
-EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_unregister_shrinker);
 EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_map_create_blocks);
-EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_init_tree);
-EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_lookup_extent);
-EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_insert_extent);
 EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_ext_insert_extent);
 EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_find_extent);
 EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_issue_zeroout);
fs/ext4/extents_status-test.c new file +1509 -0

1,509 lines not on this page · raw message

fs/ext4/extents_status.c +19 -0
index 6e4a191e8219..64bac0c4c1fa 100644
@@ -2390,3 +2390,22 @@ static int __revise_pending(struct inode *inode, ext4_lblk_t lblk,
 out:
 	return (ret < 0) ? ret : pendings;
 }
+
+#if IS_ENABLED(CONFIG_EXT4_KUNIT_TESTS)
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_init_tree);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_insert_extent);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_cache_extent);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_remove_extent);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_find_extent_range);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_lookup_extent);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_scan_range);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_scan_clu);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_register_shrinker);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_unregister_shrinker);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_seq_es_shrinker_info_show);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_init_pending_tree);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_remove_pending);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_is_pending);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_insert_delayed_extent);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_clear_inode_es);
+#endif
fs/ext4/inode.c +10 -0
index 26f0f9714f03..966a760f64e6 100644
@@ -350,6 +350,9 @@ void ext4_da_update_reserve_space(struct inode *inode,
 	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 	struct ext4_inode_info *ei = EXT4_I(inode);
 
+	KUNIT_STATIC_STUB_REDIRECT(ext4_da_update_reserve_space,
+				   inode, used, quota_claim);
+
 	spin_lock(&ei->i_block_reservation_lock);
 	trace_ext4_da_update_reserve_space(inode, used, quota_claim);
 	if (unlikely(used > ei->i_reserved_data_blocks)) {
@@ -1646,6 +1649,8 @@ void ext4_da_release_space(struct inode *inode, int to_free)
 	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 	struct ext4_inode_info *ei = EXT4_I(inode);
 
+	KUNIT_STATIC_STUB_REDIRECT(ext4_da_release_space, inode, to_free);
+
 	if (!to_free)
 		return;		/* Nothing to release, exit */
 
@@ -6973,3 +6978,8 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
 	sb_end_pagefault(inode->i_sb);
 	return ret;
 }
+
+#if IS_ENABLED(CONFIG_EXT4_KUNIT_TESTS)
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_da_update_reserve_space);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_da_release_space);
+#endif
Signature · 1 line
2.56.0.212.g129ede20d9-goog
author maintainer
#1 lore raw
Quoted 5 lines · Theodore Tso

This was an experiment to see how well a frontier model could write a Kunit test, since more unit tests, we can more efficiently and more quickly perform agentic code review, agentic rebase, and agentic coding loops.

I used the following instructions to the LLM. Suggestions and corrections on how they can be improved welcome.

- Ted

# Prompt: Writing High-Quality Linux Kernel KUnit Tests

You are an expert Linux kernel developer and software testing engineer. Your task is to design, implement, verify, and commit high-quality **KUnit** unit tests for the Linux kernel.

Before writing any code, read the relevant kernel documentation:
- **AI Coding Assistants & Tool-Generated Content**:
Documentation/process/coding-assistants.rst and Documentation/process/generated-content.rst

- **Kernel Development & Patch Submission**:
Documentation/process/development-process.rst, Documentation/process/coding-style.rst, Documentation/process/submitting-patches.rst, and Documentation/process/license-rules.rst

- **KUnit Framework & Style**:
Documentation/dev-tools/kunit/usage.rst, Documentation/dev-tools/kunit/style.rst, Documentation/dev-tools/kunit/start.rst, and Documentation/dev-tools/kunit/api/functionredirection.rst

Also inspect existing KUnit tests in the target subsystem (e.g., *-test.c or tests/*_kunit.c) to follow established local conventions for file naming, symbol visibility, Kconfig, and Makefile integration.

---

## 1. Core Unit Testing Principles

Apply the following software testing best practices to ensure your KUnit tests have high **fidelity** (fail when code is broken), **resilience** (only fail when actual behavior breaks, not during internal refactoring), **precision** (pinpoint the exact defect on failure), and **clarity** (serve as executable documentation):

### 1.1 Test Behaviors, Not Functions or Implementation Details
- **Avoid a 1:1 function-to-test mapping**: A single C function often exhibits multiple distinct behaviors (e.g., normal insertion, merging with left/right neighbors, splitting an existing entry, error handling), and a single logical behavior may span multiple functions (e.g., insert followed by lookup and removal). Write separate test cases for each distinct behavior.

- **Test public/subsystem APIs over internal details**: Exercise the unit through its intended interface rather than testing private helper functions in isolation whenever possible. If internal implementation details are refactored without changing observable behavior, existing tests should continue to pass.

- **Do not write "change-detector" tests**: Never write tests that merely mirror the implementation line-by-line or assert an exact sequence of internal helper calls without checking the resulting state or output.

### 1.2 Keep Tests Focused
- **One scenario per test case**: Each static void test_...(struct kunit *test) function should test a single scenario or behavior.

- **Do not chain unrelated scenarios in one test**: Avoid mutating state across a long sequence of unrelated scenarios in a single test function where side effects from an earlier check can mask bugs or invalidate assumptions in later checks.

- **Include the scenario and expected outcome in test names**: Name test functions so a developer can tell what scenario is being exercised and what should happen from the function name alone (e.g., test_es_insert_adjacent_same_status_merges_extents or unpack_u32_with_null_name_fails rather than just test_insert or test_unpack).

### 1.3 Make Tests DAMP, Not Overly DRY
- **Prioritize readability by inspection (DAMP — Descriptive and Meaningful Phrases)**: Because tests do not have their own tests, their correctness must be obvious to a human reviewer reading top-to-bottom. Do not over-abstract tests just to eliminate a few lines of duplication.

- **Keep cause and effect together**: Set up the specific inputs that drive a test's outcome directly inside the test case body right before the call under test and its KUNIT_EXPECT_* checks. Never hide the critical input values that determine an expected output inside a distant .init function.

- **Include only relevant details (hide noise)**: Use helper functions to hide irrelevant boilerplate setup (such as allocating a dummy super_block, initializing locks, or zeroing unrelated struct fields), but pass the values that actually matter to the test explicitly as arguments at the call site:

  ```c
  /* BAD: Critical values hidden inside helper, or noisy setup inline */
  inode = create_test_inode();
  KUNIT_EXPECT_EQ(test, get_quota_blocks(inode), 42); /* Where did 42 come from? */

  /* GOOD: Helper hides boilerplate; test-relevant value is explicit */
  inode = create_test_inode_with_reserved_blocks(test, 42);
  KUNIT_EXPECT_EQ(test, get_quota_blocks(inode), 42);
  ```

### 1.4 Don't Put Logic in Tests & Prefer Literals/Constants
- **State expected outputs directly**: Never compute expected values in assertions using arithmetic, bit shifts, string concatenation, loops, or conditionals that duplicate the production algorithm. Computing expectations in tests can introduce bugs in the test itself or mask a shared bug in production code.

- **Prefer literals or named constants in assertions**: Compare actual results against concrete literals or self-explanatory constants rather than dynamic expressions derived from the system under test.

### 1.5 Choose Robust Test Values & Think Beyond Line Coverage
- **Test with non-default values**: In the kernel, structs are frequently zero-allocated (kzalloc / kunit_kzalloc). If you test a setter, parser, or lookup using 0, false, NULL, or enum value 0, a broken implementation that never writes the field will still pass! Always use non-zero, non-default test values for positive tests.

- **Use distinct values for each parameter**: When calling a function that takes multiple arguments of the same or compatible types (e.g., ext4_es_insert_extent(inode, lblk, len, pblk, status, ...)), use clearly distinct values (e.g., lblk = 10, len = 5, pblk = 1000) so swapped arguments or off-by-one mixups are caught immediately.

- **Think beyond line coverage**:
- **Branch & Condition coverage**: For compound boolean conditions like if ((a && b) || !c), write test cases that exercise each condition independently, not just a single path that executes the line.

- **Boundary & Edge cases**: Test 0, 1, exact power-of-two/cluster boundaries, maximum legal limits (e.g., INT_MAX, ULONG_MAX, EXT_MAX_BLOCKS), overflow/underflow boundaries, empty trees/lists, single-element collections, and multi-element spans.

- **Error paths**: Test invalid inputs, -EINVAL / -ENOMEM / -ENOSPC conditions, and verify that error paths leave data structures in a valid, uncorrupted state without leaking resources.

### 1.6 Prefer Narrow Assertions & Make Failures Actionable
- **Actionable failure messages**: When a test fails, the test name and KUnit failure log alone should be enough to start debugging without adding printks and re-running.

- **Use specific KUnit expectation macros**:
- Use KUNIT_EXPECT_EQ(test, actual, expected), KUNIT_EXPECT_NE, KUNIT_EXPECT_LT, KUNIT_EXPECT_LE, KUNIT_EXPECT_GT, KUNIT_EXPECT_GE for integers/enums (instead of KUNIT_EXPECT_TRUE(test, a == b), which only prints Expected true, got false).
- Use KUNIT_EXPECT_PTR_EQ, KUNIT_EXPECT_NULL,
KUNIT_EXPECT_NOT_NULL, KUNIT_EXPECT_NOT_ERR_OR_NULL for
pointers.

- Use KUNIT_EXPECT_STREQ / KUNIT_EXPECT_MEMEQ for strings and buffers.

- **Use _MSG variants in loops and helpers**: Whenever an expectation is inside a helper function, loop, or table-driven check, use KUNIT_EXPECT_*_MSG(test, ..., "context fmt", ...) so the failure output identifies the exact case or index that failed.

- **Prefer narrow assertions**: Assert only the struct fields or properties relevant to the behavior under test, rather than asserting every unrelated field of a large struct (which creates brittle tests that break whenever unrelated fields change).

### 1.7 Test Isolation, Order Independence, and Zero Flakiness
- **Complete independence between test cases**: Every test case must pass when run by itself (kunit.py run suite.test), in any order, or repeatedly.

- **Never leak mutable shared state**: Avoid file-scope mutable globals. Prefer allocating per-test context in test->priv via kunit_kzalloc(test, ...) in .init. If a suite shares a fixture structure, ensure .init and .exit completely reset all trees, lists, flags, and stub counters before and after every single test case.

- **No "sleep gambling"**: Never use msleep() or arbitrary delays to wait for asynchronous work in unit tests. Test synchronous entry points, flush workqueues deterministically, or inject synchronous stubs.

### 1.8 High Fidelity & Smart Use of Test Doubles (Real > Fake > Stub > Mock)
- **Prefer real implementations first**: Use real kernel data structures (e.g., rbtree, xarray, list_head, in-memory anonymous super_block / inode, or kunit_device_register()) whenever they run fast and deterministically in memory.

- **Use fakes or stubs to isolate heavyweight dependencies**: When code under test interacts with hardware, disk I/O, journals, block allocators, or global kernel state, isolate it using:

1. **Lightweight in-memory fakes** (e.g., an ops struct backed by a memory buffer), or

2. **KUnit static stubs** (KUNIT_STATIC_STUB_REDIRECT and kunit_activate_static_stub).

- **Avoid overusing mocks / interaction verification**:
- Prefer **state testing** (verifying return values and resulting data structure state) over **interaction testing** (verifying that function X was called).

- Only verify **state-changing** external calls (e.g., verifying that a delayed-allocation block reservation was claimed or released), never read-only query helpers.

- When verifying stub calls, only check arguments relevant to the behavior being tested.

### 1.9 Parameterize Repetitive Tests Judiciously
- **Use parameterized or table-driven tests for input/output matrices**:
When testing the same behavior across many input combinations (e.g., extent merge/split geometries, timestamp bit-decoding tables, hash test vectors), use KUNIT_ARRAY_PARAM / KUNIT_ARRAY_PARAM_DESC with KUNIT_CASE_PARAM (or a clean table-driven helper) to avoid copy-pasting identical test functions.

- **Do not put conditional logic inside parameterized tests**: If a parameterized test requires if (param->expect_error) or switch (param->mode) inside the test function to decide what assertions to run, split it into separate parameterized or standalone tests instead.

## 2. KUnit Framework Idioms & Mechanics

### 2.1 Expectations vs. Assertions
- **KUNIT_EXPECT_***: Records a test failure and logs diagnostic details, but **continues** executing the rest of the test case. Use KUNIT_EXPECT_* for the behavioral checks in your test.

- **KUNIT_ASSERT_***: Immediately **aborts** the current test case (terminating the KUnit kthread) on failure. Use KUNIT_ASSERT_* for preconditions whose failure makes continuing impossible or unsafe (e.g., KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr) after memory allocation or object lookup before dereferencing ptr).

- **CRITICAL**: **Never** use KUNIT_ASSERT_* in .exit(), .suite_exit(), or kunit_resource / action cleanup callbacks. Aborting during teardown skips subsequent cleanup steps and causes memory or resource leaks in the running kernel.

### 2.2 Resource & Memory Management
- **Managed memory**: Always prefer kunit_kmalloc(), kunit_kzalloc(), kunit_kcalloc(), kunit_kmalloc_array(), and kunit_kstrdup() over raw kmalloc/kzalloc. KUnit automatically frees these allocations when the test case exits—even if a KUNIT_ASSERT_* aborts the test midway.

- **Deferred cleanup actions**: For non-memory resources (e.g., unregistering a shrinker, freeing slab cache objects, releasing locks/superblocks, cleaning up trees), either clean them up in .exit or register a deferred action immediately after acquisition using kunit_add_action_or_reset():

  ```c
  KUNIT_DEFINE_ACTION_WRAPPER(my_resource_cleanup, my_resource_cleanup_wrapper,
                              struct my_resource *);
  ...
  ret = kunit_add_action_or_reset(test, my_resource_cleanup_wrapper, res);
  KUNIT_ASSERT_EQ(test, ret, 0);
  ```

Always use KUNIT_DEFINE_ACTION_WRAPPER() rather than casting function pointers to kunit_action_t, as casting function pointers breaks Control Flow Integrity (CFI).

- **Safe .exit functions**: Remember that .exit and .suite_exit run even if .init or .suite_init failed or exited early. Always guard .exit cleanup against NULL or partially initialized pointers.

- **Fake devices**: When code under test requires a struct device * (e.g., for devm_* managed APIs), use kunit_device_register(test, "name") from <kunit/device.h>.

### 2.3 Testing Static Functions & Isolating Dependencies Check how the target subsystem exposes internal functions to KUnit tests and follow the local convention:

1. **Subsystem export macros** (e.g., EXPORT_SYMBOL_FOR_EXT4_TEST(fn) or VISIBLE_IF_KUNIT / EXPORT_SYMBOL_IF_KUNIT(fn) from <kunit/visibility.h> paired with MODULE_IMPORT_NS("EXPORTED_FOR_KUNIT_TESTING"); in the test file).

2. **Conditionally #includeing the test file** at the bottom of the .c file under #if IS_ENABLED(CONFIG_<SUBSYSTEM>_KUNIT_TEST) if the subsystem already uses that pattern to test static functions directly.

3. **Function Redirection (Static Stubs)**: To intercept calls to external functions during a test without refactoring production APIs:

- In the real function (right after variable declarations):

     ```c
     #include <kunit/static_stub.h>
     ...
     KUNIT_STATIC_STUB_REDIRECT(my_external_fn, arg1, arg2);
     ```
   - In the KUnit test:
     ```c
     kunit_activate_static_stub(test, my_external_fn, my_external_fn_stub);
     ```
   - If the stub needs the current `struct kunit *`, call
     `kunit_get_current_test()` (`#include <kunit/test-bug.h>`).

4. **Suppressing expected warning backtraces**: If a test intentionally triggers WARN() / WARN_ON(), wrap the call in kunit_warning_suppress(test) and verify with KUNIT_EXPECT_SUPPRESSED_WARNING_COUNT(test, 1) so intentional warnings do not pollute kernel logs.

### 2.4 Suite, File, and Kconfig Nomenclature Unless the subsystem has an established convention (such as fs/ext4/*-test.c linked into ext4-test.o), follow Documentation/dev-tools/kunit/style.rst:

- **Subsystem & Suite naming**: Name the suite <subsystem>_<area> using underscores, without redundant _kunit or _test suffixes in .name (unless matching existing suites in that directory).

- **File naming**: Place new standalone test files in tests/<suite>_kunit.c (or match the subsystem's existing *-test.c / *_kunit.c pattern).

- **Kconfig**:

  ```kconfig
  config FOO_KUNIT_TEST
  	tristate "KUnit test for foo" if !KUNIT_ALL_TESTS
  	depends on FOO && KUNIT
  	default KUNIT_ALL_TESTS
  	help
  	  This builds unit tests for foo.

  	  For more information on KUnit and unit tests in general,
  	  please refer to the KUnit documentation in
  	  Documentation/dev-tools/kunit/.

  	  If unsure, say N.
  ```

---

## 3. Building, Running, and Verifying Tests

You **must** build and run your KUnit tests before creating a commit:

1. **Run the test suite with kunit.py**:

   ```bash
   ./tools/testing/kunit/kunit.py run <suite_name>
   ```

(If a .kunitconfig exists in the repo root or subsystem directory, pass --kunitconfig=.kunitconfig or --kunitconfig=<subdir>/.kunitconfig, or add needed configs via --kconfig_add CONFIG_...=y).

2. **Verify zero build warnings**: Ensure the files you added or modified compile with zero compiler warnings or errors.

3. **Verify test isolation**: Run individual test cases using glob filtering to confirm no test depends on state left behind by an earlier test:

   ```bash
   ./tools/testing/kunit/kunit.py run "<suite_name>.<test_case_name>"
   ```

4. **Verify test fidelity**: Confirm that your tests actually exercise the intended code paths (e.g., via GCOV/coverage if configured, or by mentally/temporarily verifying that breaking the condition under test causes the KUnit expectation to fail).

---

## 4. Creating Kernel Commits & AI Assistant Compliance

When committing your KUnit tests, strictly follow the Linux kernel development rules referenced in README, Documentation/process/coding-style.rst, Documentation/process/submitting-patches.rst, Documentation/process/coding-assistants.rst, and Documentation/process/generated-content.rst:

### 4.1 Coding Style & Licensing
- **SPDX & Module License**:
- Line 1 of every new .c test file must be // SPDX-License-Identifier: GPL-2.0 (or the appropriate GPL-2.0-compatible identifier).
- Include MODULE_LICENSE("GPL"); and MODULE_DESCRIPTION("..."); at the bottom of the test module.
- **Linux Kernel Coding Style**:
- Use **8-character hard tabs** for indentation (never spaces).
- Follow K&R brace placement (opening brace on the same line for
control statements, on the next line for functions; omit braces
around single-statement if/else/for blocks unless one branch
requires braces).
- Keep lines within **80 columns** unless exceeding 80 columns significantly improves readability (e.g., unbroken format strings).
- Do not mix declarations and code in the middle of blocks; keep local
variable declarations cleanly ordered at the start of
functions/blocks.
- **Run checkpatch.pl**:

  ```bash
  ./scripts/checkpatch.pl -g HEAD
  ```

Fix all ERROR, WARNING, and CHECK issues reported by checkpatch.pl.

### 4.2 Commit Structure & Changelog Format
- **Separate logical changes**: Each commit must represent a single logical change and leave the kernel buildable and all tests passing.

- **Subject line**:
- Format: <subsystem>: <concise summary phrase in imperative mood>
- Keep the subject line under 70–75 characters (e.g., ext4: add KUnit tests for extents status tree).

- **Commit message body**:
- Wrap lines at **72–75 columns**.
- Write in the **imperative mood** (e.g., "Add KUnit tests for...", "Export symbol X for KUnit testing...", not "This patch adds..." or "I added...").

- Explain **why** the tests are useful, **what** behaviors, data structures, and edge cases are covered, any minimal production code hooks added (such as KUNIT_STATIC_STUB_REDIRECT or KUnit symbol exports), and how the tests were verified.

- If referencing any prior commit, always use at least 12 characters of the SHA-1 hash and its quoted one-line summary: Commit <12+ char sha1> ("<subject line>").

### 4.3 Mandatory AI Coding Assistant Rules (DCO & Attribution)
Per Documentation/process/coding-assistants.rst and Documentation/process/generated-content.rst:
1. **NEVER add a Signed-off-by: tag**: AI agents cannot legally
certify the Developer Certificate of Origin (DCO). Do **not**
generate or append a Signed-off-by: line for yourself or on behalf
of the human user. The human submitter will review the commit and add
their own Signed-off-by: tag.

2. **Explicitly report verification status**: In your final response to the user, clearly state what was built and tested (including kunit.py and checkpatch.pl results), and explicitly note anything that could not be built or verified.

#2 lore raw
In reply to Theodore Tso
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] ext4: use-after-free in es_cleanup_inode() due to KUnit teardown order - [Medium] ext4: lock leak…

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ext4: use-after-free in es_cleanup_inode() due to KUnit teardown order
- [Medium] ext4: lock leak and sleep-in-atomic in assert_es_tree() under i_es_lock
- [Medium] ext4: extents_status-test: unlock sb->s_umount before kunit_activate_static_stub() --

commit 68a97fa816f24b4b23fae3c4dae5fc1c10033a0d
Author: Theodore Tso <[email protected]>

ext4: add KUinit tests for fs/ext4/extents_status.c

This commit adds KUnit tests for the ext4 extents status tree and bigalloc pending reservations, along with static stub redirects and symbol exports needed by the test module.

Quoted 6 lines
fs/ext4/extents_status-test.c new file +0 -0
new file mode 100644
index 0000000000000..65376666a362f
@@ -0,0 +1,1509 @@

[ ... ]

Quoted 37 lines
fs/ext4/extents_status-test.c new file +37 -0
+static void es_cleanup_inode(struct inode *inode)
+{
+	struct ext4_inode_info *ei = EXT4_I(inode);
+	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
+	struct rb_node *node;
+
+	sbi->s_mount_state &= ~EXT4_FC_REPLAY;
+	ext4_es_remove_extent(inode, 0, EXT_MAX_BLOCKS);
+
+	while ((node = rb_first(&ei->i_pending_tree.root)) != NULL) {
+		struct pending_reservation *pr;
+
+		pr = rb_entry(node, struct pending_reservation, rb_node);
+		ext4_remove_pending(inode, EXT4_C2B(sbi, pr->lclu));
+	}
+}
+
+KUNIT_DEFINE_ACTION_WRAPPER(es_cleanup_inode_action, es_cleanup_inode,
+			    struct inode *);
+
+static struct inode *es_alloc_test_inode(struct kunit *test,
+					 struct super_block *sb,
+					 unsigned long ino)
+{
+	struct ext4_inode_info *ei;
+	int ret;
+
+	ei = kunit_kzalloc(test, sizeof(*ei), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, ei);
+	es_init_mock_inode(ei, sb, ino);
+
+	ret = kunit_add_action_or_reset(test, es_cleanup_inode_action,
+					&ei->vfs_inode);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	return &ei->vfs_inode;
+}

[ ... ]

Quoted 2 lines
fs/ext4/extents_status-test.c new file +2 -0
+static int es_kunit_init(struct kunit *test)
+{

[ ... ]

Quoted 4 lines
fs/ext4/extents_status-test.c new file +4 -0
+	es_init_mock_inode(ctx->ei, sb, 12);
+	ctx->sb = sb;
+	ctx->sbi = sbi;
+	ctx->shrinker_registered = true;

fs/ext4/extents_status-test.c line 222

Quoted 10 lines
fs/ext4/extents_status-test.c new file +10 -0
+	test->priv = ctx;
+
+	kunit_activate_static_stub(test, ext4_da_update_reserve_space,
+				   ext4_da_update_reserve_space_stub);
+	kunit_activate_static_stub(test, ext4_da_release_space,
+				   ext4_da_release_space_stub);
+
+	up_write(&sb->s_umount);
+	return 0;
+}

[Severity: Medium]
Should up_write(&sb->s_umount) happen before kunit_activate_static_stub()?

sget_fc() returns with sb->s_umount held write-locked, and test->priv = ctx is assigned before calling kunit_activate_static_stub(). Inside __kunit_activate_static_stub(), an allocation failure triggers KUNIT_ASSERT_NOT_ERR_OR_NULL(), which aborts the init kthread via kunit_try_catch_throw() before up_write(&sb->s_umount) is reached.

When KUnit then runs es_kunit_exit() in the cleanup kthread, deactivate_super(ctx->sb) -> __super_lock_excl() will attempt to acquire sb->s_umount and deadlock.

Quoted 5 lines
fs/ext4/extents_status-test.c new file +5 -0
+
+static void es_kunit_exit(struct kunit *test)
+{
+	struct es_test_ctx *ctx = test->priv;
+

fs/ext4/extents_status-test.c line 237

Quoted 10 lines
fs/ext4/extents_status-test.c new file +10 -0
+	if (!ctx)
+		return;
+
+	if (ctx->ei && ctx->sb)
+		es_cleanup_inode(&ctx->ei->vfs_inode);
+	if (ctx->shrinker_registered)
+		ext4_es_unregister_shrinker(ctx->sbi);
+	if (ctx->sb)
+		deactivate_super(ctx->sb);
+}

[Severity: High]
Can registering es_cleanup_inode_action() via kunit_add_action_or_reset() in es_alloc_test_inode() cause a use-after-free when KUnit tears down the test?

In lib/kunit/test.c, kunit_run_case_cleanup() runs suite->exit(test) before unwinding test resources via kunit_case_internal_cleanup():

kunit_run_case_cleanup()
suite->exit(test) -> es_kunit_exit()

    ext4_es_unregister_shrinker(ctx->sbi)
    deactivate_super(ctx->sb)

kunit_case_internal_cleanup(test)
kunit_cleanup(test)
es_cleanup_inode_action(inode2)
es_cleanup_inode(inode2)

When test_es_shrinker_skips_precached_inode_until_last_resort() allocates inode2 with es_alloc_test_inode(), es_kunit_exit() destroys the s_es_stats per-CPU counters via ext4_es_unregister_shrinker() and drops the superblock reference via deactivate_super(ctx->sb) before es_cleanup_inode(inode2) runs.

es_cleanup_inode(inode2) and ext4_es_remove_extent() then dereference inode->i_sb after deactivate_super(ctx->sb). If inode2 still has any extents in i_es_tree (for example if an earlier assertion fails before inode2 is shrunk), ext4_es_remove_extent() -> ext4_es_free_extent() will also call percpu_counter_dec() on the already-destroyed s_es_stats counters where fbc->counters is NULL.

Quoted 34 lines
fs/ext4/extents_status-test.c new file +34 -0
+
+static void assert_es_tree(struct kunit *test, struct inode *inode,
+			   const struct expected_es *exp, unsigned int nr_exp)
+{
+	struct ext4_inode_info *ei = EXT4_I(inode);
+	struct rb_node *node;
+	unsigned int i = 0;
+	ext4_lblk_t prev_end = 0;
+
+	read_lock(&ei->i_es_lock);
+	for (node = rb_first(&ei->i_es_tree.root); node; node = rb_next(node)) {
+		struct extent_status *es =
+			rb_entry(node, struct extent_status, rb_node);
+
+		KUNIT_ASSERT_LT_MSG(test, i, nr_exp,
+				    "unexpected extra extent [%u/%u) type 0x%x",
+				    es->es_lblk, es->es_len, ext4_es_type(es));
+
+		if (i > 0)
+			KUNIT_EXPECT_GT_MSG(test, es->es_lblk, prev_end,
+					    "extent %u not strictly ordered",
+					    i);
+
+		KUNIT_EXPECT_EQ_MSG(test, es->es_lblk, exp[i].lblk,
+				    "extent %u lblk mismatch", i);
+		KUNIT_EXPECT_EQ_MSG(test, es->es_len, exp[i].len,
+				    "extent %u len mismatch", i);
+		KUNIT_EXPECT_EQ_MSG(test, ext4_es_type(es), exp[i].type,
+				    "extent %u type mismatch", i);
+
+		if (exp[i].type & (EXTENT_STATUS_WRITTEN |
+				   EXTENT_STATUS_UNWRITTEN))
+			KUNIT_EXPECT_EQ_MSG(test, ext4_es_pblock(es),
+					    exp[i].pblk,

fs/ext4/extents_status-test.c line 281

Quoted 10 lines
fs/ext4/extents_status-test.c new file +10 -0
+					    "extent %u pblk mismatch", i);
+
+		prev_end = es->es_lblk + es->es_len - 1;
+		i++;
+	}
+	read_unlock(&ei->i_es_lock);
+
+	KUNIT_EXPECT_EQ(test, i, nr_exp);
+	KUNIT_EXPECT_EQ(test, ei->i_es_all_nr, nr_exp);
+}

[Severity: Medium]
Is it safe to invoke KUNIT_ASSERT_LT_MSG() and KUNIT_EXPECT_*_MSG() while holding read_lock(&ei->i_es_lock)?

When a KUnit expectation or assertion fails, __kunit_do_failed_assertion() -> kunit_fail() calls kunit_alloc_string_stream(test, GFP_KERNEL), which performs a sleeping allocation under the ei->i_es_lock rwlock.

In addition, if KUNIT_ASSERT_LT_MSG() fails, __kunit_abort() exits the test kthread immediately without calling read_unlock(&ei->i_es_lock). During teardown, es_kunit_exit() -> es_cleanup_inode() -> ext4_es_remove_extent() will then call write_lock(&EXT4_I(inode)->i_es_lock) and deadlock on the leaked read lock.

[ ... ]

Quoted 11 lines
fs/ext4/extents_status-test.c new file +11 -0
+static void
+test_es_shrinker_skips_precached_inode_until_last_resort(struct kunit *test)
+{
+	struct es_test_ctx *ctx = test->priv;
+	struct inode *inode1 = &ctx->ei->vfs_inode;
+	struct inode *inode2;
+	struct shrinker *shrink = ctx->sbi->s_es_shrinker;
+	struct shrink_control sc;
+	unsigned long shrunk;
+
+	inode2 = es_alloc_test_inode(test, ctx->sb, 13);

[ ... ]

Quoted 1 line

+}

[ ... ]

Quoted 3 lines
Signature · 1 line
2.51.2
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
author maintainer
#3 lore raw

PATCH 1/1 ext4: add KUinit tests for fs/ext4/extents_status.c

These tests were created with the help of Gemini Pro and verified by a human.

---
Changes from v1:
- Fix Sasshiko issues:
* Fix a Use-After-Free on sb and destroyed percpu_counters when es_cleanup_inode_action() runs after es_kunit_exit()
* Prevent system memory reclaims from potentially causing flaky tests
* Avoid a potential deadlock after a test failure
* Follow the locking rules in the tests by taking i_data_sem before modifying the extent status tree in the tests
* Fully initialize the shrink_control structure

 fs/ext4/Makefile              |    3 +-
 fs/ext4/extents.c             |    5 -
 fs/ext4/extents_status-test.c | 1640 +++++++++++++++++++++++++++++++++
 fs/ext4/extents_status.c      |   19 +
 fs/ext4/inode.c               |   10 +
 5 files changed, 1671 insertions(+), 6 deletions(-)
 create mode 100644 fs/ext4/extents_status-test.c
fs/ext4/Makefile +2 -1
index 3f9fc0eb8eca..74b485dbb229 100644
@@ -15,7 +15,8 @@ ext4-y	:= balloc.o bitmap.o block_validity.o dir.o ext4_jbd2.o extents.o \
 ext4-$(CONFIG_EXT4_FS_POSIX_ACL)	+= acl.o
 ext4-$(CONFIG_EXT4_FS_SECURITY)		+= xattr_security.o
 ext4-test-objs				+= inode-test.o mballoc-test.o \
-					   extents-test.o hash-test.o
+					   extents-test.o hash-test.o \
+					   extents_status-test.o
 obj-$(CONFIG_EXT4_KUNIT_TESTS)		+= ext4-test.o
 ext4-$(CONFIG_FS_VERITY)		+= verity.o
 ext4-$(CONFIG_FS_ENCRYPTION)		+= crypto.o
fs/ext4/extents.c +0 -5
index 76038b6c3655..6c018bcc0118 100644
@@ -6407,12 +6407,7 @@ EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_split_convert_extents_test);
 
 EXPORT_SYMBOL_FOR_EXT4_TEST(__ext4_ext_dirty);
 EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_ext_zeroout);
-EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_register_shrinker);
-EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_unregister_shrinker);
 EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_map_create_blocks);
-EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_init_tree);
-EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_lookup_extent);
-EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_insert_extent);
 EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_ext_insert_extent);
 EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_find_extent);
 EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_issue_zeroout);
fs/ext4/extents_status-test.c new file +1640 -0

1,640 lines not on this page · raw message

fs/ext4/extents_status.c +19 -0
index 6e4a191e8219..64bac0c4c1fa 100644
@@ -2390,3 +2390,22 @@ static int __revise_pending(struct inode *inode, ext4_lblk_t lblk,
 out:
 	return (ret < 0) ? ret : pendings;
 }
+
+#if IS_ENABLED(CONFIG_EXT4_KUNIT_TESTS)
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_init_tree);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_insert_extent);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_cache_extent);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_remove_extent);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_find_extent_range);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_lookup_extent);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_scan_range);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_scan_clu);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_register_shrinker);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_unregister_shrinker);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_seq_es_shrinker_info_show);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_init_pending_tree);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_remove_pending);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_is_pending);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_es_insert_delayed_extent);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_clear_inode_es);
+#endif
fs/ext4/inode.c +10 -0
index 26f0f9714f03..966a760f64e6 100644
@@ -350,6 +350,9 @@ void ext4_da_update_reserve_space(struct inode *inode,
 	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 	struct ext4_inode_info *ei = EXT4_I(inode);
 
+	KUNIT_STATIC_STUB_REDIRECT(ext4_da_update_reserve_space,
+				   inode, used, quota_claim);
+
 	spin_lock(&ei->i_block_reservation_lock);
 	trace_ext4_da_update_reserve_space(inode, used, quota_claim);
 	if (unlikely(used > ei->i_reserved_data_blocks)) {
@@ -1646,6 +1649,8 @@ void ext4_da_release_space(struct inode *inode, int to_free)
 	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 	struct ext4_inode_info *ei = EXT4_I(inode);
 
+	KUNIT_STATIC_STUB_REDIRECT(ext4_da_release_space, inode, to_free);
+
 	if (!to_free)
 		return;		/* Nothing to release, exit */
 
@@ -6973,3 +6978,8 @@ vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf)
 	sb_end_pagefault(inode->i_sb);
 	return ret;
 }
+
+#if IS_ENABLED(CONFIG_EXT4_KUNIT_TESTS)
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_da_update_reserve_space);
+EXPORT_SYMBOL_FOR_EXT4_TEST(ext4_da_release_space);
+#endif

base-commit: 93f51579e7df248780214094418f205253383cc5

Signature · 1 line
2.56.0.212.g129ede20d9-goog
#4 lore raw
Sashiko has reviewed this patch and found no issues.
Quoted 4 lines

These tests were created with the help of Gemini Pro and verified by a human.

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1