vlib: SCHED node runtime inherits stale `stop_timer_handle_plus_1` to workers, asserts on per-worker `vlib_node_schedule()`

[Xcc-git]

Summary

start_workers() in src/vlib/threads.c deep-copies main's per-node-type
runtime arrays — including nodes_by_type[VLIB_NODE_TYPE_SCHED] — into
each worker via vec_dup_aligned(). The
vlib_node_runtime_t::stop_timer_handle_plus_1 field, however, is an
index into the per-thread timer wheel; main's value has no meaning
for any worker. If a plugin schedules a SCHED node from a
VLIB_INIT_FUNCTION and then re-schedules it from its
VLIB_WORKER_INIT_FUNCTION, the worker-side vlib_node_schedule() trips
its ASSERT (vlib_node_is_scheduled (vm, node_index) == 0) on the
inherited stale handle and VPP aborts.

Affected version

Observed on master. The state-inheritance has existed since the SCHED
node type was introduced — see start_workers() runtime-fork loop.

Reproduction

Minimal plugin shape:

static clib_error_t *
demo_init (vlib_main_t *vm)
{
  demo_main_t *dm = &demo_main;
  vlib_node_t *n = vlib_get_node_by_name (vm, (u8 *)"demo-sched");
  dm->sched_node_index = n->index;
  vlib_node_schedule (vm, dm->sched_node_index, 0.05);
  return 0;
}
VLIB_INIT_FUNCTION (demo_init);

static clib_error_t *
demo_worker_init (vlib_main_t *vm)
{
  demo_main_t *dm = &demo_main;
  vlib_node_schedule (vm, dm->sched_node_index, 0.05);
  return 0;
}
VLIB_WORKER_INIT_FUNCTION (demo_worker_init);

VLIB_REGISTER_NODE (demo_sched_node) = {
  .function     = demo_sched_dispatch,
  .name         = "demo-sched",
  .type         = VLIB_NODE_TYPE_SCHED,
  .vector_size  = sizeof (u32),
};

Boot the resulting VPP with cpu { workers 1 } (or any positive worker
count).

Expected

Each thread (main + every worker) has the SCHED node scheduled on its
own timer wheel.  Aging / per-thread periodic work proceeds.

Actual

node_funcs.h:283 (vlib_node_schedule) assertion
    `vlib_node_is_scheduled (vm, node_index) == 0' fails
...
#6  vlib_node_schedule + 0xf8
#7  demo_worker_init + 0x67
#8  call_init_exit_functions_internal
#9  vlib_call_init_exit_functions_no_sort
#10 vlib_worker_thread_fn
#11 vlib_worker_thread_bootstrap_fn
#12 start_thread

VPP aborts during worker startup.

Root cause

src/vlib/threads.c — inside the per-thread runtime fork loop in
start_workers() (around L746-L767 on master):

foreach_int (nt, VLIB_NODE_TYPE_INTERNAL,
             VLIB_NODE_TYPE_PRE_INPUT, VLIB_NODE_TYPE_INPUT,
             VLIB_NODE_TYPE_SCHED)
  {
    ...
    nm_clone->nodes_by_type[nt] = vec_dup_aligned (
        nm->nodes_by_type[nt], CLIB_CACHE_LINE_BYTES);
    ...
    vec_foreach (rt, nm_clone->nodes_by_type[nt])
      {
        vlib_node_t *n = vlib_get_node (vm, rt->node_index);
        /* copy initial runtime_data from node */
        if (n->runtime_data && n->runtime_data_bytes > 0)
          clib_memcpy (rt->runtime_data, n->runtime_data, ...);
      }
  }

vec_dup_aligned() copies the entire vlib_node_runtime_t, including
stop_timer_handle_plus_1.  The fork loop explicitly handles
runtime_data re-init but leaves stop_timer_handle_plus_1 at main's
value.

vlib_node_schedule() in src/vlib/node_funcs.h then asserts:

always_inline void
vlib_node_schedule (vlib_main_t *vm, u32 node_index, f64 dt)
{
  vlib_node_runtime_t *rt = vlib_node_get_runtime (vm, node_index);
  ...
  ASSERT (vm == vlib_get_main ());
  ASSERT (vlib_node_is_scheduled (vm, node_index) == 0);
  ...
}

with vlib_node_is_scheduled() returning
rt->stop_timer_handle_plus_1 ? 1 : 0.  Since the worker inherited a
non-zero value from main, the assert fires before any actual scheduling
runs.

Note that even if the assert were silenced, the inherited handle is an
index into main's tw_timer wheel — calling
vlib_node_unschedule() from the worker would use that handle against
the worker's wheel and corrupt it.

Why this hasn't been hit upstream

$ grep -rln "VLIB_NODE_TYPE_SCHED" src/plugins src/vnet
src/plugins/snort/main.c

- src/plugins/snort/main.c uses SCHED in interrupt-driven mode
(VLIB_NODE_STATE_INTERRUPT + vlib_node_set_interrupt_pending)
and never calls vlib_node_schedule() — so it never writes
stop_timer_handle_plus_1.
- Other periodic / aging-style nodes
(ip{4,6}_full_reass, NAT, etc.) use VLIB_NODE_TYPE_PROCESS
  - vlib_process_wait_for_event_or_clock instead of SCHED.

No in-tree code exercises "schedule from VLIB_INIT_FUNCTION AND from
VLIB_WORKER_INIT_FUNCTION", which is the pattern that triggers this.

Proposed fix

Reset the per-thread handle in the worker runtime fork loop, alongside
the existing runtime_data init:

--- a/src/vlib/threads.c
+++ b/src/vlib/threads.c
@@ -763,6 +763,12 @@ start_workers (vlib_main_t * vm)
                       clib_memcpy (rt->runtime_data, n->runtime_data,
                                    clib_min (VLIB_NODE_RUNTIME_DATA_SIZE,
                                              n->runtime_data_bytes));
+                     /* stop_timer_handle_plus_1 indexes a per-thread
+                      * timer wheel; main's value is meaningless to
+                      * this worker.  Reset so a worker-side
+                      * vlib_node_schedule () does not trip the
+                      * "already scheduled" assert on a stale
+                      * main-thread handle. */
+                     rt->stop_timer_handle_plus_1 = 0;
                    }
                }

Single u32 write in a worker startup path (runs once per worker at
boot); zero runtime overhead.  Cannot break any existing user: any code
that relied on inheriting this handle was reading an index into main's
timer wheel rather than its own, which is incorrect by construction.

Environment
- Linux x86_64, debug build (assertion only fires in debug, but the
underlying state corruption exists in release too — a release-build
worker would silently skip the schedule call and any subsequent
vlib_node_unschedule() would corrupt the worker's tw_timer wheel)