Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider: - [Critical] Relying on the empty stub `ha_cancel_timer_sync()` leaves the budget timer active when windows close, causing Use-After-Free panics on timer expiry. - [High] Concurrent task exit during `tlob_unbind_reap()` causes Use-After-Free and list corruption. - [High] `tlob_destroy_task()` dereferences `ha_mon` outside of the RCU read-side critical section, causing a Use-After-Free. - [High] The clock anchor `ha_mon->env_store[clk_elapsed_tlob]` is never initialized, causing immediate hrtimer firing due to integer underflow. - [High] `tlob_destroy_task()` uses a PID lookup to find the monitor, risking UAF and corruption of an unrelated task's monitor due to PID recycling. - [High] Dangling pointer in the RV framework hash table leading to UAF if `da_handle_start_run_event` fails. - [Medium] Unbinding uprobes while a task is actively monitored permanently leaks its mempool slot. - [Medium] Missing initialization of the `stopping` flag on fresh allocations allows sched tracepoints to trigger invalid monitor transitions. --
--- Patch [6]: [PATCH v5 6/9] rv: Add tlob hybrid automaton monitor --- commit 2dbaa3c57b50d6013814617edf9ea93aed73291d Author: Wen Yang <[email protected]> Subject: rv: Add tlob hybrid automaton monitor This commit adds tlob (task latency over budget), a per-task hybrid automaton RV monitor. It tracks wall-clock time across a user-delimited code section and emits an error when elapsed time exceeds a configurable budget. > diff --git a/kernel/trace/rv/monitors/tlob/tlob.c > b/kernel/trace/rv/monitors/tlob/tlob.c > --- /dev/null > +++ b/kernel/trace/rv/monitors/tlob/tlob.c [ ... ] > @@ -218,6 +218,22 @@ static inline void ha_setup_invariants(struct ha_monitor > *ha_mon, > enum states curr_state, enum events > event, > enum states next_state, u64 time_ns) > { [ ... ] > + if (atomic_read_acquire(&ha_get_target(ha_mon)->stopping)) { > + /* > + * Restart (stopped -> running): arm the timer, then clear > + * stopping so __tlob_acc() admits sched events only once the > + * state is already running_tlob. _release pairs with the > + * acquires in __tlob_acc/tlob_reset_notify. > + */ > + if (next_state < state_max_tlob) > + ha_start_timer_ns(ha_mon, clk_elapsed_tlob, > BUDGET_NS(ha_mon), time_ns); > + atomic_set_release(&ha_get_target(ha_mon)->stopping, 0); > + return; > + } > + > + if (next_state < state_max_tlob) > + ha_start_timer_ns(ha_mon, clk_elapsed_tlob, BUDGET_NS(ha_mon), > time_ns); [Severity: High] Does this path forget to initialize the clock anchor? ha_setup_invariants() does not seem to assign ha_mon->env_store[clk_elapsed_tlob] = time_ns here. Because it remains at the uninitialized sentinel value, won't the elapsed time calculation underflow and cause the relative hrtimer to fire immediately upon window start? > + else > + ha_cancel_timer(ha_mon); > +} [ ... ] > @@ -480,6 +480,41 @@ static int tlob_start_task(struct task_struct *task, u64 > threshold_ns, void *bin > { [ ... ] > + ws = tlob_ws_alloc(); > + if (!ws) > + return -ENOSPC; > + > + ws->task = task; [Severity: Medium] Should the stopping flag be initialized to 1 here? For a fresh allocation, tlob_ws_alloc() zeroes the memory, leaving ws->stopping = 0. If a scheduler event happens during start before ha_setup_invariants() is reached, won't __tlob_acc() allow it through while in stopped_tlob, causing an invalid transition and monitor reset? > + get_task_struct(task); > + ws->threshold_ns = threshold_ns; > + ws->last_ts = ktime_get(); > + raw_spin_lock_init(&ws->entry_lock); > + ws->binding = binding; > + if (binding) > + list_add_tail(&ws->started_node, > + &((struct tlob_uprobe_binding > *)binding)->started_list); > + > + /* Dispatch failed (pool exhausted or monitor disabled): unwind the > slot. */ > + if (!da_handle_start_run_event(task->pid, ws, start_tlob)) { > + if (binding) > + list_del_init(&ws->started_node); > + put_task_struct(task); > + tlob_ws_direct_return(ws); > + return -ENOSPC; > + } [Severity: High] By directly freeing ws via tlob_ws_direct_return(ws) without calling da_destroy_storage(task->pid), does this leave a dangling pointer in the framework hash table? If da_handle_start_run_event() fails, it appears it has already allocated storage in the RCU-protected da_monitor_ht hash table. Subsequent tracepoints might dereference this freed memory. > + > + return 0; > +} [ ... ] > @@ -623,20 +623,32 @@ static void tlob_destroy_task(struct task_struct *task) > { > + struct ha_monitor *ha_mon; > + struct tlob_task_state *ws; > + > + scoped_guard(rcu) { > + ha_mon = ha_get_monitor(task->pid, NULL); [Severity: High] Is it safe to look up the monitor by task->pid here? If tlob_unbind_reap() delays calling tlob_destroy_task() for a task that has already exited, the original PID might be recycled and assigned to a new process. If this new process is also monitored by tlob, could this blindly claim and destroy the new process's monitor while leaking the original task state? > + if (!ha_mon) > + return; > + ws = ha_get_target(ha_mon); > + if (WARN_ON_ONCE(!ws)) > + return; > + if (atomic_cmpxchg_release(&ws->destroying, 0, 1) != 0) > + return; > + } > + > + tlob_detach_from_binding(ws); > + > + /* Force the window ended: @task may never have reached STOP or a > timer. */ > + atomic_set(&ws->stopping, 1); > + ha_cancel_timer_sync(ha_mon); [Severity: High] Is ha_mon still safe to dereference after the RCU read-side critical section ends? The scoped_guard(rcu) block closes just above, dropping the RCU lock. If the monitor is concurrently disabled, ha_monitor_destroy() issues synchronize_rcu() and frees the monitor storage. Could the storage be freed before the teardown calls on ha_mon execute here? [Severity: Critical] Is this stub definition intentional? If ha_cancel_timer_sync() is defined as an empty inline stub in include/rv/ha_monitor.h, it seems the hrtimer is never actually dequeued. When tlob_destroy_task() subsequently frees the monitor storage, could the still-active timer fire and execute its callback on freed memory? > + > + scoped_guard(rcu) { > + da_monitor_reset(&ha_mon->da_mon); > + } > + da_destroy_storage(task->pid); [ ... ] > @@ -748,16 +748,22 @@ static void tlob_unbind_reap(struct tlob_uprobe_binding > *b) > { > + struct tlob_task_state *ws, *tmp; > + LIST_HEAD(to_destroy); > + > + scoped_guard(spinlock, &tlob_ws_lock) { > + list_for_each_entry_safe(ws, tmp, &b->started_list, > started_node) { > + list_del_init(&ws->started_node); > + ws->binding = NULL; > + if (atomic_read(&ws->stopping)) > + list_add_tail(&ws->started_node, &to_destroy); > + } > + } [Severity: Medium] Does this sequence permanently leak the mempool slot for active tasks? If an active task is unbound, its ws->binding is set to NULL but it is not added to to_destroy. Since the uprobe is unregistered, the stop probe never fires. Even if it could, tlob_stop_task() returns -EALREADY because ws->binding is NULL. Will the task remain in running_tlob forever, leaking its pool slot until the task exits? > + > + list_for_each_entry_safe(ws, tmp, &to_destroy, started_node) { [Severity: High] Can concurrent task exits cause list corruption and use-after-free here? tlob_unbind_reap() populates to_destroy and clears ws->binding under lock, then drops the lock and iterates to_destroy locklessly. If a task exits concurrently: handle_sched_process_exit() tlob_destroy_task() tlob_detach_from_binding() Because ws->binding is already NULL, tlob_detach_from_binding() returns early without acquiring the lock or calling list_del_init(). tlob_destroy_task() then frees ws, and when tlob_unbind_reap() resumes iterating, it accesses the freed ws object. > + list_del_init(&ws->started_node); > + tlob_destroy_task(ws->task); > + } > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
