Skip to content

fix(so): reuse pinned dkapture bpf objects correctly - #97

Open
yuKing123-king wants to merge 1 commit into
DKapture:mainfrom
yuKing123-king:fix/bpf-manager_reuse_pinError
Open

yuKing123-king wants to merge 1 commit into
DKapture:mainfrom
yuKing123-king:fix/bpf-manager_reuse_pinError

Conversation

@yuKing123-king

Copy link
Copy Markdown
Contributor

No description provided.

Signed-off-by: Wang Yu <wangyu6@uniontech.com>
Comment thread so/bpf-manager.cpp
struct bpf_map_info info = {};
u32 len = sizeof(info);
std::string map_pin_path(PIN_PATH "/map-");
std::string map_pin_path(PIN_PATH "/");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

为什么改这

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

因为在bpf-manager.cpp:207中bpf_object__pin_maps(m_obj->obj, PIN_PATH);这个函数会调用 libbpf,把当前 BPF object 里的所有 maps pin 到 PIN_PATH 下。但是libbpf 默认会用 map 自己的名字作为最后一级文件名,而没有map-这个前缀。以map-dk_shared_mem 这个map为例, 会导致

  1. 存在性判断永远看错地方,代码以为 /sys/fs/bpf/dkapture/map-dk_shared_mem 不存在,于是误判“需要新建”。
  2. 实际创建时又去创建真正的 dk_shared_mem, 但这个真实 pin 已经存在,于是 libbpf 报: failed to pin map: File exists
  3. 并且当前仓库里没有把 map pin 成 map-xxx 的定义

@dkapture-ci-bot

Copy link
Copy Markdown
Contributor

你好,这是对 libdkapture#97 fix(so): reuse pinned dkapture bpf objects correctly 的评审。

本轮为 DKapture 两仓(libdkapture / dkapture-bpf)全部以 fix 开头 open PR 的批量评审,共 17 个;汇总表如下,本 PR 加粗。逐项详评见分隔线下方。

PR 标题 作者 规模 结论 主要风险
#149 fix(net-traffic): initialize rules before loadin yuKing123-king +19/-9 可合并 规则安装顺序与其它工具约定不一致;本仓改动无正确性缺陷
#147 fix(net-filter): abort rule loading on invalid c yuKing123-king +9/-3 需修改 失败时 clear_rules() 波及 add_rule() 装入的规则;空白注释行导致整个加载失败
#141 fix(dkapture): honor parsed pid in read(vector<p JoeSergen +23/-2 需修改 unsafe_find 单记录语义使 /proc//fd 只回调一次;返回值语义漂移
#140 fix(dkapture): return bytes read, not remaining JoeSergen +10/-4 可合并 无阻塞风险;与文档契约对齐,仓内无调用方依赖旧语义
#114 fix: replace manual destructor calls in construc JoeSergen +28/-7 需修改 捆绑了与 open PR #112 逐字节相同的 fs_watch 修复,跨 PR 重复
#125 fix(lsof): start ringbuf consumer before iterato yuKing123-king +17/-3 需修改 线程启动后错误路径仍 goto err_out → 释放运行中 rb(UAF)+ 线程泄漏
#112 fix: fs_watch calls trace_file_init instead of m JoeSergen +1/-1 可合并 无;仅修一处复制粘贴错误
#103 fix: make power-snoop internal symbols static fo yuKing123-king +3/-2 可合并 改动无害;但 PR 描述的 multiple definition 在当前构建配置下无法复现
#119 fix(syscall-stat): stop skipping syscall key 0 d yuKing123-king +18/-6 可合并 循环终止四条路径已逐一核实;缩进与提交拆分小问题
#115 fix(syscall-stat): improve builtin flow yuKing123-king +204/-41 需修改 三处 bpf_get_map_fd 错误路径未设 ret,最终 return ret 误报成功
#117 fix(trace-signal): validate invalid command line yuKing123-king +190/-29 需修改 BUILTIN 测试入口未接入 test/Makefile,不可达;register_signal 残留
#102 fix(trace-exec): reject invalid command line arg yuKing123-king +135/-22 需修改 BUILTIN 入口同样未接入构建;-h 退出码 0→1 属未说明的行为变更
#97 fix(so): reuse pinned dkapture bpf objects corre yuKing123-king +1/-1 需修改 test mock 仍按 map- 前缀命名 pin,合并后 gtest FindMap 用例失败
#35 fix(pagefault): add max_entries and value_size f yuKing123-king +7/-3 可合并 修复真实,但已被 main 上等效修复 1e90486 取代,建议确认后关闭
#32 fix(peek-fd): correct args field name from mvlen yuKing123-king +1/-1 可合并 无功能风险;标题/描述与实际改动方向不符
#30 fix(trace-signal): avoid inflight event key coll yuKing123-king +36/-22 需修改 sys_exit_kill 的 !rule 早退路径仍泄漏 inflight 条目
#29 fix(syscall-stat): replace exec fexit with kprob yuKing123-king +93/-13 阻塞 exec 路径从 struct filename* 本身读字符串,-f 过滤将完全失效

