Kernel thread stop reap blocked

[andreykarpenko-qc]

No description provided.

[stzahi1]

Please remove code lines which are in comments

[andreykarpenko-qc]

Done

[Erich Plondke (eplondke)]

This is a great feature, it's something we need pretty badly: the ability to kill another thread, and building on top of that being able to kill a vm/process/etc.

There's some partial support for killing another task and we test it at least somewhat. If we set the H2K_VMSTATUS_KILL bit in a thread it's supposed to wake up and call H2K_thread_stop(). But we currently don't ever set that bit outside of testing! :-) But I think that infrastructure might be the best way to reuse some of the other infrastructure to get a thread killed off regardless of its state.

Conceptually, it seems like then the "vm death" sequence is:

• Mark the VM as "being killed"
• Mark all threads in the VCPU for death.
• Possibly, make sure we don't create new VCPUs in a VM "being killed"
• If the number of VCPUs in the VM goes to zero, we either "signal the parent" (like we might already do?) or "just tear down the VM" as a configured option.
• Hopefully this works if a guest is asking for itself to be torn down, or the parent is tearing down the child.

This seems pretty great, but I'm hopeful that we can leverage the existing infrastructure a little more if it's working. If it's not working... maybe we can fix it... or if it's too broken then we can create something new like this.

Let me know if you want to chat live about this!

[Erich Plondke (eplondke)]

Let's try to avoid this special case

[andreykarpenko-qc]

There is an analyze of the difference

2. Structure & size
 ┌────────────────────┬────────────────────────────────────────┬────────────────────────────────────┐
 │                    │              orig_nohint               │             curr_hint              │
 ├────────────────────┼────────────────────────────────────────┼────────────────────────────────────┤
 │ resched body       │ inlined twice (into both entry points) │ outlined once into .text           │
 ├────────────────────┼────────────────────────────────────────┼────────────────────────────────────┤
 │ Entry points       │ full body each                         │ 4–9-packet stubs that jump to body │
 ├────────────────────┼────────────────────────────────────────┼────────────────────────────────────┤
 │ .text.core.resched │ 0x13c (316 B)                          │ 0x38 (56 B, stubs only)            │
 ├────────────────────┼────────────────────────────────────────┼────────────────────────────────────┤
 │ .text              │ —                                      │ 0xd0 (208 B, shared body)          │
 ├────────────────────┼────────────────────────────────────────┼────────────────────────────────────┤
 │ Total code         │ 316 B                                  │ 264 B ✅                           │
 └────────────────────┴────────────────────────────────────────┴────────────────────────────────────┘

 Despite curr_hint carrying the extra teardown logic, it's 52 B smaller — outlining emits the body once instead of duplicating it into H2K_resched and H2K_resched_cluster.

 2. Hot path (me != NULL, the common reschedule)

 orig_nohint — H2K_resched:
 00  setup / allocframe
 0c  r17 = me ; save r19:18
 10  ciad           ← clear int
 14  k0lock         ← BKL
 18  cmp me==0 ; if(!p0) jump:t 0x30   ← TAKEN to reach body (cold else falls through)
 30  ┐
 ..  │ runlist_remove + ready_append (inlined)
 60  │ call ring_append
 88  ┘ call dosched
 90  restore ; 94 deallocframe
 - 1 conditional branch, taken (correctly hinted :t) to jump over the cold me==NULL code into the body.
 - ~18 packets, 2 calls. Cold wait_mask path is the fall-through at 0x20 (so cold code sits in the middle).

 curr_hint — stub → body:
 stub H2K_resched: r0=me; ciad; k0lock; jump → body   ← 1 unconditional taken jump
 body:
 00  cmp me==0 ; allocframe
 0c  if(p0) jump:nt 0x90   ← NOT taken (me!=0); cold wait_mask now at TAIL 0x90
 18  runlist_remove (inlined)
 38  r0 = me->vmblock      ; if(vmblock==0) jump:t 0x4c
 44  r2 = vmblock->exiting ; if(exiting!=0) jump:nt 0xa0  ← NOT taken; teardown at TAIL 0xa0
 4c  ready_append (inlined)
 5c  call ring_append
 80  call dosched
 88  restore ; 8c deallocframe
 - Conditional branches all fall through in the hot case (predicted :nt), so the hot path is contiguous to dosched.
 - Cost of the feature: +2 loads (vmblock @+0x14, exiting @+0xac) and the guard compares.
 - Cost of outlining: 1 mandatory unconditional jump from stub to body (always-taken, fixed target — trivially predicted).
 - ~23 packets, 2 calls.

[Erich Plondke (eplondke)]

Do we want to tear down the whole VM? Or just put it in all-threads-stopped where the spawning task frees it (current approach)? This is kind of like "zombie" and needing to join in posix? Although also that's a hassle sometimes :-)

[Bryan Bayerdorffer (bryanb-h2)]

Yeah, I don't think that we should have a concept of the "main" thread (vcpu) of a vm. For a paravirtualized guest like linux, the vcpus are just a symmetric set of cpus -- there is no "main" cpu that stops all the others when it stops -- in most architectures anyway -- and I don't think that's something we should introduce in the HVM architecture.
So it's ok (and necessary) to have the concept of a main posix thread, but that shouldn't map to a particular vcpu even though in h2os that's how we implement it.
pthread_exit() of the main thread should kill all the other pthreads, but not (directly) all the vcpus. It's quite possible that there is a vcpu that is not running the code of a pthread.
Therefore I think the mechanisms for "stop all pthreads" and "stop the vm" should be separate, logically, though they can share some infrastructure.

[Erich Plondke (eplondke)]

Thread stop and vm stop should probably be separated (although perhaps one should call the other?)

[Bryan Bayerdorffer (bryanb-h2)]

Yeah that. Should have kept reading...

[Bryan Bayerdorffer (bryanb-h2)]

This is a great feature, it's something we need pretty badly: the ability to kill another thread, and building on top of that being able to kill a vm/process/etc.

There's some partial support for killing another task and we test it at least somewhat. If we set the H2K_VMSTATUS_KILL bit in a thread it's supposed to wake up and call H2K_thread_stop(). But we currently don't ever set that bit outside of testing! :-) But I think that infrastructure might be the best way to reuse some of the other infrastructure to get a thread killed off regardless of its state.

Conceptually, it seems like then the "vm death" sequence is:

* Mark the VM as "being killed"

* Mark all threads in the VCPU for death.

* Possibly, make sure we don't create new VCPUs in a VM "being killed"

* If the number of VCPUs in the VM goes to zero, we either "signal the parent" (like we might already do?) or "just tear down the VM" as a configured option.

* Hopefully this works if a guest is asking for itself to be torn down, or the parent is tearing down the child.

This seems pretty great, but I'm hopeful that we can leverage the existing infrastructure a little more if it's working. If it's not working... maybe we can fix it... or if it's too broken then we can create something new like this.

Let me know if you want to chat live about this!

Currently, when the last vcpu stops we send H2K_VM_CHILDINT to the parent vcpu if it's still alive (!H2K_STATUS_DEAD), then tear down the vm unconditionally. We also signal the parent if any child vcpu exits non-0.
Maybe this should be a shint instead so the parent doesn't have to hang around, but it seemed kind of good practice at the time I added this to expect the parent to wait (blocked) until the child vm finishes. This was intended mainly to wake up booter when stuff finishes.

We can certainly add a vmop to set H2K_VMSTATUS_KILL on a vm (child only) and then interrupt any running vcpus from that vm so that they go do vmwork and discover that they need to terminate.

[Erich Plondke (eplondke)]

The tests here are really awesome. They found some great problems.

[Erich Plondke (eplondke)]

Let's discuss these two PRs esp. the race condition with blocking / bkl

[Erich Plondke (eplondke)]

I think this code has the race condition issue that I ran into on my branch. Let's discuss.

[Erich Plondke (eplondke)]

Also, this function seems to duplicate a lot with other thread cancel functionality, duplicating it is likely to cause problems over time with maintenance.

[andreykarpenko-qc]

