portal: periodic poll heartbeat + narrow the sysctl parent-dir guard - #96
Open
lacraig2 wants to merge 2 commits into
Open
portal: periodic poll heartbeat + narrow the sysctl parent-dir guard#96lacraig2 wants to merge 2 commits into
lacraig2 wants to merge 2 commits into
Conversation
handle_op_sysctl_create_file guarded against a pre-5.0 register_sysctl() crash by refusing to create an entry whose parent sysctl directory did not already exist. That guard was far too broad: register_sysctl() creates a genuinely new *sysctl* directory just fine on those kernels. The crash it was protecting against is specific to filesystem-backed, non-sysctl subtrees (/proc/sys/fs/binfmt_misc is the canonical one), where the intermediate component cannot be created, the call returns -EROFS, and the cleanup path then faults in drop_sysctl_table()->rb_erase(). The over-broad check silently dropped legitimate vendor sysctl directories. That is not a cosmetic loss: embedded firmware commonly exposes a bootloader/board environment as its own /proc/sys subtree and then spins in a boot script waiting for it to appear, so dropping the registration hangs the boot forever rather than degrading. Narrow the guard to an explicit filesystem-backed list via a new igloo_sysctl_dir_is_fs_backed() helper, mirroring the Python _NON_SYSCTL_SUBTREES list in hyperfile/sysctl.py (which normally rejects these before they reach the driver; this is the driver-side backstop). igloo_sysctl_dir_exists() is kept but marked __maybe_unused so the traversal helper is still available. Verified on a mipseb/4.10 guest: sysctl-registration failures 3 -> 0, and a boot that previously hung waiting for a vendor sysctl node now proceeds.
Some firmware main loops are driven entirely by a device that delivers a periodic hardware event -- a watchdog being the common case. Such a loop typically sits in epoll_wait() with timeout=-1 on a set containing that device and advances its state machine once per tick. Neither existing poll behaviour models this. Always-ready makes epoll return immediately every iteration, so the loop spins and starves the emulator. Never-ready (the issue #77 wait-queue path) parks the loop forever, so it deadlocks after the last event it was waiting on. Crucially this cannot be fixed host-side: with timeout=-1 the kernel only re-invokes ->poll() after a wake on the device's wait queue, so the periodic wake has to originate from a kernel timer in the driver. Add an optional poll_interval_ms to the devfs create request. When it is > 0 the entry gets a self-rearming timer that sets poll_ready and wakes poll_wq every interval; the poll proxy then reports POLLIN|POLLRDNORM for exactly one poll() per tick (consumed with atomic_xchg) and not-ready in between. POLLOUT is deliberately not reported: a constantly-writable mask would re-spin a loop that also watches EPOLLOUT. 0 keeps the previous behaviour exactly, so this is inert for every existing node. timer_setup()/from_timer() landed in 4.14, so the callback is version-guarded and falls back to setup_timer() with the entry pointer in ->data on older kernels. Built for mipseb/4.10. Verified with a guest whose service manager polls a watchdog: the spin (139k failed-read log lines) and the deadlock both disappear and the loop advances at the configured cadence.
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
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.
Two independent portal fixes, both found while bringing up a firmware whose
service manager is driven by a watchdog heartbeat and which exposes a vendor
bootloader environment under
/proc/sys. Each is generally useful; neither isdevice-specific.
1.
portal_sysctl: refuse only fs-backed subtrees, not every new sysctl dirhandle_op_sysctl_create_fileguarded a pre-5.0register_sysctl()crash byrefusing to create an entry whose parent sysctl directory did not already
exist. That is far too broad —
register_sysctl()creates a genuinely newsysctl directory just fine on those kernels.
The crash it protects against is specific to filesystem-backed, non-sysctl
subtrees (
/proc/sys/fs/binfmt_miscbeing canonical): the intermediate componentcan't be created, the call returns
-EROFS, and the cleanup path then faults indrop_sysctl_table()->rb_erase().The failure mode this caused is worse than a cosmetic drop: embedded firmware
commonly exposes a bootloader/board environment as its own
/proc/syssubtree andthen spins in a boot script waiting for it to appear, so silently dropping the
registration hangs the boot forever instead of degrading.
Fix: narrow the guard to an explicit fs-backed list via a new
igloo_sysctl_dir_is_fs_backed(), mirroring the Python_NON_SYSCTL_SUBTREESlistin
hyperfile/sysctl.py(which normally rejects these before they reach thedriver — this is the driver-side backstop).
igloo_sysctl_dir_exists()is retainedas
__maybe_unused.Verified on a mipseb/4.10 guest: sysctl-registration failures 3 → 0, and a boot
that previously hung waiting for a vendor sysctl node proceeds.
2.
portal_devfs: add an optional periodic poll heartbeat timerSome firmware main loops are driven entirely by a device that delivers a periodic
hardware event (a watchdog is the common case): the loop sits in
epoll_wait()with
timeout=-1on a set containing that device and advances its service statemachine once per tick.
Neither existing poll behaviour models that:
starves the emulator.
deadlocks after the last event it was waiting on.
This cannot be fixed host-side: with
timeout=-1the kernel only re-invokes->poll()after a wake on the device's wait queue, so the periodic wake mustoriginate from a kernel timer in the driver.
Adds an optional
poll_interval_msto the devfs create request. When> 0, theentry gets a self-rearming timer that sets
poll_readyand wakespoll_wqeveryinterval; the poll proxy reports
POLLIN|POLLRDNORMfor exactly onepoll()pertick (consumed via
atomic_xchg) and not-ready in between.POLLOUTis deliberately not reported — a constantly-writable mask wouldre-spin a loop that also watches
EPOLLOUT.poll_interval_ms == 0preserves current behaviour exactly, so this is inertfor every existing node.
timer_setup()/from_timer()landed in 4.14, so the callback isversion-guarded and falls back to
setup_timer()with the entry pointer in->data.Verified with a guest whose service manager polls a watchdog: both the spin
(139k failed-read log lines) and the deadlock disappear, and the loop advances at
the configured cadence.
Build
./build.sh --versions 4.10 --targets mipseb→+++ SUCCESS: mipseb 4.10oncurrent
main.Consumer
The host half (
poll: {model: periodic, interval_ms: N}) is a separate penguin PR;this driver change is what makes it possible. A rehosting that depends on both is
staged as a draft.