Kernel: Do not cancel stale timers when servicing sys$alarm

The sys$alarm() syscall has logic to cache a m_alarm_timer to avoid
allocating a new timer for every call to alarm. Unfortunately that
logic was broken, and there were conditions in which we could have
a timer allocated, but it was no longer on the timer queue, and we
would attempt to cancel that timer again resulting in an infinite
loop waiting for the timers callback to fire.

To fix this, we need to track if a timer is currently in use or not,
allowing us to avoid attempting to cancel inactive timers.

Luke and Tom did the initial investigation, I just happened to have
time to write a repro and attempt a fix, so I'm adding them as the
as co-authors of this commit.

Co-authored-by: Luke <[email protected]>
Co-authored-by: Tom <[email protected]>

Kernel/Syscalls/alarm.cpp

@@ -15,13 +15,14 @@ KResultOr<FlatPtr> Process::sys$alarm(unsigned seconds)
  REQUIRE_PROMISE(stdio);
  unsigned previous_alarm_remaining = 0;
  if (m_alarm_timer) {
- if (TimerQueue::the().cancel_timer(*m_alarm_timer)) {
+ bool was_in_use = false;
+ if (TimerQueue::the().cancel_timer(*m_alarm_timer, &was_in_use)) {
  // The timer hasn't fired. Round up the remaining time (if any)
  Time remaining = m_alarm_timer->remaining() + Time::from_nanoseconds(999'999'999);
  previous_alarm_remaining = remaining.to_truncated_seconds();
  }
  // We had an existing alarm, must return a non-zero value here!
- if (previous_alarm_remaining == 0)
+ if (was_in_use && previous_alarm_remaining == 0)
  previous_alarm_remaining = 1;
  }
 

Kernel/TimerQueue.cpp

@@ -90,6 +90,7 @@ void TimerQueue::add_timer_locked(NonnullRefPtr<Timer> timer)
 
  timer->clear_cancelled();
  timer->clear_callback_finished();
+ timer->set_in_use();
 
  auto& queue = queue_for_timer(*timer);
  if (queue.list.is_empty()) {
@@ -189,12 +190,23 @@ bool TimerQueue::cancel_timer(TimerId id)
  return true;
 }
 
-bool TimerQueue::cancel_timer(Timer& timer)
+bool TimerQueue::cancel_timer(Timer& timer, bool* was_in_use)
 {
+ bool in_use = timer.is_in_use();
+ if (was_in_use)
+ *was_in_use = in_use;
+
+ // If the timer isn't in use, the cancellation is a no-op.
+ if (!in_use) {
+ VERIFY(!timer.m_list_node.is_in_list());
+ return false;
+ }
+
  bool did_already_run = timer.set_cancelled();
  auto& timer_queue = queue_for_timer(timer);
-
  if (!did_already_run) {
+ timer.clear_in_use();
+
  ScopedSpinLock lock(g_timerqueue_lock);
  if (timer_queue.list.contains(timer)) {
  // The timer has not fired, remove it
@@ -218,7 +230,7 @@ bool TimerQueue::cancel_timer(Timer& timer)
  while (!timer.is_callback_finished())
  Processor::wait_check();
 
- return true;
+ return false;
 }
 
 void TimerQueue::remove_timer_locked(Queue& queue, Timer& timer)
@@ -265,6 +277,7 @@ void TimerQueue::fire()
  ScopedSpinLock lock(g_timerqueue_lock);
  m_timers_executing.remove(*timer);
  }
+ timer->clear_in_use();
  timer->set_callback_finished();
  // Drop the reference we added when queueing the timer
  timer->unref();

Kernel/TimerQueue.h

@@ -45,6 +45,7 @@ class Timer : public RefCounted<Timer> {
  Function<void()> m_callback;
  Atomic<bool> m_cancelled { false };
  Atomic<bool> m_callback_finished { false };
+ Atomic<bool> m_in_use { false };
 
  bool operator<(const Timer& rhs) const
  {
@@ -58,11 +59,18 @@ class Timer : public RefCounted<Timer> {
  {
  return m_id == rhs.m_id;
  }
+
  void clear_cancelled() { return m_cancelled.store(false, AK::memory_order_release); }
  bool set_cancelled() { return m_cancelled.exchange(true, AK::memory_order_acq_rel); }
+
+ bool is_in_use() { return m_in_use.load(AK::memory_order_acquire); };
+ void set_in_use() { m_in_use.store(true, AK::memory_order_release); }
+ void clear_in_use() { return m_in_use.store(false, AK::memory_order_release); }
+
  bool is_callback_finished() const { return m_callback_finished.load(AK::memory_order_acquire); }
  void clear_callback_finished() { m_callback_finished.store(false, AK::memory_order_release); }
  void set_callback_finished() { m_callback_finished.store(true, AK::memory_order_release); }
+
  Time now(bool) const;
 
  bool is_queued() const { return m_list_node.is_in_list(); }
@@ -83,7 +91,7 @@ class TimerQueue {
  bool add_timer_without_id(NonnullRefPtr<Timer>, clockid_t, const Time&, Function<void()>&&);
  TimerId add_timer(clockid_t, const Time& timeout, Function<void()>&& callback);
  bool cancel_timer(TimerId id);
- bool cancel_timer(Timer&);
+ bool cancel_timer(Timer& timer, bool* was_in_use = nullptr);
  bool cancel_timer(NonnullRefPtr<Timer>&& timer)
  {
  return cancel_timer(*move(timer));