`● I have the complete picture. Let me give you the full evidence-based investigation.

  Investigation: the "B stranded on a torn-down vmblock" claim

  I traced the +653 scenario end-to-end in the debugger and compared against the pre-fix master code. My original one-line characterization was directionally 
  right but imprecise. Here is what actually happens.

  What I measured (fixed kernel, +653)

  The reap sequence during A's exit(0) teardown, watched live:

  ┌───────────────────────────────────┬────────────────────────────┬───────────────────────────────────────────────────────────────────────────────────────┐
  │               event               │          context           │                                         path                                          │
  ├───────────────────────────────────┼────────────────────────────┼───────────────────────────────────────────────────────────────────────────────────────┤
  │ A frees itself                    │ me = 0xff015800            │ vm_stop_locked:108 (per-me cleanup)                                                   │
  ├───────────────────────────────────┼────────────────────────────┼───────────────────────────────────────────────────────────────────────────────────────┤
  │ A reaps parked main (BLOCKED on   │ 0xff015a80                 │ vm_stop_locked:113 → reap_one_locked → H2K_futex_cancel + H2K_free_context_locked     │
  │ main_park)                        │                            │                                                                                       │
  ├───────────────────────────────────┼────────────────────────────┼───────────────────────────────────────────────────────────────────────────────────────┤
  │ A scans B (still RUNNING)         │ 0xff015940                 │ reap_one_locked → case RUNNING: return 0 — skipped                                    │
  ├───────────────────────────────────┼────────────────────────────┼───────────────────────────────────────────────────────────────────────────────────────┤
  │ A's finalize                      │ vmblock 0xff013300         │ num_cpus=1, status=0, exiting=1 → guard (num_cpus!=0 && status==0) returns early —    │
  │                                   │                            │ vmblock NOT freed                                                                     │
  ├───────────────────────────────────┼────────────────────────────┼───────────────────────────────────────────────────────────────────────────────────────┤
  │ A releases BKL (via H2K_dosched)  │ —                          │ —                                                                                     │
  ├───────────────────────────────────┼────────────────────────────┼───────────────────────────────────────────────────────────────────────────────────────┤
  │ B acquires BKL, commits BLOCKED   │ 0xff015940, byte@+0xb:     │ futex_classic.S:87                                                                    │
  │                                   │ 0x01→0x03                  │                                                                                       │
  ├───────────────────────────────────┼────────────────────────────┼───────────────────────────────────────────────────────────────────────────────────────┤
  │ B self-reaps                      │ 0xff015940                 │ H2K_dosched → resched() sees me->vmblock->exiting → frees self, clears context (ring  │
  │                                   │                            │ node → self-referential)                                                              │
  └───────────────────────────────────┴────────────────────────────┴───────────────────────────────────────────────────────────────────────────────────────┘

  The critical measured fact: at A's finalize, num_cpus == 1. That single outstanding count is B. The finalize guard therefore does not free the vmblock — B's
  un-reaped slot keeps it alive. This is exactly what makes B's later BLOCKED commit safe: the vmblock is still valid when B touches me->vmblock->exiting.

  The two mechanisms that catch B (this branch adds both)

  1. vmblock->exiting gate in resched() (resched.ref.c:22-29) — new on this branch. When B finally yields via H2K_dosched, resched sees exiting=1 and self-reaps
  B instead of requeuing it, then runs finalize_if_done which now sees num_cpus==0 and completes teardown.
  2. Pass-2 IPI in vm_stop_locked — A fires CLUSTER_RESCHED_INT at every RUNNING peer's HW thread, forcing B into H2K_resched_cluster → same exiting self-reap
  path even if B never voluntarily blocks.

  Either path converges on the same self-reap. I confirmed the VM ends fully torn down (all HW threads in H2K_wait_forever, B's context recycled to the free
  list).

  What actually happens on the pre-fix kernel (master)

  I checked — and my "use-after-free of the vmblock" wording was too strong. On master:
  - There is no H2K_vm_stop, no exiting field, no reap_one_locked, no Pass-2 IPI at all. The whole VM-teardown-on-exit path is new on this branch.
  - resched() on master unconditionally does H2K_ready_append(me); H2K_dosched(me) — no exiting-gate.

  So the real pre-fix failure mode isn't "B dereferences a freed vmblock." It's that exit() had no whole-VM teardown mechanism: a peer blocking on a futex
  during another thread's exit would either be stranded as a permanently BLOCKED thread that's never reaped (missed-wakeup / leaked context — the futex is never
  resumed, no IPI ever comes), or the teardown accounting is simply absent. The UAF risk is the design hazard this branch had to avoid while adding teardown —
  i.e., if teardown freed the vmblock while B was still RUNNING and about to block, B would then touch freed memory. The num_cpus==1 early-return I measured is
  precisely the guard that prevents that.

  Corrected statement

  ▎ B commits to BLOCKED after A's reap scan skips it (B still RUNNING). The fix keeps the vmblock alive across this window (A's finalize early-returns because
  ▎ num_cpus==1 accounts for un-reaped B), and guarantees B is subsequently reaped via the new vmblock->exiting self-reap path in resched — reached either when 
  ▎ B voluntarily blocks or via A's Pass-2 CLUSTER_RESCHED_INT. Without this branch's teardown machinery, that late-blocking peer had no reaper at all: it would
  ▎ leak as a stranded BLOCKED context on a VM that's supposed to be gone.
 `

[Erich Plondke (eplondke)]

can this be moved to be common?

[Erich Plondke (eplondke)]

This allows the process to tear itself down, but doesn't allow you to kill a child VM... that's also a useful thing to be able to do.