本 PR 评审详情

作者: yuKing123-king | 规模: +1/-1 | 文件: 1(so/bpf-manager.cpp)
结论: 需修改
主要风险: 生产侧单行修改方向正确,但 mock 与单测仍按旧的 map- 前缀命名 pin 文件,合并后 gtest 的 FindMap 用例必然失败,且被修复的复用路径在 CI 中从此完全不被执行。

总体结论: 该 PR 将 bpf_find_map 的存在性检查从 PIN_PATH "/map-" 改为 PIN_PATH "/"。改动本身正确且必要:构造函数在 bpf-manager.cpp:207 用 libbpf 的 bpf_object__pin_maps(obj, PIN_PATH) pin map,libbpf 默认以 map 自身名字(如 dk_shared_mem)作为末级文件名,不加 map- 前缀;而全仓库(grep 验证)没有任何生产代码创建 map- 前缀的 pin,因此旧检查永远 ENOENT,生产环境复用分支是死代码——第二个实例会重新 open_and_load 并在 bpf_object__pin_maps 处 EEXIST 抛异常,'复用 pinned bpf objects' 特性实际从未生效。此修复让检查路径与真实 pin 布局一致,作者在行级评论中对机制的解说与 libbpf 语义及源码一致。但改动只修了三处 pin 命名约定之一:test/mock.cpp:358 仍以 map-<name> 创建文件(且 mock 的 bpf_object__pin_maps 是空 stub,mock.cpp:273-276,access 未被 mock),导致 gtest 下 access(PIN_PATH/dk_shared_mem) 失败、bpf_find_map 提前返回 -ENOENT,test/bpf-test.cpp:122-123 的 ASSERT_GT(ret,0) 必然失败;同时构造函数在 mock 下永远走创建分支,本次修复所启用的复用路径从此没有任何测试覆盖。更好的做法是:同一 PR 内把 mock 与相关断言同步改为无前缀命名(与真实 libbpf 对齐),并考虑删掉 access() 快速门(bpf_find_map 内本就有按 map name 全量扫描的兜底路径,不依赖 pin 文件名),避免同一 pin 布局在生产代码、libbpf、mock 三处各写一份。

