Skip to content

fix(tx_pool): guard homa_tx_pool_gc against uninitialized max_pool - #94

Open
randomizedcoder wants to merge 1 commit into
PlatformLab:mainfrom
randomizedcoder:fix/tx-pool-max-pool-uninit
Open

randomizedcoder wants to merge 1 commit into
PlatformLab:mainfrom
randomizedcoder:fix/tx-pool-max-pool-uninit

Conversation

@randomizedcoder

Copy link
Copy Markdown

Bug: uninitialized max_pool deref in homa_tx_pool_gc()

homa_tx_pool_gc() declares its max_pool local without an initializer:

struct homa_tx_pool *max_pool;

max_pool is assigned only inside the pool-scanning loop, and only when a pool's low_mark beats the running maximum (which starts at -1):

max_low_mark = -1;
for (i = 0; i <= homa->max_numa; i++) {
    struct homa_tx_pool *pool = homa->tx_pools[i];
    if (!pool)
        continue;
    ...
    if (pool->low_mark > max_low_mark) {
        max_low_mark = pool->low_mark;
        max_pool = pool;
    }
    ...
}
/* ... */
spin_lock_bh(&max_pool->mutex);   /* max_pool may be indeterminate */

If no pool is eligible — e.g. every homa->tx_pools[] entry is NULL while homa->max_numa is still set (exactly the state left behind by homa_tx_pool_cleanup()) — the loop body never runs, max_pool keeps its indeterminate value, and the code dereferences it just below, crashing on garbage.

Fix

-	struct homa_tx_pool *max_pool;
+	struct homa_tx_pool *max_pool = NULL;
+	/* No pool was eligible (e.g. every tx_pools[] entry is NULL, so the
+	 * loop above never set max_pool); nothing to free and no lock to take.
+	 */
+	if (!max_pool)
+		return;
+
 	spin_lock_bh(&max_pool->mutex);

Minimal and self-contained: initialize to NULL, and bail out early when no pool was selected (there is nothing to reclaim and no lock to take).

Verification (TDD, red → green)

A regression test, homa_tx_pool_gc__eligible_pool_selection, was written first and confirmed to fail, then made to pass by the fix.

Red (before the fix) — the corner row faults inside homa_tx_pool_gc:

...case: single_pool_frees_down_to_release_max
...case: pools_present_but_nothing_to_free
...case: no_eligible_pool_must_not_deref
AddressSanitizer: SEGV ../homa_tx_pool.c:436 in homa_tx_pool_gc
  (WRITE to poisoned stack address 0xf5f5f5f5f5f5f5f5 — the uninitialized max_pool)

Green (after the fix) — the new test passes and the existing homa_tx_pool suite stays green:

homa_tx_pool.homa_tx_pool_gc__eligible_pool_selection ......... 1 / 1 passed
homa_tx_pool (whole fixture) ................................. 30 / 30 passed

Built and run against the kselftest unit harness in test/ (ASan on).


A proposal for your consideration: table-driven tests + TDD

This is the first of a small series of fixes I'd like to contribute, each found via static analysis / fuzzing and each developed test-first. I've written the tests in a table-driven style, and since that's new for this repo I wanted to raise it explicitly and friendly-ly, as an idea for evaluation rather than a fait accompli — happy to convert to the existing one-scenario-per-TEST_F style if you'd prefer.

What it looks like (from this PR):

static const struct {
    const char *name;
    int pages, low_mark, frees_per_sec;
    bool clear_pools;
    int exp_core0_avail;
} cases[] = {
    {"single_pool_frees_down_to_release_max", 10, 8, 10, false, 5},
    {"pools_present_but_nothing_to_free",      0, 0, 10, false, 0},
    {"no_eligible_pool_must_not_deref",        0, 0, 10, true,  0},
};

for (i = 0; i < (int)ARRAY_SIZE(cases); i++) {
    TH_LOG("case: %s", cases[i].name);   /* names the row on failure */
    ...                                  /* EXPECT_* (not ASSERT_*) so all rows run */
}

Why we think it helps this project:

  • Coverage is visible. Positive / boundary / corner cases sit in one table; it's easy to see what's covered and to add a row.
  • Failures are self-identifying. TH_LOG("case: %s", ...) prints the row name before its assertions, so a red row is named, not just a line number.
  • It pairs naturally with TDD. Each new bug becomes one more row whose expectation is written before the fix — the corner row here is literally the crash this PR fixes.

Deliberate choices to stay friendly to the existing harness:

  • Uses only the repo's own kselftest_harness.h (TEST_F, EXPECT_*, TH_LOG) — no new framework or dependency.
  • EXPECT_* inside the loop (never ASSERT_*), so one failing row doesn't hide the others.
  • Each row is independent (here via cleanup + init), so ordering never matters.

If this direction is welcome, the follow-up fixes in this series will use the same pattern to demonstrate it on a range of bug shapes. If not, just say the word and I'll match the current convention. Thanks for taking a look!

homa_tx_pool_gc() declares `max_pool` without an initializer and only
assigns it inside the pool-scanning loop when a pool's low_mark exceeds
the running maximum (which starts at -1). If no pool is eligible -- for
example every homa->tx_pools[] entry is NULL while homa->max_numa is
still set (the state left by homa_tx_pool_cleanup()) -- the loop never
runs its body, max_pool keeps its indeterminate value, and the code then
dereferences it at `spin_lock_bh(&max_pool->mutex)` / `max_pool->...`,
crashing on garbage.

Initialize `max_pool` to NULL and return early when no pool was selected;
there is nothing to reclaim and no lock to take in that case.

Adds a table-driven regression test, homa_tx_pool_gc__eligible_pool_selection,
whose corner row ("no_eligible_pool_must_not_deref") reproduces the fault:
before the fix it SIGSEGVs in homa_tx_pool_gc (ASan: write to a poisoned
stack address); after the fix all rows pass and the existing tx_pool suite
stays green (30/30).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant