Skip to content

feat: add DDE grouped VT control plane - #2

Merged
zccrs merged 3 commits into
linuxdeepin:dde/0.9.3from
zccrs:backport/dde-0.9.3-grouped-vt
Jun 2, 2026
Merged

zccrs merged 3 commits into
linuxdeepin:dde/0.9.3from
zccrs:backport/dde-0.9.3-grouped-vt

Conversation

@zccrs

@zccrs zccrs commented May 30, 2026

Copy link
Copy Markdown
Member

Summary

  • add a DDE control socket for grouped VT registration and notifications
  • keep seatd VT-bound while allowing grouped internal VT switches
  • retain dde-seatd fork packaging/runtime naming and document the new socket args

Test

  • meson compile -C build-grouped
  • meson test -C build-grouped
  • reuse lint

@github-actions

Copy link
Copy Markdown

TAG Bot

TAG: 0.9.3
EXISTED: yes
DISTRIBUTION: unstable

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot reviewed 38 out of 38 changed files in this pull request and generated 6 comments.

Comment thread seatd/seatd.c Resolved
Comment thread seatd/control.c Resolved
Comment thread seatd/server.c Resolved
Comment thread seatd/server.c Resolved
Comment thread seatd/seatd.c Resolved
Comment thread seatd/control.c Resolved
@zccrs
zccrs force-pushed the backport/dde-0.9.3-grouped-vt branch 2 times, most recently from b8e963b to ee9f135 Compare May 30, 2026 10:02
@zccrs
zccrs requested a review from Copilot June 1, 2026 03:55

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.

Comment thread seatd/seatd.c Resolved
Comment thread seatd/seat.c Resolved
Comment thread seatd/seatd.c Resolved
Comment thread seatd/seat.c Resolved
Comment thread seatd/control.c Resolved
Comment thread seatd/server.c Resolved
Comment thread common/terminal.c Resolved
Comment thread include/control.h Resolved
Comment thread seatd/control.c Resolved
@zccrs
zccrs force-pushed the backport/dde-0.9.3-grouped-vt branch 4 times, most recently from 008dc40 to a7bbdd8 Compare June 1, 2026 06:24
vt_close is the cleanup counterpart to vt_open. vt_open correctly
sets VT_PROCESS mode so that seatd can manage VT switching via signals.
However, vt_close was also setting VT_PROCESS (passing true to
terminal_set_process_switching), leaving the VT controlled by seatd for
no reason.

Pass false to restore VT_AUTO mode, letting the kernel manage VT
switching again after seatd releases it.

Signed-off-by: Quentin THÉBAULT <quentin.thebault@defenso.fr>
(cherry picked from commit 4bcdc81)
@zccrs
zccrs force-pushed the backport/dde-0.9.3-grouped-vt branch from a7bbdd8 to bf743d9 Compare June 1, 2026 06:55
@github-actions

github-actions Bot commented Jun 1, 2026 •

Copy link
Copy Markdown

CLA Assistant Lite bot:

如果你是以企业贡献者的身份进行提交,请联系我们签署企业贡献者许可协议
If you submit as corporate contributor, please contact us to sign our Corporate Contributor License Agreement

感谢您的提交,我们非常感谢。 像许多开源项目一样,在接受您的贡献之前,我们要求您签署我们的个人贡献者许可协议。 您只需发布与以下格式相同的评论即可签署个人贡献者许可协议。
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Individual Contributor License Agreement before we can accept your contribution. You can sign the Individual Contributor License Agreement by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA.

JiDe Zhang seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request

@zccrs
zccrs force-pushed the backport/dde-0.9.3-grouped-vt branch 13 times, most recently from 435d025 to f234b0f Compare June 2, 2026 02:40
@zccrs
zccrs force-pushed the backport/dde-0.9.3-grouped-vt branch from f234b0f to b774a20 Compare June 2, 2026 03:00
@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

这份 diff 为 dde-seatd 引入了非常重要的功能:DDE 控制套接字、VT 分组以及基于内核事件的 VT 切换感知。整体架构设计清晰,代码风格与上游 seatd 保持了一致。

但在语法逻辑、并发安全、代码性能和安全性方面,存在一些需要重点关注和改进的地方。以下是详细的审查意见:

1. 语法与逻辑

  • DRM 权限判断逻辑存在隐患 (common/drm.c)
    drm_is_master 函数通过尝试 ioctl(DRM_IOCTL_AUTH_MAGIC) 来判断是否是 Master,但这依赖内核对特定错误码的返回。更严重的是,如果当前进程是 DRM Master,且 magic = 0,某些内核版本可能会将其视作一次无效的鉴权尝试并返回 EINVAL,甚至可能在极端安全配置下影响会话状态。
    建议:判断是否为 DRM Master 的标准且安全做法是检查设备节点的权限位(O_RDWR),或者尝试 drmSetMaster/drmDropMaster。在 seatd 的上下文中,最稳妥的做法是:不要在激活设备时去“查询”状态,而是直接尝试 drm_set_master,如果返回 EPERM/EACCES 则说明已经是 Master 或无权限,此时将 active 设为 true 即可。 这样既安全又避免了魔数判断。

  • VT 事件读取逻辑可能丢失事件 (seatd/server.c)
    在 server_handle_vt_event 中:

    while (read(fd, &msg, sizeof(msg)) == (ssize_t)sizeof(msg)) { ... }

    如果 read 返回 -1 且 errno == EINTR(被信号中断),循环会直接退出,导致管道中残留的数据只能在下一次 epoll 唤醒时处理。由于 VT 切换事件有严格的时序依赖,这可能导致状态机卡死。
    建议:处理 EINTR,并在必要时通过 ioctl(VT_GETSTATE) 主动修正当前 VT 状态:

    while (true) {
        ssize_t n = read(fd, &msg, sizeof(msg));
        if (n == (ssize_t)sizeof(msg)) {
            // 处理 msg
        } else if (n == -1) {
            if (errno == EINTR) continue;
            if (errno != EAGAIN) return -1;
            break;
        } else {
            break; // EAGAIN
        }
    }
  • Control Socket 的 UID 校验过于严格 (seatd/control.c)
    在 control_handle_connection 中,if (uid != geteuid()) 限制了只有 root 用户(因为 dde-seatd 以 root 或 dde 用户运行)才能连接控制套接字。如果 DDM 未来以非 root 特权用户运行,将无法连接。
    建议:如果设计意图是仅限本地特权进程,建议改为校验 uid == 0 (root) 或者与命令行 -u 传入的用户匹配,而不是单纯的 geteuid()。

2. 并发与线程安全

  • pthread_cancel 的使用极其危险 (seatd/server.c)
    在 server_finish 中使用了 pthread_cancel。server_vt_event_loop 线程可能被取消在 write() 等系统调用中,导致管道处于不一致状态,或者持有的锁(如果有)未释放,造成死锁。
    建议:绝对不要使用 pthread_cancel。你已经使用了 atomic_bool vt_event_running,应该利用它来优雅退出:

    atomic_store(&server->vt_event_running, false);
    // 发送一个无效事件或关闭 vt_event_fd 来解除 VT_WAITEVENT 的阻塞
    close(server->vt_event_fd); 
    server->vt_event_fd = -1;
    pthread_join(server->vt_event_thread, NULL);
  • VT 事件线程与主线程的数据竞争
    server_vt_event_loop 在后台线程读取 server->vt_event_fd,而主线程在 server_finish 中会 close(server->vt_event_fd)。如果关闭时机不当,后台线程可能会在 ioctl 时收到 EBADF。
    建议:如上所述,通过主动关闭 vt_event_fd 来唤醒线程,线程在 ioctl 返回错误后检查 vt_event_running,然后自然退出。

  • 链表遍历缺乏并发保护
    control_find_owner_client 遍历了 server->seats 和 seat->clients。虽然目前 control 客户端的回调是在主线程的 poller 中执行的,看似安全,但如果未来有任何信号处理或其它线程逻辑介入,将会引发 UAF 或崩溃。
    建议:在注释中明确强调 control_handle_connection 及其调用链必须仅在主 poller 线程中执行。

3. 代码性能

  • 字符串格式化函数的滥用 (seatd/seat.c)
    describe_vt_owner 和 describe_pending_vt_target 在每次 VT 状态变更时都会被调用,内部使用了多个 snprintf。虽然对性能影响微乎其微,但在内核态 VT 切换这种对延迟敏感的路径上,不必要的字符串格式化是不推荐的。
    建议:将日志记录改为直接传递结构化参数,仅在确实需要打印日志时进行格式化:

    log_infof("VT change on %s: %d -> %d (group: %d, client: %d)", 
              seat->seat_name, old_vt, new_vt, group_owner_pid, client_pid);
  • DRM Master 恢复逻辑 (seat_restore_client_drm_master)
    在恢复 DRM master 时,代码遍历了客户端的所有设备。这在设备较多时是不必要的。
    建议:在 seat_device 结构中增加一个 is_drm 的布尔值(或在打开设备时缓存 type == SEAT_DEVICE_TYPE_DRM),遍历时直接判断布尔值,避免频繁访问结构体成员判断枚举类型。

4. 代码安全

  • Control Socket 权限过于开放 (seatd/seatd.c)
    open_control_socket 将套接字权限设置为 0600,这是好的。但是,dde-seatd.service 中指定的路径是 /run/dde-seatd-control.sock。在某些 Linux 发行版中,/run 可能会被全局可读的 tmpfs 覆盖。
    建议:考虑将 control socket 放在受限制的目录中,如 /run/dde-seatd/,并确保该目录的权限为 0700,属主为 root/dde。

  • PID 重用攻击
    control_find_owner_client 和 seat_get_group_vt_owner_pid 依赖 PID 来鉴权和查找客户端。在 Linux 系统中,PID 是会被复用的。如果 DDM 传递了一个已经退出的 compositor 的 PID,而该 PID 恰好被另一个无关进程复用,可能会导致权限泄露或逻辑混乱。
    建议:

    1. 在 handle_create_group_vt 中,找到 owner 后,除了比较 PID,必须增加对 UID 的校验,确保拥有该 PID 的进程与发起 control 请求的进程属于同一用户。
    2. 考虑让 compositor(libseat 客户端)在打开 seat 时,由 dde-seatd 主动为其分配并绑定 group ID,而不是完全依赖 DDM 事后通过 PID 绑定,这样可以从架构上消除 PID 复用的风险。
  • 请求体中的缓冲区截断 (seatd/control.c)
    request.user[sizeof(request.user) - 1] = '\0'; 这种做法虽然保证了字符串终结,但如果发生截断,相当于静默修改了用户的请求。
    建议:如果发生截断(即原字符串长度 >= CONTROL_NAME_MAX),应该返回 EINVAL 或 E2BIG 错误给客户端,而不是静默接受截断后的数据。

5. Debian 打包细节

  • debian/tests/dde-seatd-launch 测试脚本:
    [ -S /run/dde-seatd.sock ] && service dde-seatd stop || true
    SEATD_LOGLEVEL=debug dde-seatd-launch -- sh -c 'printf "dde-seatd PID: %d\n" "$(pidof dde-seatd)"'
    在 CI 环境(如 autopkgtest)中,service dde-seatd stop 可能会失败,因为系统可能没有启动 systemd/sysvinit。此外,dde-seatd-launch 需要特权(操作 VT 和 DRM),确保测试控制文件声明了 needs-root(你已经声明了,这很好)。
    建议:改为更健壮的停止方式,或者直接在测试中手动启动 dde-seatd 而不依赖系统服务,以隔离测试环境。

总结

最关键需要修复的是 pthread_cancel 的使用 和 DRM Master 状态判断逻辑,这两个问题极易导致死锁和图形栈崩溃。其次,基于 PID 的鉴权机制需要加入 UID 校验以防止 PID 重用带来的安全隐患。并发模型虽然目前勉强可行,但需要通过注释和文档严格约束,避免后续维护者破坏单线程 poller 的假设。

@zccrs
zccrs force-pushed the backport/dde-0.9.3-grouped-vt branch from b774a20 to 41a1401 Compare June 2, 2026 03:22
@linuxdeepin linuxdeepin deleted a comment from zccrs Jun 2, 2026
@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

这是一份针对 dde-seatd 分支代码变更的详细审查报告。此次变更主要引入了 DDE 控制套接字、分组 VT(Grouped VT)管理、VT 事件监听线程以及相关的 Debian 打包配置。

整体来看,代码结构清晰,对上游的改动尽量做到了隔离。但在并发安全、资源管理、逻辑严谨性和系统编程规范上存在一些需要重点关注和改进的地方。


一、 语法与逻辑

1. drm_is_master 的判定逻辑存在隐患 (common/drm.c)

  • 问题:使用 ioctl(DRM_IOCTL_AUTH_MAGIC) 来判断是否为 DRM Master。当传入 magic = 0 时:
    • 如果当前进程是 Master,内核会返回 EINVAL(因为 0 不是有效的 magic)。
    • 如果不是 Master,内核会返回 EACCES 或 EPERM。
    • 但如果传入了一个恰好正在被其他进程使用的真实 magic,内核可能会返回 EACCES(鉴权失败),这会被误判为“当前进程不是 Master”。
  • 改进:在用户态,判断 DRM Master 最标准、最安全的方式是尝试 ioctl(fd, DRM_IOCTL_SET_MASTER)。如果当前已经是 Master,该 ioctl 会成功返回 0;如果不是 Master,则会失败并返回 EPERM/EACCES。如果已经是 Master,再 Drop 一次即可恢复原状。
    int drm_is_master(int fd) {
        // 尝试 SET_MASTER:如果已经是 Master 会成功,不是 Master 会返回 EACCES/EPERM
        if (ioctl(fd, DRM_IOCTL_SET_MASTER, 0) == 0) {
            // 成功说明我们现在是 Master(或者刚获取了 Master)
            // 为了保持状态不变,立刻 Drop 掉
            ioctl(fd, DRM_IOCTL_DROP_MASTER, 0);
            return 1;
        }
        if (errno == EACCES || errno == EPERM) {
            return 0;
        }
        return -1;
    }

2. seat_activate_device 中的逻辑冗余与状态污染 (seatd/seat.c)

  • 问题:在 drm_is_master 返回 1(已经是 Master)时,代码直接设置了 seat_device->active = true 并 break。但在 default 分支中,即使 drm_set_master 失败,代码依然强制设置 seat_device->active = true。这会导致设备实际未获取权限,但 seatd 认为其已激活,后续将引发严重的图形栈崩溃。
  • 改进:如果 drm_set_master 失败,不应标记为 active。
    case SEAT_DEVICE_TYPE_DRM:
        switch (drm_is_master(seat_device->fd)) {
        case 1:
            seat_device->active = true;
            break;
        case 0:
        default: // 0 或判断失败,都尝试 set_master
            if (drm_set_master(seat_device->fd) == -1) {
                log_errorf("Could not make device fd drm master: %s", strerror(errno));
                seat_device->active = false; // 必须标记为失败
                return -1; // 应当返回错误
            }
            seat_device->active = true;
            break;
        }
        break;

3. seat_vt_release 中丢失了 seat->cur_vt = -1 的赋值 (seatd/seat.c)

  • 问题:原代码在 vt_ack 后有 seat->cur_vt = -1,删除后,当 VT Release 信号发生时,seat->cur_vt 仍然保留旧值。虽然后续有 VT 事件线程更新,但在信号触发到事件到达的窗口期内,seat->cur_vt 是过期的,可能导致逻辑判断异常。
  • 改进:建议恢复 seat->cur_vt = -1; 或将其置为 0,以表明当前无活跃 VT。

