[Deepin-Kernel-SIG] [linux 6.6-y] [Upstream] bpf: Charge modmem for struct_ops trampoline#1165
Open
opsiff wants to merge 1 commit intodeepin-community:linux-6.6.yfrom
Open
[Deepin-Kernel-SIG] [linux 6.6-y] [Upstream] bpf: Charge modmem for struct_ops trampoline#1165opsiff wants to merge 1 commit intodeepin-community:linux-6.6.yfrom
opsiff wants to merge 1 commit intodeepin-community:linux-6.6.yfrom
Conversation
mainline inclusion from mainline-v6.7-rc1 category: feature Current code charges modmem for regular trampoline, but not for struct_ops trampoline. Add bpf_jit_[charge|uncharge]_modmem() to struct_ops so the trampoline is charged in both cases. Signed-off-by: Song Liu <song@kernel.org> Link: https://lore.kernel.org/r/20230914222542.2986059-1-song@kernel.org Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org> (cherry picked from commit 5c04433) Signed-off-by: Wentao Guan <guanwentao@uniontech.com>
Reviewer's GuideIntroduce modmem charging and uncharging for struct_ops trampolines to match regular trampolines, updating allocation and free paths with proper error handling. Sequence diagram for struct_ops trampoline allocation and modmem chargingsequenceDiagram
participant Caller
participant "bpf_struct_ops_map_alloc()"
participant "bpf_jit_charge_modmem()"
participant "bpf_jit_alloc_exec()"
participant "bpf_jit_uncharge_modmem()"
participant "__bpf_struct_ops_map_free()"
Caller->>"bpf_struct_ops_map_alloc()": Request struct_ops map allocation
"bpf_struct_ops_map_alloc()"->>"bpf_jit_charge_modmem()": Charge modmem for trampoline
alt Charge fails
"bpf_struct_ops_map_alloc()"->>"__bpf_struct_ops_map_free()": Free resources
"bpf_struct_ops_map_alloc()"-->>Caller: Return error
else Charge succeeds
"bpf_struct_ops_map_alloc()"->>"bpf_jit_alloc_exec()": Allocate trampoline image
alt Allocation fails
"bpf_struct_ops_map_alloc()"->>"bpf_jit_uncharge_modmem()": Uncharge modmem
"bpf_struct_ops_map_alloc()"->>"__bpf_struct_ops_map_free()": Free resources
"bpf_struct_ops_map_alloc()"-->>Caller: Return error
else Allocation succeeds
"bpf_struct_ops_map_alloc()"-->>Caller: Return map
end
end
Sequence diagram for struct_ops trampoline free and modmem unchargingsequenceDiagram
participant Caller
participant "__bpf_struct_ops_map_free()"
participant "bpf_jit_free_exec()"
participant "bpf_jit_uncharge_modmem()"
Caller->>"__bpf_struct_ops_map_free()": Free struct_ops map
alt st_map->image exists
"__bpf_struct_ops_map_free()"->>"bpf_jit_free_exec()": Free trampoline image
"__bpf_struct_ops_map_free()"->>"bpf_jit_uncharge_modmem()": Uncharge modmem
end
"__bpf_struct_ops_map_free()"-->>Caller: Done
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
opsiff
changed the title
deepin pr auto review这段代码是关于BPF (Berkeley Packet Filter) struct_ops实现的部分修改，主要涉及到内存分配和释放的改进。我来对这段代码进行审查： 1. 语法逻辑代码语法正确，逻辑流程清晰。主要改进是将内存分配和释放的操作进行了更细致的处理，特别是对 2. 代码质量代码质量有所提升，主要表现在：
3. 代码性能性能方面没有明显变化，因为主要是对错误处理路径的改进，不会影响正常执行路径的性能。 4. 代码安全安全性的改进较为显著：
改进建议：
static void __bpf_struct_ops_map_free(struct bpf_map *map)
{
struct bpf_struct_ops_map *st_map = container_of(map, struct bpf_struct_ops_map, map);
if (st_map->image) {
bpf_jit_free_exec(st_map->image);
bpf_jit_uncharge_modmem(PAGE_SIZE);
}
if (st_map->links)
bpf_struct_ops_map_put_progs(st_map);
bpf_map_area_free(st_map->links);
bpf_map_area_free(st_map->uvalue);
bpf_map_area_free(st_map);
}
static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
{
struct bpf_struct_ops_map *st_map;
const struct btf_type *t, *vt;
struct bpf_map *map;
int ret;
st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id);
if (!st_ops)
return ERR_PTR(-ENOENT);
t = st_ops->type;
vt = btf_type_by_id(st_ops->btf, st_ops->value_type_id);
if (!t || !vt)
return ERR_PTR(-EINVAL);
st_map = bpf_map_area_alloc(sizeof(*st_map), NUMA_NO_NODE);
if (!st_map)
return ERR_PTR(-ENOMEM);
st_map->st_ops = st_ops;
map = &st_map->map;
ret = bpf_jit_charge_modmem(PAGE_SIZE);
if (ret) {
bpf_map_area_free(st_map);
return ERR_PTR(ret);
}
st_map->image = bpf_jit_alloc_exec(PAGE_SIZE);
if (!st_map->image) {
bpf_jit_uncharge_modmem(PAGE_SIZE);
bpf_map_area_free(st_map);
return ERR_PTR(-ENOMEM);
}
st_map->uvalue = bpf_map_area_alloc(vt->size, NUMA_NO_NODE);
st_map->links = bpf_map_area_alloc(btf_type_vlen(t) * sizeof(struct bpf_links *), NUMA_NO_NODE);
if (!st_map->uvalue || !st_map->links) {
__bpf_struct_ops_map_free(map);
return ERR_PTR(-ENOMEM);
}
return map;
}这些改进可以使代码更加健壮，错误处理更加完善，同时保持代码的可读性和可维护性。 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
This PR backports a kernel patch from mainline v6.7-rc1 that adds proper module memory accounting for BPF struct_ops trampolines. Currently, modmem is charged for regular trampolines but not for struct_ops trampolines, creating an accounting inconsistency.
Key changes:
- Adds
bpf_jit_charge_modmem()call during struct_ops map allocation to account for trampoline memory - Adds
bpf_jit_uncharge_modmem()call during map cleanup to properly release the memory charge
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| st_map->image = bpf_jit_alloc_exec(PAGE_SIZE); | ||
| if (!st_map->image) { | ||
| /* __bpf_struct_ops_map_free() uses st_map->image as flag | ||
| * for "charged or not". In this case, we need to unchange |
There was a problem hiding this comment.
Corrected spelling of 'unchange' to 'uncharge'.
Suggested change
| * for "charged or not". In this case, we need to unchange | |
| * for "charged or not". In this case, we need to uncharge |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
3 participants
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.
mainline inclusion
from mainline-v6.7-rc1
category: feature
Current code charges modmem for regular trampoline, but not for struct_ops trampoline. Add bpf_jit_[charge|uncharge]_modmem() to struct_ops so the trampoline is charged in both cases.
Link: https://lore.kernel.org/r/20230914222542.2986059-1-song@kernel.org
(cherry picked from commit 5c04433)
Summary by Sourcery
Account for module memory usage in BPF struct_ops JIT trampolines by adding modmem charge and uncharge calls during allocation and free
New Features:
Enhancements: