Own O_NONBLOCK so a transfer cannot park a vCPU - #317
Open
jserv wants to merge 3 commits into
Open
Conversation
Every blocking-capable read and write used to poll for readiness and then make a blocking host call. That poll reserves nothing: between it and the transfer a sibling thread or a forked process sharing the open file description can take the bytes it promised, and the call that follows parks the vCPU thread where neither hv_vcpus_exit nor the wakeup pipe reaches it. An execve teardown then counts that thread as a sibling that would not leave and takes the post-PNR exit(128) instead of running the new image. elfuse now owns O_NONBLOCK on every fd whose transfer could park, and emulates the guest's blocking semantics on top of it. io_xfer is the one way to run such a transfer: it attempts the move, and waits interruptibly only when the move reports EAGAIN. The wait no longer consumes the process-wide futex interrupt one-shot, which is raised when the last clone-thread exits and belongs to futex and poll waiters; consuming it there truncated a 1 MiB blocking write to one pipe buffer with no signal involved. Owning the flag makes fd_entry_t.linux_flags the guest's view of it, so F_GETFL, F_SETFL, FIONBIO, the transfer paths and every synthetic reader answer from one place. Two consequences follow. An fd that aliases an existing description has to inherit that description's answers rather than probe for its own, which fd_alias_spec_t and its constructors now state at each of the seven sites that build one. And F_GETFL becomes shadow bits plus the bits fd_host_flag_mask says the host still owns, replacing a chain of eight per-type exceptions. Checking that chain against qemu-aarch64 found seven types reporting O_ASYNC that Linux clears: it lands FASYNC only through file_operations->fasync, and SETFL_MASK does not carry it. The guardrail grew the lanes that would have caught this class of regression: a per-transfer detector, a bulk lane, and lanes that run with a sibling thread alive, since fd-table reads skip their lock while only one thread is active and every earlier lane measured that path alone.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Sorting tests/manifest.txt to add a test moved all 22 section headers above every test binary. driver.sh attributes each test to the currently-active section, so all 80 fell under the last header and the sanitizer lanes, which select by section regex, silently ran the whole suite instead of their subset. Nothing failed, which is why five validation runs did not notice. The file is the original again plus the three tests this branch adds, each under the section that owns it. ipc_fd_entry_t grew two fields without bumping FORK_IPC_PROTOCOL_MAGIC, which is what stops a parent from handing a differently-shaped payload to a child built from another revision. Smaller corrections, each its own bug: net.c built an alias spec from an uninitialized snapshot when the source fd had already closed; F_GETFL dropped O_NOATIME, which macOS cannot report and the shadow therefore owns; tmpfile_anon returned a named descriptor when unlink failed; the host stdio test could not be built with GUEST_TEST_BINARIES set, though check requires it in that mode; and the packing assert repeated 1024 rather than asking FD_TABLE_SIZE. Deciding fasync support from can_block was wrong in both directions. Measured against qemu-aarch64: /dev/null and /dev/zero drop O_ASYNC where elfuse kept it, and /dev/urandom keeps it where elfuse dropped it. The answer comes from the object now, not from whether it can block, and the three cases are pinned alongside the eleven fd types already covered. The two proofs added on this branch had no mutations, and writing them found both contracts weak. async_udata_gen was satisfied by an accessor reading the wrong field, since its only postcondition was a range; iov_advance_index was satisfied by a loop that subtracts nothing, since the exit condition alone already places the remainder inside the entry it indexes. Both contracts now state what the function computes, and every mutation is rejected.
There was a problem hiding this comment.
4 issues found across 19 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/utils.h">
<violation number="1" location="src/utils.h:337">
P2: When `unlink(path)` fails for a real error, this closes the descriptor but leaves the `mkstemp` pathname behind. Best-effort unlink `path` during failure cleanup before returning.</violation>
</file>
<file name="src/syscall/asyncio.c">
<violation number="1" location="src/syscall/asyncio.c:284">
P2: When the guest opens `/dev/random`, this branch clears `O_ASYNC` because the character device is not a tty. Linux keeps `O_ASYNC` on `/dev/random` through `random_fasync`, so classify `/dev/random` as the random-device type or otherwise recognize its fasync support before updating the shadow flag.</violation>
</file>
<file name="src/syscall/internal.h">
<violation number="1" location="src/syscall/internal.h:546">
P2: When `F_SETFL` sets or clears `O_NOATIME` on a host-backed fd, this mask makes the bit shadow-owned but the generic `F_SETFL` path never updates that shadow. Update `O_NOATIME` across aliases after a successful `F_SETFL`, or keep it host-authoritative until that synchronization exists.</violation>
</file>
<file name="tests/manifest.txt">
<violation number="1" location="tests/manifest.txt:124">
P3: test-shim-cred-race landed in the `mremap tests` section, but it is a shim credential-race test, not an mremap test, and was previously grouped with the shim tests. This misplacement also pulls it into every SANITIZER_SECTIONS run, since that regex selects the whole `mremap` section. Move it (and the section) near the other EL1 shim tests, or give it a shim section of its own.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } while (rc < 0 && errno == EINTR); | ||
| if (rc < 0) { | ||
| int unlink_errno = errno; | ||
| close(fd); |
There was a problem hiding this comment.
P2: When unlink(path) fails for a real error, this closes the descriptor but leaves the mkstemp pathname behind. Best-effort unlink path during failure cleanup before returning.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/utils.h, line 337:
<comment>When `unlink(path)` fails for a real error, this closes the descriptor but leaves the `mkstemp` pathname behind. Best-effort unlink `path` during failure cleanup before returning.</comment>
<file context>
@@ -324,12 +324,20 @@ static inline int tmpfile_anon(const char *what)
+ } while (rc < 0 && errno == EINTR);
+ if (rc < 0) {
+ int unlink_errno = errno;
+ close(fd);
+ errno = unlink_errno;
+ return -1;
</file context>
| if (S_ISFIFO(st.st_mode) || S_ISSOCK(st.st_mode)) | ||
| return true; | ||
| if (S_ISCHR(st.st_mode)) | ||
| return isatty(host_fd) == 1; |
There was a problem hiding this comment.
P2: When the guest opens /dev/random, this branch clears O_ASYNC because the character device is not a tty. Linux keeps O_ASYNC on /dev/random through random_fasync, so classify /dev/random as the random-device type or otherwise recognize its fasync support before updating the shadow flag.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/syscall/asyncio.c, line 284:
<comment>When the guest opens `/dev/random`, this branch clears `O_ASYNC` because the character device is not a tty. Linux keeps `O_ASYNC` on `/dev/random` through `random_fasync`, so classify `/dev/random` as the random-device type or otherwise recognize its fasync support before updating the shadow flag.</comment>
<file context>
@@ -237,6 +238,53 @@ static void owner_set_slot(int guest_fd, void *ctx)
+ if (S_ISFIFO(st.st_mode) || S_ISSOCK(st.st_mode))
+ return true;
+ if (S_ISCHR(st.st_mode))
+ return isatty(host_fd) == 1;
+ return false;
+}
</file context>
| */ | ||
| int mask = | ||
| ~(LINUX_O_PATH | LINUX_O_DIRECTORY | LINUX_O_NOFOLLOW | LINUX_O_DIRECT | | ||
| LINUX_O_LARGEFILE | LINUX_O_ASYNC | LINUX_O_NOATIME); |
There was a problem hiding this comment.
P2: When F_SETFL sets or clears O_NOATIME on a host-backed fd, this mask makes the bit shadow-owned but the generic F_SETFL path never updates that shadow. Update O_NOATIME across aliases after a successful F_SETFL, or keep it host-authoritative until that synchronization exists.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/syscall/internal.h, line 546:
<comment>When `F_SETFL` sets or clears `O_NOATIME` on a host-backed fd, this mask makes the bit shadow-owned but the generic `F_SETFL` path never updates that shadow. Update `O_NOATIME` across aliases after a successful `F_SETFL`, or keep it host-authoritative until that synchronization exists.</comment>
<file context>
@@ -541,8 +541,9 @@ static inline int fd_host_flag_mask(int type)
- LINUX_O_DIRECT | LINUX_O_LARGEFILE | LINUX_O_ASYNC);
+ int mask =
+ ~(LINUX_O_PATH | LINUX_O_DIRECTORY | LINUX_O_NOFOLLOW | LINUX_O_DIRECT |
+ LINUX_O_LARGEFILE | LINUX_O_ASYNC | LINUX_O_NOATIME);
/* And the access mode, for the types elfuse opens on the host with a mode
</file context>
| @@ -79,6 +79,8 @@ test-pty | |||
| test-ioctl-fioasync | |||
There was a problem hiding this comment.
P3: test-shim-cred-race landed in the mremap tests section, but it is a shim credential-race test, not an mremap test, and was previously grouped with the shim tests. This misplacement also pulls it into every SANITIZER_SECTIONS run, since that regex selects the whole mremap section. Move it (and the section) near the other EL1 shim tests, or give it a shim section of its own.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/manifest.txt, line 124:
<comment>test-shim-cred-race landed in the `mremap tests` section, but it is a shim credential-race test, not an mremap test, and was previously grouped with the shim tests. This misplacement also pulls it into every SANITIZER_SECTIONS run, since that regex selects the whole `mremap` section. Move it (and the section) near the other EL1 shim tests, or give it a shim section of its own.</comment>
<file context>
@@ -1,158 +1,158 @@
test-mremap-infra
+test-mremap-fork-tracking
test-mremap-tail-emfile # host_nofile=elfuse-minimum
+test-shim-cred-race
+
+[section] msync MAP_SHARED tests
</file context>
elfuse never set SIGPIPE to SIG_IGN, so a host write to a pipe whose reader had gone raised it in the elfuse process itself and the default action ended the VM. Every such write is made on the guest's behalf, so the guest's own SIGPIPE is what the failure means. An ordinary pipeline whose reader exits first was enough to take everything down, before the guest's handler could run; upstream main does it too. A write that had already moved bytes when the reader vanished then reported the count with no signal at all. Linux raises SIGPIPE there as well: pipe_write signals even when it has something to return. io_write_result cannot see it, since the value it gets is a non-negative count with no errno attached, so the round that ended the transfer says so instead. Measured against qemu-aarch64: both sides return the same partial count, and only Linux signalled. tests/test-sigpipe.c covers both. It passes under real Linux as well, so the expectations are Linux's rather than a description of what elfuse happens to do, and it kills an unfixed elfuse outright. Also from the review round: the two stream copiers retried a write they cannot abandon without pausing, and forbade the SVC restart even when the input rewind had succeeded, turning a restartable sendfile or splice into a guest-visible EINTR. F_SETFL now records the settable bits macOS cannot answer for, O_DIRECT and O_NOATIME, which F_GETFL had started reading from the shadow that nothing wrote. /dev/random joins /dev/urandom as a random device, so it keeps O_ASYNC the way Linux does through random_fasync. The asyncudata mutation for the generation reduction was equivalent to the original -- multiplying by the fd span already discards the bits the reduction would have -- so the target was proving an identity rather than rejecting a defect. It reduces by the wrong span now.
There was a problem hiding this comment.
3 issues found across 12 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/utils.h">
<violation number="1" location="src/utils.h:343">
P2: When another same-UID process renames the temp entry before this unlink, `unlink(path)` returns `ENOENT` although the returned fd remains linked under the new name. Check `fstat(fd).st_nlink == 0` before accepting `ENOENT`; otherwise these callers receive a supposedly anonymous file that remains discoverable and readable.</violation>
</file>
<file name="src/syscall/io.c">
<violation number="1" location="src/syscall/io.c:611">
P3: When `io_xfer` is used by `splice_drain_chunk`, this queues `SIGPIPE` for a partial write followed by `EPIPE`, but the outer drain continues with the unsent tail and queues `SIGPIPE` again when its next write returns `EPIPE`. Track whether the enclosing transfer already reported the pipe break, or centralize this notification, so one guest splice cannot produce duplicate SIGPIPE/signalfd events.</violation>
</file>
<file name="src/syscall/fs.c">
<violation number="1" location="src/syscall/fs.c:1601">
P2: On a real socket or pipe fd, F_SETFL(O_DIRECT) now returns success and F_GETFL reports O_DIRECT, where Linux returns EINVAL. The synthetic-fd branch above this path rejects O_DIRECT with EINVAL, so the real-fd path is inconsistent with both Linux and the neighboring branch. Only record O_DIRECT in the shadow for fds that can actually accept it (regular files), and return -LINUX_EINVAL for the rest.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| * would not help -- the error is a property of the path or the directory, | ||
| * not a transient of this call, and EINTR is already handled above. | ||
| */ | ||
| if (rc < 0 && errno != ENOENT) { |
There was a problem hiding this comment.
P2: When another same-UID process renames the temp entry before this unlink, unlink(path) returns ENOENT although the returned fd remains linked under the new name. Check fstat(fd).st_nlink == 0 before accepting ENOENT; otherwise these callers receive a supposedly anonymous file that remains discoverable and readable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/utils.h, line 343:
<comment>When another same-UID process renames the temp entry before this unlink, `unlink(path)` returns `ENOENT` although the returned fd remains linked under the new name. Check `fstat(fd).st_nlink == 0` before accepting `ENOENT`; otherwise these callers receive a supposedly anonymous file that remains discoverable and readable.</comment>
<file context>
@@ -332,7 +332,15 @@ static inline int tmpfile_anon(const char *what)
+ * would not help -- the error is a property of the path or the directory,
+ * not a transient of this call, and EINTR is already handled above.
+ */
+ if (rc < 0 && errno != ENOENT) {
int unlink_errno = errno;
close(fd);
</file context>
| * below. Measured before this: Linux takes O_NOATIME from 0 to 1 and | ||
| * back, elfuse stayed at 0 throughout. | ||
| */ | ||
| int shadow_setfl = (LINUX_O_DIRECT | LINUX_O_NOATIME) & |
There was a problem hiding this comment.
P2: On a real socket or pipe fd, F_SETFL(O_DIRECT) now returns success and F_GETFL reports O_DIRECT, where Linux returns EINVAL. The synthetic-fd branch above this path rejects O_DIRECT with EINVAL, so the real-fd path is inconsistent with both Linux and the neighboring branch. Only record O_DIRECT in the shadow for fds that can actually accept it (regular files), and return -LINUX_EINVAL for the rest.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/syscall/fs.c, line 1601:
<comment>On a real socket or pipe fd, F_SETFL(O_DIRECT) now returns success and F_GETFL reports O_DIRECT, where Linux returns EINVAL. The synthetic-fd branch above this path rejects O_DIRECT with EINVAL, so the real-fd path is inconsistent with both Linux and the neighboring branch. Only record O_DIRECT in the shadow for fds that can actually accept it (regular files), and return -LINUX_EINVAL for the rest.</comment>
<file context>
@@ -1584,6 +1591,19 @@ int64_t sys_fcntl(guest_t *g, int fd, int cmd, uint64_t arg)
+ * below. Measured before this: Linux takes O_NOATIME from 0 to 1 and
+ * back, elfuse stayed at 0 throughout.
+ */
+ int shadow_setfl = (LINUX_O_DIRECT | LINUX_O_NOATIME) &
+ ~fd_host_flag_mask(fd_snap.type);
+ if (shadow_setfl)
</file context>
| * round that ended the loop says so here. Measured against qemu-aarch64: | ||
| * both sides return the same partial count, and only Linux signalled. | ||
| */ | ||
| if (!is_read && total > 0 && xfer_ret < 0 && xfer_errno == EPIPE) |
There was a problem hiding this comment.
P3: When io_xfer is used by splice_drain_chunk, this queues SIGPIPE for a partial write followed by EPIPE, but the outer drain continues with the unsent tail and queues SIGPIPE again when its next write returns EPIPE. Track whether the enclosing transfer already reported the pipe break, or centralize this notification, so one guest splice cannot produce duplicate SIGPIPE/signalfd events.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/syscall/io.c, line 611:
<comment>When `io_xfer` is used by `splice_drain_chunk`, this queues `SIGPIPE` for a partial write followed by `EPIPE`, but the outer drain continues with the unsent tail and queues `SIGPIPE` again when its next write returns `EPIPE`. Track whether the enclosing transfer already reported the pipe break, or centralize this notification, so one guest splice cannot produce duplicate SIGPIPE/signalfd events.</comment>
<file context>
@@ -585,6 +601,16 @@ int64_t io_xfer(int fd,
+ * round that ended the loop says so here. Measured against qemu-aarch64:
+ * both sides return the same partial count, and only Linux signalled.
+ */
+ if (!is_read && total > 0 && xfer_ret < 0 && xfer_errno == EPIPE)
+ signal_queue(LINUX_SIGPIPE);
+
</file context>
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.
Every blocking-capable read and write used to poll for readiness and then make a blocking host call. That poll reserves nothing: between it and the transfer a sibling thread or a forked process sharing the open file description can take the bytes it promised, and the call that follows parks the vCPU thread where neither hv_vcpus_exit nor the wakeup pipe reaches it. An execve teardown then counts that thread as a sibling that would not leave and takes the post-PNR exit(128) instead of running the new image.
elfuse now owns O_NONBLOCK on every fd whose transfer could park, and emulates the guest's blocking semantics on top of it. io_xfer is the one way to run such a transfer: it attempts the move, and waits interruptibly only when the move reports EAGAIN. The wait no longer consumes the process-wide futex interrupt one-shot, which is raised when the last clone-thread exits and belongs to futex and poll waiters; consuming it there truncated a 1 MiB blocking write to one pipe buffer with no signal involved.
Owning the flag makes fd_entry_t.linux_flags the guest's view of it, so F_GETFL, F_SETFL, FIONBIO, the transfer paths and every synthetic reader answer from one place. Two consequences follow. An fd that aliases an existing description has to inherit that description's answers rather than probe for its own, which fd_alias_spec_t and its constructors now state at each of the seven sites that build one. And F_GETFL becomes shadow bits plus the bits fd_host_flag_mask says the host still owns, replacing a chain of eight per-type exceptions.
Checking that chain against qemu-aarch64 found seven types reporting O_ASYNC that Linux clears: it lands FASYNC only through file_operations->fasync, and SETFL_MASK does not carry it.
The guardrail grew the lanes that would have caught this class of regression: a per-transfer detector, a bulk lane, and lanes that run with a sibling thread alive, since fd-table reads skip their lock while only one thread is active and every earlier lane measured that path alone.
Summary by cubic
Prevents vCPU parking by owning O_NONBLOCK on blocking-capable fds and emulating guest blocking; also stops host SIGPIPE from killing the VM. Old behavior polled then issued a blocking host call that could hang and let SIGPIPE terminate the process; new behavior runs transfers nonblocking and waits only on EAGAIN, preserves the futex one‑shot, and returns EPIPE while queuing a guest SIGPIPE (including after partial writes).
FD_DESCRIPTION_FLAGSunifies description-owned bits; F_GETFL answers as shadow + host-owned mask (now includes O_NOATIME). Aliases inherit viafd_alias_spec_tand preserveofd_id;foreign_descriptionandnonblock_ownedpropagate across dup, fork IPC, proc-fd magic links, epoll dup, and connect upgrades. Fasync is object-based (/dev/urandomand/dev/randomkeep O_ASYNC;/dev/nulland/dev/zerodrop it).io_xfergates blocking-capable I/O; waits only on EAGAIN without consuming the futex interrupt. Sockets pass per-call MSG_DONTWAIT.sendfile/copy_file_range/spliceadjust restartability and add backoff after repeated ready/EAGAIN. Nonblocking short socket writes return immediately. SIGIO udata packing andiov_advance_indexare proven.connectupgrades preserveofd_id. Fork IPC payload carries the new alias flags;FORK_IPC_PROTOCOL_MAGICbumped.tmpfile_anon()replaces open-then-unlink patterns. EINTR checker gains copy/splice rules; verify and mutants cover the new proofs. Guardrail adds transfer and multithreaded lanes.Tests and rollout
test-pipe-steal,test-stdio-nonblock-host,test-socket-shortwrite,test-fcntl-flags,test-sigpipe; guardrail adds nonblocking-read, pipe round-trip, bulk, and multithreaded lanes.Written for commit 3aefcb1. Summary will update on new commits.