Conversation
… /proc/<nr> by tid /proc exposed three behaviours that disagree with Linux 6.6. 1. Every read_at() re-rendered the record and then sliced it with the file position, so a second read() could hand back the tail of a longer render instead of staying at EOF. Serve the seq-style files the way seq_read_iter() does: render once per fd, replay that snapshot, and re-render only on rewind or when the position no longer matches the continuation point. A failed render drops the continuation point, as a failed traverse() resets the buffer. Files Linux serves through a plain ->read (oom_score_adj, /proc/<pid>/cmdline, sys/*) keep streaming on purpose. 2. Re-parenting rewrote only the group leader, so /proc/<pid>/task/<tid>/status reported a different Ppid than /proc/<pid>/status for the same thread group. Walk the whole group the way forget_original_parent() does. 3. /proc/<nr> resolved through the TGID link only, so a non-leader tid had no directory. Resolve lookups through the PID link (proc_pid_lookup()) while directory listing keeps using the TGID link (next_tgid()), and drop non-leader entries from the per-directory cache so the listing cannot leak a tid that lookup created. Add normal/procfs_task_semantics to the dunitest whitelist. Five of its twelve cases fail on the pre-fix kernel; the rest guard the directions that must not change. Signed-off-by: longjin <longjin@dragonos.org>
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 580e98b961
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…er fd
The first version of this fix froze the whole record of every seq-style
procfs file per fd. Two review comments on this PR showed where that is
too coarse:
- /proc/<pid>/maps buffered a copy of the whole mapping table in every
open fd, so a process that creates many VMAs can multiply that buffer
by RLIMIT_NOFILE (1048576 by default). Linux freezes one buffer, not
the record, so maps now renders one slice at a time from a cursor that
is looked up the way find_vma() does (find_nearest(): the mapping
covering the address, else the first one above it, as vma_iter_init()
plus vma_next() return), and the fd holds at most one page plus the
single line that crossed the bound.
- /proc/<pid>/{mounts,mountinfo,mountstats} resolved the target again on
the first read, so a setns(), unshare() or chroot() after open()
changed what the fd reported. MountView::capture() now pins mnt_ns and
the root at open time, the way mounts_open_common() stores p->ns and
p->root, and read_at() renders from that view only.
The shared driver was aligned with seq_read_iter() in two more places: a
read at offset 0 always rewinds, because Linux resets m->index/m->count
on every ki_pos == 0 read, and a read that arrives without the state
ProcFile::open() installed is refused with EINVAL instead of silently
falling back to the per-read rendering this fix removes.
Tests: normal/procfs_task_semantics grows to 18 cases, including
MapsStreamsMappingsAddedAfterFirstRead, MapsLargeReadReassemblesTheSameRecord,
MountInfoKeepsTheRootPinnedAtOpen, MapsStreamKeepsCopiedBytesWhenTargetDies
and MapsStreamKeepsTheAddressSpaceOpenedOn. Restoring the whole-table maps
render, or the read-time mount resolution, makes 5 of the 18 fail, so they
are falsifiable rather than tautological. Guest run (QEMU/KVM): 18/18, and
the 11 existing procfs suites are unchanged from the baseline
(proc_thread_accounting_test still fails for a missing /sys/fs/cgroup in
the probe rootfs, on the baseline as well).
Signed-off-by: longjin <longjin@dragonos.org>
fslongjin
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 906f6fc400
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… slice Review of DragonOS-Community#2285 found four places where a procfs node reported another task's state, or buffered more than the file it serves has to: - /proc/net/arp went through proc_read_snapshot(), so every open fd held a copy of the whole neighbour table: N descriptors multiplied an unbounded table by N. It re-sampled the caller's network namespace on every read as well. The file now renders one seq slice at a time through proc_read_seq() and pins the namespace at open(), the way seq_open_net() stores it in the seq private data. - /proc/<tid>/fd, /proc/<tid>/fdinfo, the mounts family and /proc/<tid>/cgroup resolved the thread group leader rather than the task the node names, so a thread that took a private files table (close_range(CLOSE_RANGE_UNSHARE)) or its own fs_struct (unshare(CLONE_FS)) was reported through the leader. They now resolve ProcPidTarget::task(), which is Linux get_proc_task(inode): the leader for /proc/<tgid>, the thread for /proc/<tid>. - /proc/<nr>/stat took CPU time (fields 14/15) from the per-thread accounting while its fault counters (10/12) aggregated the thread group. Both now come from the one usage view do_task_stat() selects with whole=1, so the two directories of a task group cannot disagree. - The mounts open path reported ESRCH for a task that is gone; mounts_open_common() leaves EINVAL there. The dunitest suite grows to 22 cases: it pins the thread-scoped fd and mounts subtrees and the thread-group stat view against threads holding private state, plus a sliced-read reassembly case for /proc/net/arp. The file now owns a single probe-thread helper instead of two. Signed-off-by: longjin <longjin@dragonos.org>
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…trait graph
The kernel's object graph is one strongly-connected component, so proving
`Send`/`Sync` for a type such as `File` walks an obligation chain of roughly 130
levels through it. That chain is just past rustc's default budget of 128: once
`/proc/net/arp` began pinning its network namespace per file (the
`seq_open_net()` behaviour asked for in review), the riscv64 and loongarch64
builds failed with
error[E0275]: overflow evaluating the requirement
`alloc::sync::Weak<filesystem::page_cache::PageCache>: Sync`
at `kernel/src/filesystem/page_cache/writeback.rs:4393`, while x86_64 still
passed. `writeback.rs` is untouched by this branch, so the report is about the
solver's budget, not about a type that is actually unsound.
Adding only the new `ProcfsFilePrivateData.net_ns` field on top of the previous
commit reproduces that exact diagnostic, so this branch's new edge is what
consumed the last of the margin; master already resolves at a depth of 121-128
on riscv64. The budget is load-bearing rather than cosmetic as well: the
generated code measurably differs between budgets (the kernel crate's `.text`
grows by about 2.6% from 128 to 256), so it cannot simply be ignored.
Set the limit to 256, the value rustc suggests for this diagnostic and about
twice the measured depth, rather than dropping an edge from the type graph or
giving up the per-file namespace pin that `seq_open_net()` requires.
Verified: riscv64 and loongarch64 `cargo build --release` with the CI flags and
x86_64 `make kernel` all succeed, and `FMT_CHECK=1 make fmt` is clean.
Signed-off-by: longjin <longjin@dragonos.org>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f94b8e70d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| proc_read_snapshot(offset, len, buf, &mut data, move || { | ||
| render_mount_file(&view, self.kind) | ||
| }) |
There was a problem hiding this comment.
当具有挂载权限的容器进程创建接近 mount-max（默认 100,000）个挂载点，并打开大量 /proc/self/{mounts,mountinfo,mountstats} fd 后各读取一个字节时，proc_read_snapshot() 会为每个 fd 渲染并长期保留整张挂载表；结合默认可达 1,048,576 的 RLIMIT_NOFILE，这会形成 O(fd 数量 × 挂载数量) 的全局内核堆放大。Linux 的 seq_file 只缓存当前输出块，因此这些文件也应使用带挂载游标的有界增量渲染，而不是整表快照。
AGENTS.md reference: AGENTS.md:L9-L11
Useful? React with 👍 / 👎.
| let ns = task.nsproxy().mnt_ns.clone(); | ||
| let root = task | ||
| .try_fs_struct() | ||
| .ok_or(SystemError::ENOENT)? | ||
| .root() |
There was a problem hiding this comment.
当目标线程在另一个进程打开其 mountinfo 的同时执行 setns(CLONE_NEWNS) 或 unshare(CLONE_NEWNS) 时，这两个独立读取可能捕获不一致的视图：PreparedNamespaceInstall::commit() 先在 nsproxy.rs:426-427 发布新 fs_struct，再于 430 行发布新 nsproxy，因此这里可组合出旧挂载命名空间和新根目录。随后渲染的路径和拓扑来自不同命名空间；应像 Linux 的 mounts_open_common() 一样，在共同的任务状态锁下同时固定二者。
AGENTS.md reference: AGENTS.md:L9-L11
Useful? React with 👍 / 👎.
1 participant
Fixes #2283.
What was wrong
Three behaviours of
/procdisagreed with Linux 6.6:read_at()re-rendered the record and then sliced it by file position, so a secondread()could return the tail of a newer, longer render instead of hitting EOF. Linuxseq_read_iter()keeps one buffer per fd and only re-enters the handler when that buffer runs dry.reparentonly rewrote the thread group leader, so/proc/<pid>/task/<tid>/statusand/proc/<pid>/statusreported differentPpidfor the same thread group./proc/<nr>was resolved through the TGID chain only, so a non-leader tid had no directory, unlikeproc_pid_lookup().Fixes
1. One seq driver for the whole procfs read path (
procfs/utils.rs)ProcfsSeq(rendered bytes + resume cursor + "record ended" flag, mirroringstruct seq_file) plusproc_read_seq(), which reproducesseq_read_iter()/seq_lseek():offset == 0rewinds (Linux resetsm->index/m->counton everyki_pos == 0read);offset == fd positioncontinues from the buffer, without re-entering the record source — this is what makes EOF stay EOF while content grows;seq_read_iter(): bytes already copied out are returned, and only a slice that produced nothing reports the error.proc_read_snapshot()is a 12-line wrapper over the same driver forsingle_open()-style files, so there is one implementation of the semantics, not two. Files that Linux serves with a plain->read(oom_score_adj,/proc/<pid>/cmdline,sys/*) keep streaming; the driver refuses a read that arrives without the stateProcFile::open()installed instead of silently falling back to the per-read rendering this PR removes.2.
/proc/<pid>/mapsrenders one slice at a time (procfs/pid/maps.rs)The first version of this PR froze the whole mapping table per fd. A PR reviewer pointed out that this is an unprivileged memory DoS: a process can create many VMAs, open many
/proc/self/mapsfds (defaultRLIMIT_NOFILEis 1048576) and read one byte from each, so kernel heap grows asfds x mappings. Linux freezes one buffer, not the record (m->size = PAGE_SIZE, and the fill loop stops as soon as it has at least what the reader asked for).So
mapsnow streams:render_maps_slice()walks the mapping table from a cursor that is looked up the wayfind_vma()does (find_nearest(): the mapping covering the address, else the first one above it, which is whatvma_iter_init()+vma_next()return) and stops at the slice budget; the fd holds at most one page plus the one line that crossed the bound. A mapping added after the first read shows up later in the stream, andopen()pins the address space underexec_update_read()so a continuation read never re-resolves the target. The descriptor is pinned, not the memory: likeproc_mem_open()(mmgrab()thenmmput(), "but do not pin its memory"), each slice re-checks the user count throughtry_acquire(), soexecve()/exit still tears the mappings down and the stream ends there instead of walking a dead table.3.
/proc/<pid>/{mounts,mountinfo,mountstats}pin their view at open (procfs/mount/view.rs)The reviewer's second point: the task reference was dropped and the target re-resolved on the first read, so a
setns(),unshare()orchroot()afteropen()changed what the fd reported.MountView::capture()now takesmnt_ns+ root inopen()and stores them in the file's private data, exactly likemounts_open_common()storesp->ns/p->root;read_at()renders from that view only.4. Thread-group re-parent and
/proc/<nr>lookupreparent_one_task_locked()now walks the whole thread group the wayforget_original_parent()does, and/proc/<nr>is resolved through the PID chain (proc_pid_lookup()) while directory listing keeps using the TGID chain (next_tgid()), with non-leader entries filtered out of the per-directory cache so listing cannot leak directories that lookup created.Review follow-up (
dc0164bff)The third review round found four places where a node reported another task's state, or buffered more than it serves:
a.
/proc/net/arpstreams per slice and pins its namespace (procfs/net/arp.rs,net/neighbor/mod.rs)It went through
proc_read_snapshot(), so every open fd held a copy of the whole neighbour table and N descriptors multiplied an unbounded table by N.render_arp_slice()now renders throughproc_read_seq()with the same one-page budgetmapsuses, and emits the header only on the first slice. The old code also re-sampledProcessManager::current_netns()on every read, so asetns()between two reads could join the header of one namespace to the entries of another;open()now pins the namespace in the fd's private data, which is whatseq_open_net()does withget_proc_net(inode).neighbor::get_arp_entries()takes that namespace from its caller instead of resolving the current one itself.b. Hidden tids resolve their own task (
pid/mod.rs,pid/fd.rs,pid/fdinfo.rs,pid/cgroup.rs,mount/inode/pid_mount.rs)/proc/<tid>/fd,/proc/<tid>/fdinfo, the mounts family and/proc/<tid>/cgroupresolvedthread_group_leader(). A thread that took a private files table (close_range(CLOSE_RANGE_UNSHARE)) or a privatefs_struct(unshare(CLONE_FS), whichCLONE_NEWNSimplies) was therefore reported through the leader. They now useProcPidTarget::task(), i.e. Linuxget_proc_task(inode)throughPIDTYPE_PID: the leader for/proc/<tgid>, the named thread for/proc/<tid>. The existence gate forfd/fdinfomoved to the same lookup, so it can no longer fall back to an empty directory for a thread that is still alive.c.
/proc/<nr>/stattakes CPU time from the same view as its fault counters (pid/stat.rs)Fields 14/15 came from
pcb.cputime()(the named thread, and for/proc/<tgid>the leader alone) while 10/12 aggregated the thread group. Both now come from the oneusageview the node'sStatScopeselects, mirroringdo_task_stat()wherewholepicksthread_group_cputime_adjusted()together with the group counters. A hidden tid keeps the group view, because/proc/<nr>/statisproc_tgid_stat()(whole = 1) whatevernrnames;/proc/<pid>/task/<tid>/statkeeps the per-thread one.d. The mounts open path reports
EINVALfor a task that is gone, asmounts_open_common()does (ret = -EINVAL); it reportedESRCHbefore.Known bounds (deliberate, not hidden)
/proc/<pid>/mapsstill has no ptrace gate (proc_mem_open()usesPTRACE_MODE_READon Linux). Pre-existing, tracked separately./proc/<pid>/{uid,gid}_mapread deadlocks on aSpinLock; also pre-existing and unchanged by this PR./proc/<pid>/net, so/proc/net/arppins the net namespace of the task that opened it. Linux's/proc/netis a symlink toself/net, which resolves through the group leader; the two differ only for a thread that took a private net namespace, and closing that gap means adding/proc/<pid>/netrather than changing this file.Build follow-up (
0f94b8e70)Build riscv64andBuild loongarch64failed ondc0164bffwithat
kernel/src/filesystem/page_cache/writeback.rs:4393, while x86_64 still passed. That file is untouched by this branch. The kernel's object graph is one strongly-connected component, so provingSend/Syncfor a type such asFilewalks an obligation chain of roughly 130 levels through it, andmasteralready resolves at a depth of 121-128 on riscv64, so rustc's default budget of 128 was nearly exhausted. Adding only theProcfsFilePrivateData.net_nsfield from part (a) on top of the previous commit reproduces that exact diagnostic, which is what this branch's new edge tipped over.The budget is load-bearing rather than cosmetic: the generated code measurably differs between budgets (the kernel crate's
.textgrows by ~2.6% from 128 to 256), and 129 - just enough to stop reporting the error - still produces the same object code as the default 128. The fix therefore sets#![recursion_limit = "256"]inkernel/src/lib.rs, the value rustc suggests for this diagnostic and about twice the measured depth, rather than dropping an edge from the type graph or giving up the per-file namespace pin thatseq_open_net()requires.Verified with the CI flags: riscv64 and loongarch64
cargo build --releaseand x86_64make kernelall succeed, andFMT_CHECK=1 make fmtis clean. Because x86_64 code generation is affected, the x86_64 kernel's behaviour is covered by the existingDunitestandIntegration Testjobs rather than argued from the attribute being "build time only".Tests
normal/procfs_task_semantics(22 cases) plus 11 existing procfs suites, run in the guest (QEMU/KVM, same rootfs):procfs_task_semantics22/22 PASSED, including the newMapsStreamsMappingsAddedAfterFirstRead,MapsLargeReadReassemblesTheSameRecord,MountInfoKeepsTheRootPinnedAtOpen,MapsStreamKeepsCopiedBytesWhenTargetDies,MapsStreamKeepsTheAddressSpaceOpenedOn,SeekPastEndStaysEofAndRewindReRenders,TidFdSubtreeUsesTheThreadsFilesTable,TidMountsUsesTheThreadsRoot,TidStatReportsThreadGroupUsageandArpChunkedReadReassemblesTheSameRecord;close_range(CLOSE_RANGE_UNSHARE)for a private files table,unshare(CLONE_FS)pluschroot()for a private root, and a probe thread that burns user time so the per-thread and thread-group stat views cannot coincide;proc_thread_accounting_teststill fails for an environment reason (the probe rootfs has no/sys/fs/cgroup/cgroup.subtree_control), on the baseline as well;mapsrender and the read-time mount resolution makes 5 of the round-2 cases fail, and reverting the three round-3 fixes (the leader lookups infd/fdinfo/mountsandpcb.cputime()for fields 14/15) fails exactlyTidFdSubtreeUsesTheThreadsFilesTable,TidMountsUsesTheThreadsRootandTidStatReportsThreadGroupUsage, with 19 of 22 passing. The new cases are therefore falsifiable rather than tautological.make fmtandmake -C kernel fmt FMT_CHECK=--checkpass;make kernelpasses with no new warnings.Signed-off-by: longjin longjin@dragonos.org