Skip to content

fix: classify KVM_RUN failures by whether the timer fired, not was armed #92

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
perbu wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: classify KVM_RUN failures by whether the timer fired, not was armed #92

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions lib/tinykvm/arm64/vcpu_run.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,19 +129,29 @@ void vCPU::disable_timer()
long vCPU::run_once()
{
int result;
int run_errno = 0;
{
ScopedProfiler<MachineProfiling::VCpuRun> prof(machine().profiling());
this->flush_registers();
result = ioctl(this->fd, KVM_RUN, 0);
/* Before invalidate_register_cache() or the profiler's destructor can
make a syscall of their own and overwrite it. */
run_errno = errno;
this->invalidate_register_cache();
}
if (UNLIKELY(result < 0)) {
if (this->timer_ticks) {
/* Did the timer *fire*, not was one armed: timer_ticks holds the
configured timeout in milliseconds, so testing it alone would relabel
every KVM_RUN failure inside a timed run (an EFAULT from protected or
unbacked host memory in particular) as a timeout and discard errno.
See the same reasoning in the amd64 vcpu_run.cpp. */
const bool timer_armed = (this->timer_ticks != 0);
if (timer_armed && (timer_was_triggered || run_errno == EINTR)) {
Machine::timeout_exception("Timeout Exception", this->timer_ticks);
} else if (errno == EINTR) {
} else if (run_errno == EINTR) {
Machine::timeout_exception("Interrupted (signal)", 0);
}
Machine::machine_exception("KVM_RUN failed (errno)", errno);
Machine::machine_exception("KVM_RUN failed (errno)", run_errno);
}
if (this->timer_ticks && UNLIKELY(timer_was_triggered)) {
Machine::timeout_exception("Timeout Exception", this->timer_ticks);
Expand Down
26 changes: 22 additions & 4 deletions lib/tinykvm/vcpu_run.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,20 +99,38 @@ void vCPU::disable_timer()
long vCPU::run_once()
{
int result;
int run_errno = 0;
{
ScopedProfiler<MachineProfiling::VCpuRun> prof(machine().profiling());
result = ioctl(this->fd, KVM_RUN, 0);
/* Captured here, not below: the profiler's destructor runs at the end of
this scope and is entitled to make syscalls of its own, any one of
which would overwrite errno before it is read. */
run_errno = errno;
}
// Handle potential KVM_RUN failure or execution timeout
if (UNLIKELY(result < 0)) {
if (this->timer_ticks) {
/* Whether this is a timeout is decided by whether the timer *fired*, not
by whether one was armed. `timer_ticks` is the configured timeout in
milliseconds (vCPU::run() sets it from its argument), so testing it
alone relabels every KVM_RUN failure inside a timed run - an EFAULT
from a guest touch of unbacked or protected host memory in particular -
as "Timeout Exception", and discards errno with it.
The timer manifests two ways, both accepted here: the SIGUSR2 handler
sets the thread_local timer_was_triggered, and the signal makes KVM_RUN
return EINTR. Either one, with a timer armed, is a genuine timeout;
fa-serve's request deadline handling depends on it still being a
MachineTimeoutException. Anything else keeps its errno and is reported
as what it was. */
const bool timer_armed = (this->timer_ticks != 0);
if (timer_armed && (timer_was_triggered || run_errno == EINTR)) {
if constexpr (VERBOSE_TIMER) {
printf("Timer %p triggered\n", timer_id);
}
Machine::timeout_exception("Timeout Exception", this->timer_ticks);
} else if (errno == EINTR) {
} else if (run_errno == EINTR) {
Machine::timeout_exception("Interrupted (signal)", 0);
} else if (errno == EFAULT) {
} else if (run_errno == EFAULT) {
#ifdef KVM_EXIT_MEMORY_FAULT
if (kvm_run->exit_reason == KVM_EXIT_MEMORY_FAULT) {
// This is a memory fault, we can throw a MemoryException
Expand All @@ -123,7 +141,7 @@ long vCPU::run_once()
#endif
Machine::machine_exception("KVM_RUN failed with EFAULT, but exit_reason is unknown\n", kvm_run->exit_reason);
} else {
Machine::machine_exception("KVM_RUN failed (errno)", errno);
Machine::machine_exception("KVM_RUN failed (errno)", run_errno);
}
} else if (this->timer_ticks) {
// Occasionally we miss timer interruptions, and we must catch it via TLS.
Expand Down
Loading