page_cache_ext: fix use-after-free in list sampling + a latent lock-pairing hazard - #6
Open
anadav wants to merge 2 commits into
Open
page_cache_ext: fix use-after-free in list sampling + a latent lock-pairing hazard#6anadav wants to merge 2 commits into
anadav wants to merge 2 commits into
Conversation
The snip-then-score-unlocked optimization in __bpf_cache_ext_list_sample() races with valid_folios_del(): the cache_ext_list_node wrapper is kfreed after that path drops the registry write_lock, so nodes sitting in the per-CPU sample_folios_arr[] can be freed while score_fn() dereferences them. The LIST_POISON check in __putback_list_nodes() catches only some of these, and itself reads freed memory. Hold the registry write_lock across snip + score + putback. score_fn is non-sleepable, so calling it under rwlock_t is safe. Note: this serializes sampling against all list updates and measurably slows sampling-based policies under memory pressure (we measured ~30% throughput loss for the LHD policy on YCSB-C/LevelDB with a 4 GiB cgroup, on a 6.18 port of this code); a perf-preserving fix would refcount the node wrapper. Correctness first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ed with cache_ext_list_iterate_extended() chooses read_lock vs write_lock by comparing the iterate modes against CACHE_EXT_ITERATE_SKIP, but releases by comparing against CACHE_EXT_CONTINUE_ITER -- an iterate *return code* from a different enum. This works today only because both constants happen to be 0; reordering either enum would silently pair read_lock with write_unlock and corrupt the rwlock. Use the same condition on both sides. Co-Authored-By: Claude Fable 5 <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.
Two synchronization fixes in
mm/page_cache_ext_ds.c, found while porting cache_ext to Linux 6.18 and benchmarking it:1. Use-after-free in
__bpf_cache_ext_list_sample(). The snip-then-score-unlocked optimization races withvalid_folios_del(), which kfrees thecache_ext_list_nodewrapper after dropping the registry write_lock. Nodes sitting in the per-CPUsample_folios_arr[]can therefore be freed whilescore_fn()dereferences them; theLIST_POISONcheck in__putback_list_nodes()catches only some cases and itself reads freed memory. The fix holds the registry write_lock across snip + score + putback (score_fnis non-sleepable, so calling it underrwlock_tis safe).Honest caveat: this serializes sampling against all list updates and measurably slows sampling-based policies under memory pressure — on our 6.18 port we measured ~30% throughput loss for the LHD policy (YCSB-C on LevelDB, 4 GiB cgroup, victim quality unchanged; the loss is lock serialization in the reclaim path — disk utilization drops from saturated to ~75%). A performance-preserving fix would refcount the node wrapper instead; we took correctness first and want to flag the trade-off to you rather than hide it.
2. Latent lock-pairing hazard in
cache_ext_list_iterate_extended(). The lock is chosen by comparing iterate modes againstCACHE_EXT_ITERATE_SKIP, but released by comparing againstCACHE_EXT_CONTINUE_ITER— a return code from a different enum. This is benign today only because both constants happen to be 0; reordering either enum would silently pairread_lockwithwrite_unlockand corrupt the rwlock. The fix uses the same condition on both sides.🤖 Generated with Claude Code