Re: [PATCH v7 11/14] rv: Add tlob hybrid automaton monitor
flat view
From: sashiko-bot@kernel.org
Date: 2026-10-11 19:58:08
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Critical] rv: TOCTOU race in ha_setup_invariants allows hrtimer Use-After-Free - [High] rv: deadlock in tlob_binding_lock between process context and RCU softirq - [High] rv: tlob: list corruption due to lockless da_destroy_storage() calls - [Medium] rv: race between tlob_stop_task and timer callback causes INVALID_STATE error -- commit 90ea80dda3503cc5ac796d8401bafbbd97a373b9 Author: Wen Yang [off-list ref] rv: Add tlob hybrid automaton monitor tlob (task latency over budget) is a per-task hybrid automaton RV monitor that tracks wall-clock time across a user-delimited code section and emits an error when the elapsed time exceeds a configurable budget.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
+static inline void tlob_detach_from_binding(struct tlob_record *rec) +{ + guard(spinlock)(&tlob_binding_lock); + + if (!rec->binding) + return; + rec->binding = NULL; + list_del_init(&rec->started_node); +} + +/* + * Per-record teardown during monitor disable. TLOB_RETIRED claims it, so an + * exit or unbind racing disable frees the record once. + */ +static inline void tlob_extra_cleanup(struct da_monitor *da_mon) +{ + struct ha_monitor *ha_mon = to_ha_monitor(da_mon); + struct tlob_record *rec = ha_get_target(ha_mon); + + if (!rec) + return; + + if (atomic_xchg(&rec->state, TLOB_RETIRED) == TLOB_RETIRED) + return; + + tlob_detach_from_binding(rec); + call_rcu(&rec->rcu, tlob_rec_return_cb); +}
[Severity: High] Does this lock acquisition risk a deadlock between process context and RCU softirq? The tlob_binding_lock is acquired in process context functions like tlob_start_task() using scoped_guard(spinlock, &tlob_binding_lock), which does not disable softirqs. During monitor teardown, the RV framework's da_destroy_storage() frees the monitor record via call_rcu(). The RCU callback executes in RCU_SOFTIRQ context and invokes tlob_extra_cleanup(), which calls tlob_detach_from_binding() and acquires the same lock. If a process holding the lock is interrupted by the RCU softirq on the same CPU, the softirq will deadlock attempting to acquire it. Should tlob_binding_lock be acquired using spin_lock_bh() in all process-context paths? [ ... ]
quoted hunk ↗ jump to hunk
+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) +{ + struct tlob_record *rec = ha_get_target(ha_mon); + + if (next_state == stopped_tlob) + return; + + if (next_state >= state_max_tlob) { + ha_cancel_timer(ha_mon); + return; + } + + if (event != start_tlob) { + /* A sched event racing the window end must not re-arm. */ + if (atomic_read_acquire(&rec->state) != TLOB_OPEN) + return; + ha_start_timer_ns(ha_mon, clk_elapsed_tlob, + BUDGET_NS(ha_mon), time_ns); + return; + } + + ha_reset_clk_ns(ha_mon, clk_elapsed_tlob, time_ns); + ha_start_timer_ns(ha_mon, clk_elapsed_tlob, BUDGET_NS(ha_mon), + time_ns); + /* Reopen only a closed record; a teardown claim keeps it closed. */ + atomic_cmpxchg_release(&rec->state, TLOB_CLOSED, TLOB_OPEN); +}
[Severity: Critical] Can this sequence allow a use-after-free of the hrtimer? There appears to be a race between the state check and arming the timer. A teardown path can concurrently claim the record by setting its state to TLOB_RETIRED, cancel the timer, and queue the record for freeing via call_rcu(). If the teardown occurs between ha_setup_invariants() checking the state and calling ha_start_timer_ns(), the timer will be freshly re-armed on a record that has already been queued for destruction. Even in the start_tlob path, ha_start_timer_ns() is called unconditionally before atomic_cmpxchg_release(). If the cmpxchg fails because of a concurrent teardown, the armed timer is never rolled back. Once the RCU grace period elapses, the record is freed. The still-armed timer remains on the hrtimer rbtree, leading to a use-after-free when it expires. [ ... ]
quoted hunk ↗ jump to hunk
+static void tlob_stop_task(struct task_struct *task, + struct tlob_uprobe_binding *binding) +{ + struct ha_monitor *ha_mon; + struct tlob_record *rec; + + scoped_guard(rcu) { + ha_mon = ha_get_monitor(task->pid, NULL); + if (!ha_mon) + return; + + rec = ha_get_target(ha_mon); + if (WARN_ON_ONCE(!rec)) + return; + + /* Only the opener may end it; NULL (task exit) skips. */ + if (binding && READ_ONCE(rec->binding) != binding) + return; + + /* Claim the window end; the timer may have taken it first. */ + if (atomic_cmpxchg_release(&rec->state, TLOB_OPEN, + TLOB_CLOSED) != TLOB_OPEN) + return; + + ha_cancel_timer_sync(ha_mon); + } + + /* The DA returns to stopped; the record stays closed. */ + da_handle_event(task->pid, NULL, stop_tlob); +}
[Severity: Medium] Will this result in an invalid state error if tlob_stop_task() races with the timer callback? tlob_stop_task() claims the window closure and then calls ha_cancel_timer_sync(). If the hrtimer just expired and its callback is running concurrently on another CPU, ha_cancel_timer_sync() spin-waits for it to complete. The generic timer callback will unconditionally emit a budget violation error and reset the automaton to stopped_tlob. After ha_cancel_timer_sync() returns, tlob_stop_task() injects the stop_tlob event. Since the automaton defines stop_tlob from the stopped_tlob state as invalid, this causes the framework to incorrectly flag an invalid transition despite tlob_stop_task() legitimately ending the window. [ ... ]
quoted hunk ↗ jump to hunk
+static void tlob_destroy_claimed(struct tlob_record *rec) +{ + pid_t id = rec->id; + + tlob_detach_from_binding(rec); + + scoped_guard(rcu) { + struct ha_monitor *ha_mon = ha_get_monitor(id, NULL); + + if (!WARN_ON_ONCE(!ha_mon)) + da_monitor_reset(&ha_mon->da_mon); + } + da_destroy_storage(id); + + /* Readers may still hold rec; the pool slot returns after a GP. */ + call_rcu(&rec->rcu, tlob_rec_return_cb); +}
[Severity: High] Does this code corrupt the hash table list pointers? da_monitor_ht is a global hash table without internal locking. The function da_destroy_storage() uses hash_del_rcu(), which modifies hlist pointers and requires the caller to hold a writer lock to serialize against concurrent writers. While tlob_start_task() acquires tlob_binding_lock to serialize additions, tlob_destroy_claimed() and the error path of tlob_start_task() call da_destroy_storage() without holding tlob_binding_lock or any other bucket-level lock. Concurrent task exits or unbind operations that map to the same hash bucket could corrupt the hash table's linked list. -- Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791745088.git.wen.yang@linux.dev?part=11