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

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Critical] Concurrent task exit causes Use-After-Free and list corruption in `tlob_unbind_reap`.
- [High] Race between `tlob_stop_task` claiming the window and the HA timer callback suppresses the detail trace and hides budget violations.
- [High] `tlob_ws_lock` is a `spinlock_t` acquired in tracepoint and uprobe exception contexts, causing sleeping in atomic context on PREEMPT_RT.

Pre-existing issues:
- [Critical] Concurrent lockless calls to `hash_add_rcu` and `hash_del_rcu` on `da_monitor_ht` corrupt the hash bucket lists.
--

--- Patch [6]: [PATCH v6 6/9] rv: Add tlob hybrid automaton monitor ---
commit ecef387f80f071ac66f0456fa83acfd88043286c
Author: Wen Yang <[email protected]>
Subject: rv: Add tlob hybrid automaton monitor

This patch introduces tlob (task latency over budget), a per-task hybrid
automaton RV monitor. It tracks wall-clock time across a user-delimited code
section using uprobes and emits warnings when the elapsed time exceeds a
configured threshold.

> diff --git a/kernel/trace/rv/monitors/tlob/tlob.c b/kernel/trace/rv/monitors/tlob/tlob.c
> new file mode 100644
> index 0000000000000..08b1bee884cc4
> --- /dev/null
> +++ b/kernel/trace/rv/monitors/tlob/tlob.c

[ ... ]

> +/* Serialises tlob_task_state ownership: restart, detach, unbind reap. */
> +static DEFINE_SPINLOCK(tlob_ws_lock);

[Severity: High]
Could this spinlock cause sleeping in atomic context warnings on PREEMPT_RT
kernels?

On PREEMPT_RT, spinlock_t maps to a sleepable rt_mutex. However, this lock
is acquired in atomic contexts. For example, in the tracepoint callback path:

handle_sched_process_exit()
  tlob_destroy_task()
    tlob_detach_from_binding()

And in the uprobe exception path:

tlob_uprobe_entry_handler()
  tlob_start_task()

Should this use a raw_spinlock_t instead to avoid violating PREEMPT_RT
rules?

[ ... ]

> +static int tlob_stop_task(struct task_struct *task, void *binding)
> +{
> +	struct ha_monitor *ha_mon;
> +	struct tlob_task_state *ws;
> +	bool budget_exceeded;
> +
> +	scoped_guard(rcu) {
> +		ha_mon = ha_get_monitor(task->pid, NULL);
> +		if (!ha_mon)
> +			return -ESRCH;
> +
> +		ws = ha_get_target(ha_mon);
> +		if (WARN_ON_ONCE(!ws))
> +			return -ESRCH;
> +
> +		/* Only the binding that opened the window may end it; NULL
> +		 * (task exit) skips the check.  Symmetric with the restart
> +		 * check in tlob_start_task(). */
> +		if (binding && ws->binding != binding)
> +			return -EALREADY;
> +
> +		/* cmpxchg (0->1) claims the window under RCU; _release pairs
> +		 * with the acquire in ha_setup_invariants(). */
> +		if (atomic_cmpxchg_release(&ws->stopping, 0, 1) != 0)
> +			return -EAGAIN;

[Severity: High]
Is there a race condition here if the latency budget expires at the exact
same time the task hits the stop uprobe?

If tlob_stop_task() wins the cmpxchg, it claims the window. The hrtimer
callback could then run concurrently and call tlob_reset_notify(), which
checks the stopping flag and returns early. This would skip emitting the
detail tracepoint and skip setting budget_exceeded to true. 

Since tlob_stop_task() then reads budget_exceeded as false and returns 0,
could this silently hide genuine budget violations?

[ ... ]

> +static void tlob_destroy_task(struct task_struct *task)
> +{

[ ... ]

> +	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);
> +
> +	scoped_guard(rcu) {
> +		da_monitor_reset(&ha_mon->da_mon);
> +	}
> +	da_destroy_storage(task->pid);

[Severity: Critical]
This is a pre-existing issue, but is it safe to call da_destroy_storage()
here without holding a lock? 

Looking at da_destroy_storage() in include/rv/da_monitor.h, it calls
hash_del_rcu(&mon_storage->node). If multiple monitored tasks whose PIDs
map to the same hash bucket exit concurrently, or one exits while another
starts, calling hash_del_rcu() and hash_add_rcu() concurrently without
mutual exclusion on the same bucket can corrupt the hlist pointers.

[ ... ]

> +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);
> +		}
> +	}
> +
> +	list_for_each_entry_safe(ws, tmp, &to_destroy, started_node) {

[Severity: Critical]
Can this iteration trigger a use-after-free if a parked task exits
concurrently?

When tlob_unbind_reap() moves parked windows to the local to_destroy list
and drops the spinlock, it iterates the list without holding rcu_read_lock().

If handle_sched_process_exit() invokes tlob_destroy_task() concurrently,
it skips detaching from the binding list (since ws->binding is already NULL)
and calls call_rcu() to free the task state. Without RCU read protection
here, the grace period could expire and free the memory while it is still
linked on the local to_destroy list.

> +		list_del_init(&ws->started_node);
> +		tlob_destroy_task(ws->task);
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6