Keep epoll interest alive while a duplicate of the registered fd survives - #1230
Will Portnoy (willportnoy) wants to merge 4 commits into
Conversation
…red fd survives Linux epoll(7) removes a file descriptor from an interest list only after every descriptor referring to the underlying open file description has been closed. The shim instead anchored each interest to the per-descriptor `TypedFd`, so closing the registered descriptor dropped the interest even when a `dup` referring to the same open file description remained open: `EpollEntry::poll` bailed on a dead `Weak<TypedFd>` and the readiness was never delivered. Anchor epoll interest to the open file description instead: - Add `WeakEntryHandle` to `litebox::fd`: a durable, dup-surviving weak reference to a descriptor's shared entry, plus `EntryHandle::downgrade`, `as_ptr`, and shared (open-file-description-level) metadata access. - Re-point epoll's `DescriptorRef` at a per-subsystem `WeakEntryHandle`, key interests by the open file description's stable address, and re-poll through the shared entry (eventfd/unix/pipe via the entry's `IOPollable`, socket/file via aliased metadata). Observer registration is unchanged; it already targets the shared pollable. - Expose `with_iopollable` on the pipe entry so a pipe can be polled without a live per-descriptor `PipeFd`. Add tests/epoll_dup.c: register an eventfd, dup it, close the original, and verify the interest still delivers events and survives re-arming. It passes natively, fails without this change under Litebox, and passes with it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1494e366-b3cf-4196-91a0-1430cb9d5cc8
…criptor `close_and_duplicate_if_shared` checked `Arc::strong_count == 1` and then took ownership with `Arc::into_inner(...).unwrap()`. With durable `WeakEntryHandle`s now held by epoll interests, another thread can `upgrade()` the shared entry lock-free (bypassing the descriptor-table lock) between the count check and the unwrap, transiently raising the strong count and panicking the unwrap. Take ownership with `Arc::try_unwrap` instead and, on the error path, put the entry back and return `CloseResult::Duplicated` so the descriptor is closed once the transient reference drops — the same self-healing path already used when the entry is genuinely shared. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1494e366-b3cf-4196-91a0-1430cb9d5cc8
…rvival # Conflicts: # litebox_shim_linux/src/syscalls/epoll.rs
There was a problem hiding this comment.
Thanks Will, I reviewed the changes in the litebox core, since this is changing our API surface. I think some changes are needed to make this more compatible with future evolution. The base of the changes is mostly reasonable, just needs some tweaks.
The changes should be fairly straightforward, but do let me know if you'd prefer I open up a PR with the changes I have in mind to the core, instead of tweaking those things over this PR.
Oh also, please do make sure to keep the PR description human-written, not AI-generated-human-reviewed.
- Replace `EntryHandle`/`WeakEntryHandle` `as_ptr() -> *const ()` with `stable_key() -> EntryStableKey`, an opaque `PartialEq+Eq+Ord+Hash+Copy` identity, and key epoll interests on it instead of a raw address. - Rename `with_shared_metadata` to `with_entry_metadata`. - Unify `EntryHandle`'s `PhantomData` with `WeakEntryHandle`'s (`fn(Subsystem) -> Subsystem`), the form that keeps the handle unconditionally `Send`/`Sync`. - Trim the handle/pipe doc comments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1494e366-b3cf-4196-91a0-1430cb9d5cc8
|
🤖 SemverChecks 🤖 No breaking API changes detected Note: this does not mean API is unchanged, or even that there are no breaking changes; simply, none of the detections triggered. |
|
Jay Bosamiya (Microsoft) (@jaybosamiya-ms) can you please take a look at Will's fixes when you get a chance? Thanks! |
Jay Bosamiya (Microsoft) (jaybosamiya-ms)
left a comment
There was a problem hiding this comment.
Thanks Will for making the updates. There are still a few changes needed:
- The introduction of a weak entry handle means that we need to revisit the assumptions of all existing functions that were previously assuming or only working with strong counts. As far as I can tell,
remove(and/or its callers) break upon a racingupgrade. Specifically, if anupgradeoccurs at just the right time, then it is possible for noremoveto ever return the entry (i.e., it silently gets dropped), which can break other code. However, if we updateremoveto account for this race, not close the descriptor, and keep it in, then a bunch of other code that assumes aremovewill actually give it the entry will break. Some sort of re-designing is needed, and it is unclear what the nicer redesign is. It would take more effort to figure out what the correct design for this is. I'm not necessarily suggesting a change here, but am saying that at least we need some documentation aroundremovethat says// XXX: Due to #1230, a race of a weak handle upgrade might cause a fully missed entryor something similar to that, so that we are at least documenting the potential issue. - Similar, revisiting assumptions of
drain_entries_full_covered_bywhich also can fail due to a race with anupgrade; basically the strong-count snapshot it takes might be invalidated by a racingupgrade. The right move is to update the code to retain the entry when it loses the race, rather than relying entirely on the snapshot. This one is less problematic for callers, since they already have the built-in retry-like logic. So this one we should definitely fix. - The PR description is still AI generated. I'd mentioned this in my prior review too, but please do make sure to keep the PR description human-written, not AI-generated-human-reviewed.
Problem
Linux
epoll(7)removes a descriptor from an interest list only after every descriptor referring to the underlying open file description (OFD) has been closed. The shim instead anchored each interest to the per-descriptorTypedFd, so closing the registered descriptor dropped the interest even when adupreferring to the same OFD was still open —EpollEntry::pollbailed on a deadWeak<TypedFd>and readiness was never delivered.Fix
Anchor epoll interest to the open file description:
WeakEntryHandletolitebox::fd— a durable,dup-surviving weak reference to a descriptor's shared entry, plusEntryHandle::downgrade/as_ptr/with_shared_metadata.DescriptorRefat a per-subsystemWeakEntryHandle, key interests by the OFD's stable address, and re-poll through the shared entry (eventfd/unix/pipe via the entry'sIOPollable, socket/file via aliased metadata). Observer registration is unchanged; it already targets the shared pollable.with_iopollableon the pipe entry so a pipe can be polled without a live per-descriptorPipeFd.Covers all six epoll-able fd types on
main(eventfd, unix, pipe, socket, file, and the pre-existing epoll-on-epollunimplemented!()), via exhaustive matches.Test
tests/epoll_dup.c: register an eventfd,dupit, close the original, and verify the interest still delivers events and survives re-arming. Passes natively, fails without this change under Litebox, and passes with it.