fix(tx_pool): guard homa_tx_pool_gc against uninitialized max_pool

[randomizedcoder]

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!

[johnousterhout]

Thanks for the PRs; I'm starting to work through them now.

Concerning your proposal to change the structure of the unit tests to be more table driven: I'm pretty comfortable with the current structure of the tests and it's not obvious to me that the table-driven approach is better, so I'm going to stick with the current approach; thanks anyway for the suggestion.

[johnousterhout]

Also, a couple of comments on Homa testing style:

• No TH_LOG statements, please. Output should be completely quiet unless there are test failures.
• Tests are organized in a structure isomorphic to the code (which makes it easy to see coverage). This means (a) one test file per code file (except I put tests for inline methods in header files in the same file as tests for the corresponding code file), (b) one group of tests for each function in a file, in the same order that the functions appear in the file, and (c) tests for a function are ordered according to the code they test (except sometimes there is an initial "basics" test that exercises a bunch of random stuff). The new test you added is not in its proper isomorphic position (this may be harder to do with the table-driven approach).

[johnousterhout]

After looking over this PR, I don't think it's necessary. The problem discussed here can only occur if someone invokes homa_tx_pool_gc after invoking homa_tx_pool_cleanup. That is a violation of the homa_tx_pool API (no pool functions may be invoked after homa_tx_pool_cleanup is invoked). I considered keeping the check anyway (belt and suspenders?) but decided against it: having that check there could mislead people into thinking it's okay to invoke homa_tx_pool functions after homa_tx_pool_cleanup has been invoked, and that is not the case.

I will clarify the comments for homa_tx_pool_cleanup so it's clear that you can't use a pool after it has been cleaned up.

Thanks anyway for submitting the PR...