主要问题:

  • (P1) test/mock.cpp:358 — mock 的 pin 命名与生产 libbpf 不一致,改动后 gtest 断链。生产侧 pin 实际由 so/bpf-manager.cpp:207 的 bpf_object__pin_maps(obj, PIN_PATH) 完成,libbpf 按 map 原名 pin 到 PIN_PATH/;但 mock 在 open_skeleton 里以 snprintf(buf, PATH_MAX, "%s/map-%s", BPF_PIN_PATH, name) 创建文件(其 bpf_object__pin_maps stub 直接返回 0,mock.cpp:273-276,access() 未被 mock)。PR 改后 bpf_find_map 检查 PIN_PATH "/dk_shared_mem"(so/bpf-manager.cpp:31,(新)),mock 环境下该文件不存在 → 提前 return -ENOENT → test/bpf-test.cpp:122-123 ASSERT_GT(ret, 0) 失败。且 BPFManager 构造函数(so/bpf-manager.cpp:183-184)从此在 gtest 下永远走 'creating new bpf mirror' 分支,复用路径零覆盖。修法:mock.cpp:358 改为 "%s/%s",并同步更新 test/dkapture-test.cpp:118、124、130 对 map-dk_shared_mem 的断言(真实 libbpf 从不产生该文件名,现行断言本身就是在固化 mock 的虚构布局)。
  • (P2) so/bpf-manager.cpp:31 — 修复让生产环境首次真正走进复用分支,但该分支此前在生产中是死代码,其健壮性未经生产验证:复用判定仅凭 access(PIN_PATH/dk_shared_mem)bpf_find_iter("dump_task") 两个文件存在性(so/bpf-manager.cpp:183-192);若上次消费者异常退出(SIGKILL 不走析构清理)留下陈旧 pin 而 kernel map 已释放,access 通过但 id 扫描找不到 → 返回 -ENOENT → 走创建分支 → bpf_object__pin_maps EEXIST → 构造函数抛异常,工具不可用直到人工清理 /sys/fs/bpf/dkapture。建议 PR 内至少处理陈旧 pin 场景(扫描失败时 unlink 陈旧 pin 后重试一次),或明确说明依赖 shm refcnt/人工清理的运维约定。

次要建议:

  • so/bpf-manager.cpp:31-36 — access() 快速门冗余且引入第三份 pin 布局知识(生产 libbpf、mock、此处各一份)。bpf_find_map 内已有的 bpf_map_get_next_id/fd_by_id/get_info_by_fd 按 name 扫描(so/bpf-manager.cpp:37-70)不依赖 pin 文件名,可直接删掉 access 门统一走扫描;或改用 PinRegistry::lookup()(so/pin-registry.h:32-34,本就是为 pin 路径精确查找设计的)替代字符串前缀拼接。
  • 提交信息: 'fix(so): reuse pinned dkapture bpf objects correctly' 符合 fix(scope) 规范、单一职责;但 commit body 仅含 Signed-off-by,未说明为何改(本次评审线程中被迫口头解释 libbpf pin 命名机制),建议把该解释写入 commit body。

亮点:

  • 作者在行级评论中对失败机理的三点归因(存在性判断看错位置、真实 pin 已存在导致 EEXIST、仓库无 map- 前缀 pin 定义)逐条与源码核实无误,说明对 libbpf pin 语义理解准确。
  • 修改最小且聚焦,未夹带无关改动。

已有讨论: 2 条行级评论。MEMBER 在 r3619802561 问'为什么改这';作者在 r3620572517 回复解释:bpf-manager.cpp:207 的 bpf_object__pin_maps 会把所有 map pin 到 PIN_PATH 下且以 map 原名命名(无 map- 前缀),导致 1) 存在性判断永远失败误判需新建;2) 重建时真实 pin 已存在,libbpf 报 failed to pin map: File exists;3) 仓库中无任何把 map pin 成 map-xxx 的代码。本评审核实该解释成立。无其他未决讨论,无 issue 评论。

证据核对:pin 创建点 so/bpf-manager.cpp:207(bpf_object__pin_maps);mock 命名 test/mock.cpp:358;mock stub test/mock.cpp:273-276;受影响断言 test/bpf-test.cpp:122-127、test/dkapture-test.cpp:118/124/130;全仓 grep 确认无生产代码创建 map- 前缀 pin、access() 未被 mock。静态评审,未运行构建/测试(BPF 无法本地编译),未访问网络。

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants