Skip to content

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

Open
sparkzky wants to merge 1 commit intoDragonOS-Community:masterfrom
sparkzky:fix-procfs-pid-entries
Open

fix(procfs): use vpid-aware lookup for /proc pid entries#1808
sparkzky wants to merge 1 commit intoDragonOS-Community:masterfrom
sparkzky:fix-procfs-pid-entries

Conversation

@sparkzky
Copy link
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
Collaborator Author

sparkzky commented Mar 9, 2026

@claude review

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

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!

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.

1 participant