Re: [PATCH v5 6/9] rv: Add tlob hybrid automaton monitor

[sashiko-bot@kernel.org]

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 <wen.yang@linux.dev>
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/cover.1787161646.git.wen.yang@linux.dev?part=6