Skip to content

bpf, arena: fix range_tree consistency on allocation failure - #13435

Open
kernel-patches-daemon-bpf[bot] wants to merge 3 commits into
bpf-next_basefrom
series/1150936=>bpf-next
Open

bpf, arena: fix range_tree consistency on allocation failure#13435
kernel-patches-daemon-bpf[bot] wants to merge 3 commits into
bpf-next_basefrom
series/1150936=>bpf-next

Conversation

@kernel-patches-daemon-bpf

Copy link
Copy Markdown

Pull request for series with
subject: bpf, arena: fix range_tree consistency on allocation failure
version: 4
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150936

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 5e289c5
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150936
version: 4

@kernel-patches-review-bot

Copy link
Copy Markdown
commit 40aa201ccb6eb071e5b4f258ad251da6d6736d01
Author: Yuan Chen <chenyuan@kylinos.cn>

bpf, arena: fix range_tree_clear inconsistency on kmalloc_nolock failure

range_tree_clear() pre-allocates the right-half node before modifying
the tree, so an allocation failure returns -ENOMEM without altering the
range tree.

> diff --git a/kernel/bpf/range_tree.c b/kernel/bpf/range_tree.c
> index 2f28886f3ff7..15b588377a76 100644
> --- a/kernel/bpf/range_tree.c
> +++ b/kernel/bpf/range_tree.c
> @@ -143,16 +143,22 @@ int range_tree_clear(struct range_tree *rt, u32 start, u32 len)
>  		if (rn->rn_start < start && rn->rn_last > last) {
>  			u32 old_last = rn->rn_last;
>
> +			/*
> +			 * Pre-allocate the right-half node before modifying
> +			 * the tree. If allocation fails we return -ENOMEM
> +			 * without altering the range tree.
> +			 */
> +			new_rn = kmalloc_nolock(sizeof(struct range_node),
> +						__GFP_ACCOUNT, NUMA_NO_NODE);
> +			if (!new_rn)
> +				return -ENOMEM;
> +
>  			/* Overlaps with the entire clearing range */
>  			range_it_remove(rn, rt);
>  			rn->rn_last = start - 1;
>  			range_it_insert(rn, rt);
>
> -			/* Add a range */
> -			new_rn = kmalloc_nolock(sizeof(struct range_node), __GFP_ACCOUNT,
> -						NUMA_NO_NODE);
> -			if (!new_rn)
> -				return -ENOMEM;
> +			/* Add right-half range */
>  			new_rn->rn_start = last + 1;
>  			new_rn->rn_last = old_last;
>  			range_it_insert(new_rn, rt);

This isn't a bug, but would it read better to keep the "Overlaps with
the entire clearing range" comment at the top of the branch, next to
the condition it describes, rather than having it appear in the middle
after the new allocation block?



AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

In-Reply-To-Subject: bpf, arena: fix range_tree_clear inconsistency on kmalloc_nolock failure
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32736729475

@kernel-patches-review-bot

Copy link
Copy Markdown
commit 5181d7902a32f573705e0f22e14a9d795971f2ce
Author: Yuan Chen <chenyuan@kylinos.cn>

bpf, arena: fix range_tree_set inconsistency on kmalloc_nolock failure

> bpf, arena: fix range_tree_set inconsistency on kmalloc_nolock failure
>
> range_tree_set() pre-allocates the node needed for a brand-new range
> before calling range_tree_clear(), so an allocation failure returns
> -ENOMEM without having modified the tree (previously the overlapping
> nodes were already removed by range_tree_clear() before the allocation
> was attempted, permanently losing the cleared sub-ranges).
>
> Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>

This isn't a bug, but would it be worth adding a Fixes: tag for the commit
that introduced the clear-then-allocate ordering, so the stable backport
scope is clear? The introducing commit could not be determined from this
checkout.

Also, could the body be split into two sentences so the "previously ..."
rationale is not buried in a parenthetical? For example:

  range_tree_set() pre-allocates the node needed for a brand-new range
  before calling range_tree_clear(), so an allocation failure returns
  -ENOMEM without having modified the tree. Previously the overlapping
  nodes were already removed by range_tree_clear() before the allocation
  was attempted, permanently losing the cleared sub-ranges.



AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

In-Reply-To-Subject: bpf, arena: fix range_tree_set inconsistency on kmalloc_nolock failure
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32736729475

@kernel-patches-review-bot

Copy link
Copy Markdown
commit ffd2b7be8945c61c72945ecd8abb6a15650741af
Author: Yuan Chen <chenyuan@kylinos.cn>

bpf, arena: check range_tree_set return in arena_free_pages and
arena_free_worker

This commit adds error handling for range_tree_set() failures in the arena
allocation and deallocation paths to prevent inconsistent state between the
free tree and PTEs.

> bpf, arena: check range_tree_set return in arena_free_pages and arena_free_worker
>
> arena_free_pages() and arena_free_worker() now handle range_tree_set()
> errors. arena_free_pages() aborts the free on error, and
> arena_free_worker() moves range_tree_set() before PTE clearing so that a
> failed tree update leaves the PTEs intact instead of freeing pages that
> the arena free tree does not track.
>
> Also check the range_tree_set() return value in arena_alloc_pages()'s
> error path, which restores the unpopulated tail of a partially allocated
> range; log a warning instead of silently leaking the virtual range when
> the tree update fails.
>
> range_tree_set() is failure-atomic (it pre-allocates the node before
> touching the tree), so on -ENOMEM the range stays tracked as allocated
> and the pages remain mapped and accessible. A failed free is therefore
> retryable, and arena_map_free() reclaims any retained pages at map
> destruction; aborting the free avoids clearing PTEs for pages the arena
> free tree does not track.
>
> In arena_free_worker() a failed tree update used to leave the span in
> the drained list, where the second loop would still flush TLB entries,
> zap user VMAs, and free the span itself: the free request was dropped,
> user mappings were destroyed for a free that never happened, and the
> pages stayed mapped until map destruction. Keep failed spans on
> arena->free_spans instead and retry them on a later worker run; only
> spans whose PTE clearing actually ran are flushed, zapped, and released.
>
> Suggested-by: Emil Tsalapatis <emil@etsalapatis.com>
> Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>

This looks like a bug fix for error handling in the arena code introduced
in an earlier commit. Should this include a Fixes: tag pointing to:

  Fixes: dc11a4dba246 ("bpf: Recover arena kernel faults with scratch page")



AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

In-Reply-To-Subject: bpf, arena: check range_tree_set return in arena_free_pages and arena_free_worker
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32736729475

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: d83fba2
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150936
version: 4

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: ce36e38
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150936
version: 4

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 05ea1b6
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150936
version: 4

range_tree_clear() pre-allocates the right-half node before modifying
the tree, so an allocation failure returns -ENOMEM without altering the
range tree.

Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>
range_tree_set() pre-allocates the node needed for a brand-new range
before calling range_tree_clear(), so an allocation failure returns
-ENOMEM without having modified the tree (previously the overlapping
nodes were already removed by range_tree_clear() before the allocation
was attempted, permanently losing the cleared sub-ranges).

Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>
…_free_worker

arena_free_pages() and arena_free_worker() now handle range_tree_set()
errors. arena_free_pages() aborts the free on error, and
arena_free_worker() moves range_tree_set() before PTE clearing so that a
failed tree update leaves the PTEs intact instead of freeing pages that
the arena free tree does not track.

Also check the range_tree_set() return value in arena_alloc_pages()'s
error path, which restores the unpopulated tail of a partially allocated
range; log a warning instead of silently leaking the virtual range when
the tree update fails.

range_tree_set() is failure-atomic (it pre-allocates the node before
touching the tree), so on -ENOMEM the range stays tracked as allocated
and the pages remain mapped and accessible. A failed free is therefore
retryable, and arena_map_free() reclaims any retained pages at map
destruction; aborting the free avoids clearing PTEs for pages the arena
free tree does not track.

In arena_free_worker() a failed tree update used to leave the span in
the drained list, where the second loop would still flush TLB entries,
zap user VMAs, and free the span itself: the free request was dropped,
user mappings were destroyed for a free that never happened, and the
pages stayed mapped until map destruction. Keep failed spans on
arena->free_spans instead and retry them on a later worker run; only
spans whose PTE clearing actually ran are flushed, zapped, and released.

Suggested-by: Emil Tsalapatis <emil@etsalapatis.com>
Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>
@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 1555de3
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150936
version: 4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant