fix(tx_pool): guard homa_tx_pool_gc against uninitialized max_pool - #94
Open
randomizedcoder wants to merge 1 commit into
Open
randomizedcoder wants to merge 1 commit into
randomizedcoder wants to merge 1 commit into
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug: uninitialized
max_poolderef inhoma_tx_pool_gc()homa_tx_pool_gc()declares itsmax_poollocal without an initializer:max_poolis assigned only inside the pool-scanning loop, and only when a pool'slow_markbeats the running maximum (which starts at-1):If no pool is eligible — e.g. every
homa->tx_pools[]entry isNULLwhilehoma->max_numais still set (exactly the state left behind byhoma_tx_pool_cleanup()) — the loop body never runs,max_poolkeeps its indeterminate value, and the code dereferences it just below, crashing on garbage.Fix
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:Green (after the fix) — the new test passes and the existing
homa_tx_poolsuite stays green: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_Fstyle if you'd prefer.What it looks like (from this PR):
Why we think it helps this project:
TH_LOG("case: %s", ...)prints the row name before its assertions, so a red row is named, not just a line number.Deliberate choices to stay friendly to the existing harness:
kselftest_harness.h(TEST_F,EXPECT_*,TH_LOG) — no new framework or dependency.EXPECT_*inside the loop (neverASSERT_*), so one failing row doesn't hide the others.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!