二、 代码质量与并发安全

1. 严重:pthread_cancel 的使用及线程清理 (seatd/server.c)

  • 问题:在 server_finish 中使用了 pthread_cancel 来终止 VT 事件线程。pthread_cancel 是极其危险的,如果在 ioctl 或 write 期间被取消,可能导致文件描述符泄漏或内部锁死。此外,如果线程在 write 阻塞时被异步取消,pipe 的状态可能损坏。
  • 改进:使用协作式退出机制。通过关闭 vt_event_fd 来使 ioctl(VT_WAITEVENT) 返回 EBADF 错误从而自然退出线程。
    void server_finish(struct server *server) {
        // ...
        atomic_store(&server->vt_event_running, false);
        if (server->vt_event_fd != -1) {
            close(server->vt_event_fd); // 使 ioctl 失败,唤醒线程
            server->vt_event_fd = -1;
        }
        if (server->vt_event_thread_started) {
            pthread_join(server->vt_event_thread, NULL); // 安全等待线程退出
            server->vt_event_thread_started = false;
        }
        // ...
    }

2. 严重:Pipe 缓冲区满导致死锁 (seatd/server.c)

  • 问题:server_vt_event_loop 中注释写着 "Keep the writer blocking so VT switch events are not dropped"。如果用户疯狂切换 VT(如长按 Ctrl+Alt+Fn),Linux 默认的 pipe 缓冲区(64KB)会被写满。写线程将阻塞在 write 上,而此时它又持有 vt_event_running 的检查,如果主线程此时正等待该线程退出,将产生死锁。
  • 改进:绝对不能在实时/事件线程中阻塞写入。应当使用非阻塞写入,如果写入失败(EAGAIN),说明主线程处理不过来,可以选择丢弃事件或记录警告。
    // 在创建 pipe 时,将写端也设置为 O_NONBLOCK
    // 写入时:
    ssize_t ret = write(server->vt_event_pipe[1], &msg, sizeof(msg));
    if (ret == -1) {
        if (errno == EAGAIN || errno == EWOULDBLOCK) {
            log_debug("VT event pipe full, dropping event");
        } else if (errno != EINTR) {
            break;
        }
    }

3. control_client 的生命周期与并发冲突 (seatd/control.c)

  • 问题:control_broadcast_vt_change 在主循环中被调用,遍历 control_clients。如果发送失败,会调用 control_client_destroy,这会移除当前链表节点。虽然使用了 elem = elem->next 提前保存了下一个节点,但 control_send_vt_change 可能会因为 EAGAIN 返回 -1(非致命错误),此时直接销毁客户端过于严苛。
  • 改进:区分致命错误和暂时的缓冲区满。如果是 EAGAIN,应该只更新 epoll 关注 EVENT_WRITABLE,而不是直接销毁客户端。

4. seat_get_server 的设计破坏了抽象 (seatd/seat.c)

  • 问题:seat_get_server 通过从 seat->clients 链表中取出第一个 client,然后返回 client->server。这假设了 clients 链表一定非空,且依赖了 client 的内部结构。
  • 改进:直接在 struct seat 中增加 struct server *server; 指针,在 seat_create 时传入并赋值。

三、 代码性能

1. seat_find_group_vt 线性查找 (seatd/seat.c)

  • 问题:每次查找分组 VT 都遍历链表,时间复杂度 O(N)。
  • 改进:通常系统中分组 VT 的数量极少(个位数),O(N) 完全可以接受。但如果未来有扩展,可以考虑使用哈希表(以 vt 号为 key)。

2. control_find_owner_client 双重循环遍历 (seatd/control.c)

  • 问题:通过 PID 查找 owner client,需要遍历所有 seat 下的所有 client。
  • 改进:在 struct server 或 struct seat 中维护一个以 pid_t 为键的哈希表映射,将查找时间降为 O(1)。

四、 代码安全

1. Control Socket 的鉴权过于宽松 (seatd/control.c)

  • 问题:control_handle_connection 中仅检查了 uid != geteuid()。这意味着系统上任何以 root 身份运行的进程都能连接并控制 dde-seatd(如注册恶意 VT 组,导致 DRM Master 泄漏)。如果 dde-seatd 以非 root 运行,则需确保只有 DDM 能连接。
  • 改进:除了 UID 检查,建议增加对 PID 对应可执行文件路径的校验,或者通过 Linux Socket 的 SELinux/AppArmor 上下文进行强制访问控制。或者至少验证连接者的 GID 是否符合预期。

2. get_peer 中对 FreeBSD 版本的判断可能失效 (seatd/control.c)

  • 问题:#if __FreeBSD_version >= 1300030 ... 是编译期宏。如果编译在旧版 FreeBSD 上,但在新内核上运行,将无法获取 PID;反之亦然。
  • 改进:在 FreeBSD 上,如果 cr_pid 不可靠,应在运行时通过 procfs 或 sysctl 重新验证,或要求必须在足够新的 FreeBSD 上编译。

3. Debian 打包中的权限问题 (debian/control)

  • 问题:libdde-seat1 依赖 dde-seatd。这对于提供运行时库是合理的,但要注意 dde-seatd 服务默认以 dde 用户运行,而 control socket 权限是 0600 (root only)。如果 DDM 不是以 root 运行,将无法连接 control socket。
  • 改进:确认 DDM 的运行身份。如果 DDM 不以 root 运行,open_control_socket 应当像主 socket 一样,通过 chown 将 socket 文件归属给 DDM 的用户/组。

五、 其他细节改进

  1. _GNU_SOURCE 宏的滥用:seatd/server.c 和 seatd/control.c 顶部定义了 #define _GNU_SOURCE,但代码中并未使用到 GNU 扩展(如 asprintf 等)。如果是历史遗留,建议清理;如果是为了 pthread_cancel 等扩展,建议在 Meson 编译参数中通过 -D_GNU_SOURCE 全局定义,而不是在文件内污染全局宏空间。
  2. Debian init 脚本的兼容性:debian/dde-seatd.init 使用了 /lib/init/init-d-script,这是 Debian 特有的,确保不需要支持其他发行版的 SysV init 规范。
  3. REUSE.toml 版权更新:新增的 README.zh_CN.md 被正确包含,但新增的 C 源文件(seatd/control.c)头部已有 MIT 版权声明,请确保在项目的 REUSE 软件合规性扫描中能被正确识别。

@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: justforlxz, zccrs

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

zccrs added 2 commits June 2, 2026 13:02
Track pending grouped-VT switches inside seatd so VT release can decide
whether to keep or drop DRM master before the kernel completes the switch.
Reject overlapping switch_session requests during a pending VT transition
and skip drm_set_master when the DRM fd is already master.
Refresh Debian packaging for the dde-seatd split package set and keep the
Debian metadata aligned with the grouped-VT backport without introducing a
new libdrm build dependency.
@zccrs
zccrs force-pushed the backport/dde-0.9.3-grouped-vt branch from 41a1401 to 5352569 Compare June 2, 2026 05:05
@linuxdeepin linuxdeepin deleted a comment from zccrs Jun 2, 2026
@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

这份 Git Diff 为 dde-seatd 引入了非常重要的功能扩展,包括 DDE 控制套接字、分组 VT(Grouped VT)管理、VT 切换事件监听线程以及相应的 Debian 打包和文档更新。

整体来看,代码结构清晰,协议设计合理,但在并发安全、资源生命周期管理、系统调用健壮性以及打包规范上存在一些需要重点关注的问题。以下是详细的审查意见:

