Skip to content

kmod: fix span refcount leaks around ternfs_unlink_span - #133

Open
stevephodgson wants to merge 3 commits into
mainfrom
fix-span-refcount-leak
Open

kmod: fix span refcount leaks around ternfs_unlink_span#133
stevephodgson wants to merge 3 commits into
mainfrom
fix-span-refcount-leak

Conversation

@stevephodgson

@stevephodgson stevephodgson commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

This PR is three commits that fix different bugs in and around ternfs_unlink_span(). They are in one PR merely because stacked changes in github suck.

Fix 1: Clarify contract of ternfs_unlink_span()

ternfs_unlink_span() here dropped a single reference while both of its callers held
two: one owned by the span cache (the rb-tree) and one owned by the caller. The
two callers disagreed about which one that put covered.

ternfs_get_span() hereexpected its own reference to survive. It looks the span up
and takes a reference under __lock:

  struct ternfs_span* span = lookup_span(&spans->__spans, offset);
  if (likely(span != NULL)) {
    atomic_inc(&span->refcount);
  }

and then, if the span has expired, unlinks it and sets span = NULL. Nothing in
that block consumes the reference it just took, and it cannot have expected
ternfs_unlink_span() to consume it: that would free the object while span was
still a live pointer. So the caller's reference was simply dropped on the floor.

Meanwhile, file_readpage() here expected the opposite. Its

                span = NULL; // unlink already reduced refcount

cleared the pointer so that the puts at the retry label and at out: would not
double free. That is self-consistent, but it leaks the cache's reference
instead, and since the span is off the rb-tree by then ternfs_free_file_spans()
cannot reclaim it at inode eviction either.

This PR gives ternfs_unlink_span() one unambiguous contract: it drops only the reference
held by the span cache, and the caller keeps its own. Move the put inside the
RB_EMPTY_NODE check while we are here -- the unconditional put was also a latent
double put, since a thread which lost a race to unlink the same span skipped the
rb_erase() but still dropped a reference it did not own. Then fix the callers to
match: ternfs_get_span() puts its own reference, and file_readpage() keeps its
pointer so the put at the retry label runs.

Fix 2: don't leak the span on a misaligned span in file_readpage

Simple error path fix

Fix 3: kmod: don't leak the span when readahead can't allocate pages

Simple error path fix

@stevephodgson stevephodgson added the ci If given to a PR, the PR will run CI checks. label Jul 30, 2026
@stevephodgson
stevephodgson force-pushed the fix-span-refcount-leak branch 5 times, most recently from 879ee38 to afee0ce Compare July 31, 2026 16:29
@stevephodgson stevephodgson changed the title terntests: cover symlink reads and span cache eviction kmod: fix span refcount leaks around ternfs_unlink_span Jul 31, 2026
stevephodgson and others added 3 commits July 31, 2026 16:39
It dropped one reference while both callers held two: one owned by the
span cache, one owned by the caller. ternfs_get_span() expected its own
reference to survive, file_readpage() expected it to be consumed, so
whichever ran, one reference leaked.

Make it drop only the cache's reference and leave the caller's alone, and
fix both callers to match. The put also moves inside the RB_EMPTY_NODE
check: unconditionally it was a latent double put, since a thread losing a
race to unlink the same span skipped the rb_erase() but still dropped a
reference it did not own.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Clearing span before the "span start not a multiple of page size" error
skipped the ternfs_put_span() at out:.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
file_readahead() returned outright when no page could be allocated,
leaking the span it had just got. Reachable under memory pressure on any
kernel >= 5.18.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@stevephodgson
stevephodgson force-pushed the fix-span-refcount-leak branch from afee0ce to 06e12fe Compare July 31, 2026 16:40
@stevephodgson
stevephodgson requested review from bitonic and mcrnic and removed request for bitonic July 31, 2026 16:43
@stevephodgson
stevephodgson marked this pull request as ready for review July 31, 2026 16:43
@stevephodgson
stevephodgson requested a review from bitonic July 31, 2026 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci If given to a PR, the PR will run CI checks.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant