Skip to content

fix: backport device mode and system-wide conf#1663

Merged
dengbo11 merged 1 commit into
OpenAtom-Linyaps:release/1.12from
reddevillg:fix_dev
May 18, 2026
Merged

fix: backport device mode and system-wide conf#1663
dengbo11 merged 1 commit into
OpenAtom-Linyaps:release/1.12from
reddevillg:fix_dev

Conversation

@reddevillg

Copy link
Copy Markdown
Collaborator

ca6a44e feat: add device passthru mode
c7f3a69 feat: support system-wide runtime configuration

@reddevillg reddevillg requested a review from dengbo11 May 15, 2026 10:16

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request introduces a "device passthrough" mode for the container runtime, allowing the entire host /dev directory to be bind-mounted into the sandbox. Key changes include updating the API schema to include a device_mode option, adding a --device-mode flag to the CLI, and refactoring the configuration loading logic to support system-wide configurations in addition to user-specific ones. The OCI configuration builder was also updated to handle the passthrough logic and ensure /run/udev is mounted. Review feedback identifies a logic error in bindDevNode that could lead to incorrect device node mounting when passthrough is enabled, as well as a potential type mismatch in the refactored configuration loading functions that might cause compilation issues.

Comment on lines 269 to 272
if (!devPassthru && devNodeMount) {
devNodeMount->emplace_back(std::move(m));
} else {
devNodeMount = { std::move(m) };

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.

high

The logic in bindDevNode is incorrect when devPassthru is true. The condition !devPassthru && devNodeMount will always be false, causing devNodeMount to be reset to a single-element vector on every iteration of the loop, effectively only keeping the last matching device node. If the intention is to skip individual node mounts when passthrough is enabled (since the entire /dev is already bind-mounted), the logic should be corrected to avoid overwriting the list.

            if (!devPassthru) {
                if (devNodeMount) {
                    devNodeMount->emplace_back(std::move(m));
                } else {
                    devNodeMount = { std::move(m) };
                }
            }

auto result = linglong::utils::loadRuntimeConfig(configPath);
if (!result) {
return LINGLONG_ERR(result);
return linglong::utils::loadRuntimeConfig(configPath);

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.

medium

Potential type mismatch in return statement. linglong::utils::loadRuntimeConfig(configPath) returns Result<RuntimeConfigure>, but the function loadRuntimeConfigFromDir is declared to return Result<std::optional<RuntimeConfigure>>. Unless the Result template supports implicit conversion between these types, this may cause a compilation error. It is safer to explicitly handle the result as done in the previous implementation.

    auto result = linglong::utils::loadRuntimeConfig(configPath);
    if (!result) {
        return LINGLONG_ERR(result);
    }
    return std::move(result).value();

@codecov

codecov Bot commented May 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 4.83871% with 59 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
libs/utils/src/linglong/utils/runtime_config.cpp 10.00% 25 Missing and 2 partials ⚠️
libs/linglong/src/linglong/cli/cli.cpp 0.00% 13 Missing ⚠️
...glong/oci-cfg-generators/container_cfg_builder.cpp 0.00% 12 Missing ⚠️
apps/ll-cli/src/main.cpp 0.00% 5 Missing ⚠️
libs/common/src/linglong/common/dir.cpp 0.00% 2 Missing ⚠️
Files with missing lines Coverage Δ
libs/linglong/src/linglong/cli/cli.h 8.33% <ø> (ø)
...inglong/oci-cfg-generators/container_cfg_builder.h 0.00% <ø> (ø)
libs/common/src/linglong/common/dir.cpp 27.77% <0.00%> (-3.48%) ⬇️
apps/ll-cli/src/main.cpp 0.00% <0.00%> (ø)
...glong/oci-cfg-generators/container_cfg_builder.cpp 0.00% <0.00%> (ø)
libs/linglong/src/linglong/cli/cli.cpp 1.75% <0.00%> (-0.02%) ⬇️
libs/utils/src/linglong/utils/runtime_config.cpp 23.61% <10.00%> (-1.39%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ca6a44e feat: add device passthru mode
c7f3a69 feat: support system-wide runtime configuration

Signed-off-by: reddevillg <reddevillg@gmail.com>
@deepin-ci-robot

Copy link
Copy Markdown
Collaborator

deepin pr auto review

这份代码变更主要实现了为沙箱/容器运行环境添加 device_mode(特别是 passthru 直通模式)的支持,并重构了运行时配置的加载逻辑(增加了系统级配置加载),同时更新了相关的 JSON Schema、CLI 参数和 OCI 容器配置生成器。

以下是对该 diff 的详细代码审查意见,分为语法逻辑、代码质量、代码性能和代码安全四个方面:

1. 语法与逻辑

  • deviceMode 合并逻辑存在重复项风险
    libs/utils/src/linglong/utils/runtime_config.cppMergeRuntimeConfig 函数中,deviceMode 的合并采用的是直接 insert 追加方式。如果系统配置和用户配置中都包含了 Passthru,合并后的数组中会出现两个 Passthru
    虽然在 cli.cpp 中判断 devicePassthru 时使用了 break,不影响最终逻辑,但在序列化回 JSON 或其他消费方使用时,可能会产生脏数据。
    建议:考虑在合并时去重,或者改用 std::set / std::unordered_set 来存储 DeviceOption

  • loadRuntimeConfig 返回值语义变化
    旧代码中 loadRuntimeConfig 返回 std::move(result).value(),如果解析失败会抛出异常;新代码直接返回 result。这改变了错误处理的行为,从抛出异常变为了返回包含错误的 Result 对象。逻辑上与 LINGLONG_ERR 宏配合是正确的,但需确认所有调用方都做好了 Result 的错误检查,而不是依赖捕获异常。

  • CLI 参数 --device-mode 缺少必填校验
    cliRun->add_option("--device-mode", ...) 允许传入空值或非法值(尽管有 CheckedTransformer),但如果用户输入了不在 deviceOptionMap 中的值,CLI11 默认可能会抛出异常或静默忽略,建议显式设置 ->check(CLI::ExistingItem) 或确保 CheckedTransformer 的失败行为符合预期(如直接报错退出)。

2. 代码质量

  • deviceOptionMap 生命周期与硬编码问题
    apps/ll-cli/src/main.cpp 中,deviceOptionMap 被定义为局部的 const std::map。由于 DeviceOption 是由 Schema 自动生成的枚举,每次 Schema 新增枚举值,这里都需要手动同步维护,容易遗漏。
    建议:可以考虑在 DeviceOption.hpp 的同级别目录下自动生成一个 deviceOptionMap 或者通过 C++ 的魔法枚举(如 magic_enum,如果项目中有引入)来动态映射,避免手动维护映射表。

  • 命名一致性
    Schema 和 C++ 结构体中使用的是 deviceMode,而 CLI 参数、枚举定义和变量名使用的是 deviceOption / deviceOptions。虽然一个是“模式”,一个是“选项”,但在当前上下文语义高度重合,混用会降低代码可读性。
    建议:统一命名,例如在 C++ 和 Schema 中都使用 deviceOptions,或者 CLI 也使用 --device-mode 对应 deviceModes

  • 代码重构与复用
    runtime_config.cpp 中提取了 loadRuntimeConfigFromDir 函数,消除了系统级和用户级加载的重复代码,这是一个很好的重构。

  • bindDev 参数默认值
    ContainerCfgBuilder::bindDev(bool passthru = false) 给了默认参数,这保持了向后兼容,设计良好。

3. 代码性能

  • std::map 查找性能
    main.cpp 中的 deviceOptionMap 使用了 std::map,对于只有 1 个或极少数键值对的映射,std::map(红黑树)的内存开销和查找效率不如 std::unordered_map 或简单的 if-else / std::array
    建议:由于目前只有 passthru 一项,影响微乎其微,但如果后续不会大量扩充,可考虑使用无序容器或在 CLI::Transformer 中使用初始化列表。

  • MergeRuntimeConfig 中的频繁拷贝
    在合并 envextDefs 时,使用了 result.env->insert(config.env->begin(), config.env->end()),对于 std::map,这种插入伴随着键值对的拷贝。如果配置项很多,可能会有性能损耗。
    建议:如果配置对象支持移动语义,且后续不再使用 config,可以考虑移动合并(如 merge 或遍历 make_move_iterator)。

4. 代码安全

  • 🔴 严重:passthru 模式的设备直通带来巨大的安全风险
    container_cfg_builder.cpp 中,当 passthru = true 时,直接将宿主机的 /devrbind 的方式挂载进容器:

    Mount{ .destination = "/dev",
           .options = string_list{ "rbind" },
           .source = "/dev",
           .type = "bind" },

    这意味着容器内的进程可以访问宿主机的所有设备节点(包括磁盘 /dev/sda、内存 /dev/mem、内核设备 /dev/kmsg 等)。这实际上打破了容器的隔离性,等同于赋予了容器近乎宿主机 root 的权限(如果容器内有 root 权限,可以直接读写磁盘或加载内核模块)。
    建议

    1. 必须在文档和 CLI 帮助信息中明确警告:--device-mode=passthru 会带来极高的安全风险,仅应在受信的调试场景下使用。
    2. 考虑是否需要配合 privileged(特权模式)一起使用才允许开启此选项。
    3. 如果只是为了透传特定设备(如 GPU、USB),更安全的做法是按需绑定特定设备(如 /dev/dri),而不是将整个 /dev 暴露给容器。
  • /run/udev 的挂载风险
    新增了 bindIfExist(*runMount, "/run/udev");/run/udev 包含了 udev 守护进程的状态和规则信息。将其挂载进容器可能会让容器内的进程感知到宿主机的硬件变更事件,甚至在某些特定配置下干扰宿主机的 udev 规则。
    建议:确认挂载 /run/udev 的必要性。如果只是为了 passthru 模式下让 udev 规则在容器内生效,建议仅在 passthru 为 true 时才挂载,而不是默认无条件挂载。

  • 系统级配置的提权风险
    新增了 loadSystemRuntimeConfig,它会读取系统级别的配置文件(如 /etc/linglong/config.json)。如果系统管理员在此文件中配置了 passthru,所有用户的沙箱都将默认以直通模式运行,这可能导致普通用户无意间获得操作宿主机硬件的权限。
    建议:对于影响安全的敏感选项(如 passthru),在合并配置时,应考虑权限降级逻辑:即使用户在用户级配置中开启了 passthru,如果系统级配置明确禁止,也不应生效;或者要求开启 passthru 必须通过命令行显式传入,而不应仅通过配置文件静默生效。

总结

代码整体结构清晰,重构合理。最核心的问题在于安全层面passthru 将整个 /dev 暴露给容器的行为非常危险,必须增加安全约束或告警机制。同时,建议在配置合并时处理 deviceMode 的重复项问题,并审视 /run/udev 默认挂载的必要性。

@deepin-ci-robot

Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dengbo11, reddevillg

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@dengbo11 dengbo11 merged commit 7d4ce45 into OpenAtom-Linyaps:release/1.12 May 18, 2026
16 of 17 checks passed
@reddevillg reddevillg deleted the fix_dev branch May 18, 2026 01:43
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