一、 代码安全

  1. 控制套接字权限过于宽松(高风险)
    在 seatd/seatd.c 中,open_control_socket 调用了 open_socket_internal(path, -1, -1, true, 0600)。虽然模式设为 0600,但传入的 uid 和 gid 都是 -1,这意味着套接字将属于启动 dde-seatd 的 root 用户。

    • 问题:如果 dde-seatd 以 root 启动,该控制套接字仅限 root 访问。如果通过 -u 降权,由于 chown 被跳过(uid == -1 && gid == -1),套接字仍属于 root,降权后的进程可能无法连接。
    • 建议:控制套接字应支持通过命令行参数(如 --control-user / --control-group)指定属主,或者在降权时将控制套接字的属主也修改为对应的运行用户。
  2. drm_is_master 逻辑存在副作用(中风险)
    在 common/drm.c 的 drm_is_master 中,使用 ioctl(DRM_IOCTL_AUTH_MAGIC) 来检测是否是 master。

    • 问题:DRM_IOCTL_AUTH_MAGIC 的本意是认证一个客户端的 magic token,只有 DRM Master 才有权限调用。如果当前进程不是 Master,返回 EACCES;如果是 Master 但传入无效的 magic (0),返回 EINVAL。虽然这种“试探式”调用在逻辑上能区分状态,但它是一个有副作用的系统调用,未来内核或驱动的变更可能导致不可预期行为。
    • 建议:更安全、标准的做法是尝试调用 drmSetMaster,如果已经是 Master,内核会返回 EINVAL 或 EPERM(取决于内核版本),或者直接检查 /proc/self/fd/<fd> 的状态。如果必须保留此逻辑,务必在注释中清晰说明内核对 magic=0 的处理约定。

二、 并发与逻辑

  1. VT 事件线程使用了 pthread_cancel(高风险)
    在 seatd/server.c 的 server_finish 中,使用 pthread_cancel 终止 VT 事件线程。

    • 问题:pthread_cancel 是极其危险的异步取消机制。如果线程在持有锁、分配内存或执行 write/ioctl 时被取消,会导致死锁或资源泄漏。此外,write(server->vt_event_pipe[1], ...) 是阻塞调用,如果在阻塞期间被取消,管道状态可能损坏。
    • 建议:
      a. 改用同步取消机制:保留 vt_event_running 标志,将 vt_event_fd 设置为非阻塞,并在 VT_WAITEVENT 循环中使用 poll 同时监听 vt_event_fd 和一个“唤醒管道”(eventfd 或 pipe)。当需要退出时,主线程向唤醒管道写入数据,子线程 poll 返回后检查 vt_event_running 主动干净退出。
      b. 如果必须用异步取消,至少要在创建线程时设置取消类型为 PTHREAD_CANCEL_DEFERRED,并在 write 等系统调用前后使用 pthread_cleanup_push/pop 清理资源。
  2. seat 结构体缺乏多线程保护(高风险)
    引入了 vt_event_thread 线程,它通过 server_handle_vt_event -> seat_handle_vt_event 修改 seat->cur_vt、seat->pending_vt_switch,并调用 seat_disable_client 等操作。同时,主 poller 线程也在处理控制套接字和普通套接字事件,修改同一个 seat 结构体。

    • 问题:两个线程并发读写 seat 的链表和状态变量,完全没有互斥锁保护,会导致数据竞争和链表损坏,引发 UAF 或 Crash。
    • 建议:在 struct server 或 struct seat 中引入 pthread_mutex_t。在 seat_handle_vt_event 及主线程修改 seat 状态的入口处加锁。注意锁的粒度,避免在持锁期间调用阻塞 I/O。
  3. seat_get_server 的脆弱抽象(低风险)
    seat_get_server 通过取 seat->clients.next 强转来获取 server 指针。

    • 问题:如果 clients 链表为空,将导致空指针解引用(目前代码在调用前做了非空检查,但依然危险)。此外,seat 应该直接包含 struct server *server 指针,而不是通过黑客手段反向查找。
    • 建议:在 struct seat 中直接增加 struct server *server 字段,在 seat_create 时赋值。

