portal: read the procfs files kernel_read() refuses, and stop hiding why - #102
Open
lacraig2 wants to merge 5 commits into
Open
portal: read the procfs files kernel_read() refuses, and stop hiding why#102lacraig2 wants to merge 5 commits into
lacraig2 wants to merge 5 commits into
Conversation
Live CI on all 10 6.13 arch combos found /proc/net/tcp and /proc/self/status
coming back EINVAL after zero bytes, while /proc/version, /proc/mounts,
/proc/uptime, /proc/cmdline and a real sysfs file read fine in the same boot --
and the guest itself reads /proc/self/status fine in that same boot. All 7 4.10
combos pass. Verified against the 6.13 source, the cause is one line in
__kernel_read():
if (unlikely(!file->f_op->read_iter || file->f_op->read))
return warn_unsupported(file, "read"); /* -EINVAL */
So a file that has ->read wired up, or lacks ->read_iter, cannot be read this
way at all. That is a property of the file's f_op, not of statefulness, and it
partitions procfs exactly as observed: proc_create_single* entries get
proc_iter_file_ops (.read_iter only) and work, while /proc/<pid>/status uses
proc_single_file_operations (.read = seq_read) and /proc/net/* register
proc_read = seq_read, landing on proc_reg_file_ops (.read, no .read_iter) --
both refused. 4.10 has no such guard (kernel_read -> __vfs_read -> ->read),
which is why the same paths work there, and why the older stateless
handle_op_read_file fails on exactly the same files: it uses the same
kernel_read.
Fixed for the ->read == seq_read case, which is safe to fix: seq_read's contract
is that file->private_data is a struct seq_file, so seq_read_iter can be called
directly with a kvec iterator. That makes /proc/<pid>/* readable. Gated on 5.10+,
where seq_read_iter exists and where the refusal it works around appeared;
verified to compile out on 4.10 (the 4.10 module references neither symbol) and
in on 6.13 (both resolve to exported symbols).
Deliberately NOT extended to proc_reg_file_ops (/proc/net/*): its ->read is
proc_reg_read, which forwards to a proc_ops a module cannot inspect, so there is
no way to prove private_data is a seq_file. In /proc/<pid>/mem it is an
mm_struct -- guessing would corrupt memory rather than fail. Reading those needs
a user-address bounce buffer in the calling task, which touches guest state and
is a separate decision. The header comment records the partition so the next
reader does not have to re-derive it.
Also: a read or close against an unknown or already-closed handle now reports
-EBADF instead of -EINVAL. That collision was mine and it cost real time: EINVAL
is also what kernel_read() returns for a file it will not serve, so a live
failure could not be attributed to the handle table or to the kernel's read path
without a debug build, which CI does not run. -EBADF is what read(2) uses for a
bad descriptor, so it is also just the right answer.
…hazards Four defects in the vfs bridge, all of the kind that surfaces as a hang or a mysterious dead slot rather than a test failure. 1. vfs_lock was held across the read. That is a liveness bug twice over. A read that blocks -- a FIFO with no writer, /proc/kmsg with an empty buffer -- held the mutex for as long as it blocked, so every other handle stalled behind one unlucky path with no timeout anywhere. And a read of a modelled pseudofile issues a hypercall from inside the read, re-entering the portal; that nested path demonstrably executes, so it was one table access away from deadlocking against itself. The read now runs with the lock dropped, and the slot carries a busy flag so a concurrent read or close on the same handle gets -EBUSY instead of interleaving chunks of one seq_file. Busy slots are never reclaimed, which is what keeps the struct file alive across the unlocked window; the read also holds its own reference, so the invariant is local rather than spread across three functions. 2. filp_open() used O_RDONLY alone, so opening a FIFO with no writer blocked in the OPEN -- before any locking question arises -- and wedged the hypercalling guest task forever. Opens are now O_NONBLOCK: such a file opens and reads -EAGAIN, which the host can report. Synthetic filesystems ignore the flag. 3. The generation counter was compared unmasked against a handle that only has 24 bits to carry it. After 2^24 closes of one slot no comparison could ever match again and the slot was permanently -EBADF: a slow leak that would surface on a long run and on no test. Masked, it wraps and reuses. 4. vfs_claim_slot() reclaimed the oldest slot unconditionally, including one with a read in flight -- freeing the file being read from. It now skips busy slots, and refuses the open with -ENFILE if every slot is mid-read rather than picking a victim. Builds clean for 4.10 and 6.13 (armel).
…the unlocked read safe igloo_portal() takes a fresh page per call, so the nested portal call a modelled pseudofile read makes cannot clobber the payload buffer being filled. The previous commit's fix depends on that and did not say so.
…wards It said a model should implement ONLY read_iter, on the reasoning that __kernel_read refuses any file with .read set. Both halves are wrong. The guard applies to the f_op PROCFS chose, not to what the model supplied. On 5.6+ procfs sets i_fop to proc_iter_file_ops (.read_iter, no .read) iff proc_ops->proc_read_iter is non-NULL, else proc_reg_file_ops -- and it never looks at proc_read, so keeping ->read costs nothing. And read_iter alone breaks 4.10 outright: every regular procfs file gets proc_reg_file_ops there whatever the module supplied, and proc_reg_read forwards only ->read, so the file returns -EIO to the guest as well as to the host. A model needs BOTH. Verified against fs/proc/inode.c and fs/read_write.c in both kernels; the rule is now a truth table with citations in penguin's tests/unit/test_pseudofile_host_readability.py.
handle_op_exec parsed the host's blob with `buf += strlen(buf) + 1` and no end pointer, so a blob that was not properly double-NUL terminated walked strlen() off the end of the page. The host is the only caller and now always terminates it correctly, which makes this defence in depth rather than a live bug -- a length-free walk over a page boundary is still a bug. Three things it truncated now fail instead: * An exe_path longer than 255 was shortened by strncpy and then RUN, as a different binary. And because `offset += strlen(exe_path) + 1` used the truncated length, argv parsing started in the middle of the real path, so every argument was garbage too. -ENAMETOOLONG. * More than 15 arguments: the extras were dropped AND env_buf was derived from where the argv walk stopped -- the middle of the argument list -- so the environment came out garbage. -E2BIG. * More than 15 environment variables: dropped. -E2BIG. Errors ride the existing contract: handle_op_exec already returns the call_usermodehelper retval in header.size as HYPER_RESP_READ_NUM, where negative already means failure. The host refuses all three before building the blob, so this is the second line rather than the first. Builds clean for 4.10 and 6.13 (armel).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Live CI on penguin #938 read procfs through the new
vfs_*ops on every arch/kernel combo. All 7 4.10 combos pass; all 10 6.13 combos fail on exactly two files:In the same boot the guest reads the file fine (
COMPAT_PROC_SPECIAL:self_status=readable), so this is not a kernel-config gap and not pseudofile shadowing.Cause, verified against the 6.13 source
One line in
__kernel_read():A file that has
->readwired up, or lacks->read_iter, cannot be read that way at all. That is a property of the file'sf_op, not of statefulness — and it partitions procfs exactly as measured:/proc/version,/proc/uptime,/proc/cmdlineproc_create_single*→proc_iter_file_ops(.read_iteronly)/proc/mountsmounts_operations(.read_iter = seq_read_iter)/sys/...kernfs_file_fops(.read_iter)/proc/<pid>/status,/statproc_single_file_operations(.read = seq_read)/proc/net/*proc_net_seq_ops.proc_read = seq_read→proc_reg_file_ops(.read, no.read_iter)4.10 has no such guard (
kernel_read→__vfs_read→->read), which is why the same paths work there — and why the older statelesshandle_op_read_filefails on exactly the same files: samekernel_read.What this fixes
The
->read == seq_readcase, which is safe to fix:seq_read's contract is thatfile->private_datais astruct seq_file, soseq_read_itercan be called directly with a kvec iterator, skipping the guard. That makes/proc/<pid>/*readable. Gated on 5.10+ (whereseq_read_iterexists and where the refusal appeared); verified to compile out on 4.10 (the 4.10 module references neither symbol) and in on 6.13 (both resolve to exported symbols). Built clean forigloo-4.10-armelandigloo-6.13-armel.Not extended to
proc_reg_file_ops(/proc/net/*), deliberately: its->readisproc_reg_read, which forwards to aproc_opsa module cannot inspect, so there is no way to proveprivate_datais a seq_file. In/proc/<pid>/memit is anmm_struct— guessing would corrupt memory rather than fail. Reading those needs a user-address bounce buffer in the calling task, which touches guest state and is a separate decision. The header comment records the partition so nobody has to re-derive it.Also: an errno collision that was mine
A read/close against an unknown or already-closed handle now reports
-EBADFrather than-EINVAL. The collision cost real diagnosis time —EINVALis also whatkernel_readreturns for a refused file, so a live failure could not be attributed to the handle table or the kernel's read path without a debug build, which CI does not run.-EBADFis whatread(2)uses for a bad descriptor, so it is also simply correct.Note for the peer graph
/proc/net/tcpstays unreadable on 6.13. The socket peer graph should not depend on it — the already-approvedOSI_FD_ALL/osi_fd_entry2work reads family/state/addresses/ports from the kernelstruct sockdirectly and removes the procfs dependency entirely. That is the better fix for that consumer regardless of this.