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 <[email protected]> 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. > 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 [ ... ] > +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? [ ... ] > +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. [ ... ] > +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. [ ... ] > +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/[email protected]?part=11