三、 代码性能与质量

  1. seat_activate_device 中 DRM Master 检查的冗余(性能/逻辑)
    在 seatd/seat.c 中,修改了 seat_activate_device,先调用 drm_is_master,如果不是再调用 drm_set_master。

    • 问题:这多了一次 ioctl 系统调用。实际上,直接调用 drm_set_master 即可:如果当前已经是 master,内核会返回 EINVAL 或无错误;如果不是且有权成为 master,则提升权限。先查再改在并发环境下也容易产生 TOCTOU 问题。
    • 建议:直接调用 drm_set_master,忽略 EINVAL 错误即可,减少一次内核态切换。
  2. strdup("") 的不必要内存分配(代码质量)
    在 seat_create_group_vt 中:

    group_vt->user = user != NULL ? strdup(user) : strdup("");
    • 问题:当 user 为 NULL 时,分配堆内存保存空字符串不仅浪费内存,还增加了 free 的负担。
    • 建议:如果业务允许,可以直接让 group_vt->user = NULL,在打印时判断;或者定义一个全局的 static const char empty_str[] = ""; 指向它,避免无谓的 malloc。
  3. VT 事件管道的阻塞性写入(逻辑)
    注释中写道:“Keep the writer blocking so VT switch events are not dropped”。

    • 问题:如果主线程因某种原因卡住未读取管道,VT 事件线程的 write 将阻塞,进而导致 VT_WAITEVENT 无法被及时调用,可能阻塞整个内核级的 VT 切换机制。
    • 建议:将 vt_event_pipe[1] 也设置为非阻塞。如果 write 返回 EAGAIN,说明主线程处理缓慢,此时可以选择丢弃最新事件或覆盖旧事件(环形缓冲区),绝不能让内核 VT 切换等待用户态排空管道。

四、 打包与构建

  1. Debian debian/copyright 匹配规则(打包规范)
    修改了 Files: debian/* debian/**/*。

    • 问题:在 DEP-5 规范中,debian/**/* 是多余的。debian/* 默认就会递归匹配 debian/ 目录下的所有文件和子目录文件。写法重复且不符合常见规范。
    • 建议:改回 Files: debian/*。
  2. Debian debian/control 缺失优先级(打包规范)
    源码包的 Priority: optional 被删除了。

    • 问题:虽然这是默认值,但显式声明是良好实践,删除它没有收益。
    • 建议:恢复 Priority: optional。
  3. Debian debian/dde-seatd.init 缺失依赖(打包规范)
    新增了 init 脚本,但 Required-Start 只有 $local_fs $remote_fs。

    • 问题:该脚本依赖 daemon 功能(来自 init-d-script),通常需要声明 $named 和 $network(虽然本地套接字不一定需要网络,但 init-d-script 往往包含在这些基本依赖中)。
    • 建议:确保在目标 DDE 环境中,该 init 脚本的依赖能够正常启动。通常加上 $all 或确保依赖链完整。

总结

最关键且必须修复的是并发安全问题(pthread_cancel 和 seat 结构体无锁保护),这在多核系统和频繁 VT 切换时极易导致守护进程崩溃。其次是重新评估控制套接字的权限归属,确保 DDM 能在降权环境下正常连接。建议在合并前针对多线程场景进行充分的压测(如频繁快速切换 VT)。

@zccrs

zccrs commented Jun 2, 2026

Copy link
Copy Markdown
Member Author

cla检查失败是预期内的,因为从上游cherry-pick过来了一个commit。

@zccrs
zccrs merged commit 1175470 into linuxdeepin:dde/0.9.3 Jun 2, 2026
9 of 10 checks passed
@zccrs
zccrs deleted the backport/dde-0.9.3-grouped-vt branch June 2, 2026 05:20
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.

5 participants