Skip to content

fix(procfs): use vpid-aware lookup for /proc pid entries#1808

Merged
fslongjin merged 2 commits intoDragonOS-Community:masterfrom
sparkzky:fix-procfs-pid-entries
Mar 31, 2026
Merged

fix(procfs): use vpid-aware lookup for /proc pid entries#1808
fslongjin merged 2 commits intoDragonOS-Community:masterfrom
sparkzky:fix-procfs-pid-entries

Conversation

@sparkzky
Copy link
Copy Markdown
Collaborator

@sparkzky sparkzky commented Mar 9, 2026

Switch /proc/[pid] handlers from ProcessManager::find to
find_task_by_vpid via a shared helper so procfs resolves tasks in the
caller-visible pid namespace.

Also simplify fd/fdinfo inode creation to pass pid directly and update
/proc root directory enumeration to collect visible pids from the active
pid namespace.

  Switch /proc/[pid] handlers from ProcessManager::find to
  find_task_by_vpid via a shared helper so procfs resolves tasks in the
  caller-visible pid namespace.

  Also simplify fd/fdinfo inode creation to pass pid directly and update
  /proc root directory enumeration to collect visible pids from the active
  pid namespace.

Signed-off-by: sparkzky <sparkhhhhhhhhhh@outlook.com>
@github-actions github-actions bot added the Bug fix A bug is fixed in this pull request label Mar 9, 2026
@sparkzky
Copy link
Copy Markdown
Collaborator Author

sparkzky commented Mar 9, 2026

@claude review

@dragonos-community-ai-pr-reviewer
Copy link
Copy Markdown

dragonos-community-ai-pr-reviewer bot commented Mar 9, 2026

Claude finished @sparkzky's task in 6m 6s —— View job


PR Review: fix(procfs): use vpid-aware lookup for /proc pid entries

Review Summary:
The PR implements vpid-aware PID lookups for /proc/[pid] entries, correctly handling PID namespace isolation. The changes switch from global process lookup to namespace-aware lookups and simplify inode creation by passing PIDs directly instead of process references.

✅ Positive Aspects:

  1. Correct PID Namespace Handling:

    • Switches from ProcessManager::find to find_task_by_vpid which respects the caller's PID namespace
    • /proc root enumeration now uses active_pid_ns().collect_pids() instead of global process list
    • This is essential for containerization and PID namespace isolation
  2. Improved Memory Safety:

    • Storing RawPid instead of Arc<ProcessControlBlock> reduces reference counting overhead
    • Avoids keeping processes alive unnecessarily through procfs references
    • Process lookups are done dynamically when needed
  3. Good Error Handling:

    • Most files handle ESRCH appropriately when processes exit
    • fd and fdinfo directories create empty directories when processes exit (lines 101-127, 131-157 in mod.rs)
    • Race conditions with process exit are handled reasonably
  4. Consistent Implementation:

    • All files use the shared find_process_by_vpid helper (defined in mod.rs)
    • Pattern is consistent across all /proc/[pid] files

🔍 Code Review Findings:

kernel/src/filesystem/procfs/root.rs:

  • Line 89: ProcessManager::find_task_by_vpid(pid).is_some() - Good switch to vpid-aware lookup
  • Lines 122-128: PID collection respects active PID namespace and filters PID 0 correctly
  • The populate_children implementation is cleaner than the old global process list approach

kernel/src/filesystem/procfs/pid/mod.rs:

  • Lines 43-45: find_process_by_vpid helper properly abstracts the vpid-aware lookup
  • Lines 101-127, 131-157: Good handling of exited processes for fd and fdinfo directories
  • The pattern of storing pid and looking up dynamically is well-implemented

Other files (cmdline.rs, exe.rs, fd.rs, fdinfo.rs, etc.):

  • All consistently switched to find_process_by_vpid helper
  • Error handling with ok_or(SystemError::ESRCH) is appropriate

⚠️ Minor Considerations:

  1. Race Conditions: As with any procfs implementation, there are inherent race conditions when processes exit between lookup and actual file operations. The current handling (returning ESRCH or empty directories) is standard and acceptable.

  2. Performance: The new active_pid_ns().collect_pids() might be slightly slower than the old global list, but this is the correct behavior for PID namespace support.

  3. Line 127 in mod.rs: The empty directory for exited processes has mode 0o500 (dr-x------). This might be stricter than needed but is conservative and safe.

📋 Testing Suggestions:

  • Test with PID namespaces to ensure /proc shows only visible processes
  • Test process exit while reading /proc/[pid] files
  • Verify fd and fdinfo directories behave correctly when processes exit

Overall: This is a well-implemented PR that correctly addresses PID namespace isolation in procfs. The changes are consistent, maintainable, and follow good practices. The approach of storing PIDs and looking up processes dynamically is sound.

Recommendation: APPROVE - The PR is ready for merge.


Todo list:

  • Gather context and read changed files
  • Understand the request and analyze code changes
  • Check for bugs, security issues, performance problems
  • Review for readability and maintainability
  • Check for best practices and coding standards
  • Provide comprehensive review feedback with specific references

Review completed!

@fslongjin
Copy link
Copy Markdown
Member

Code Review Feedback

请移除 fd.rs 中的调试日志

kernel/src/filesystem/procfs/pid/fd.rs 中新增的以下代码应当移除:

let open_fds: Vec<i32> =
    fd_table_guard.iter().map(|(open_fd, _)| open_fd).collect();
log::warn!(
    "procfd lookup miss: pid={} fd={} open_fds={:?}",
    self.pid,
    fd,
    open_fds
);

原因:

  1. 每次 fd lookup miss 时遍历整个 fd 表并 collectVec,生产环境开销不可接受
  2. log::warn! 级别过高,fd lookup miss 在正常运行中会频繁发生(进程关闭 fd、竞态等),会导致日志刷屏

修改后的代码应恢复为简单的:

if fd_table_guard.get_file_by_fd(fd).is_none() {
    return Err(SystemError::ENOENT);
}

@fslongjin fslongjin merged commit a75536b into DragonOS-Community:master Mar 31, 2026
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug fix A bug is fixed in this pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants