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 [off-list ref]
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.
quoted hunk ↗ jump to hunk
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?
[ ... ]
quoted hunk ↗ jump to hunk
+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)
+{
[ ... ]
quoted hunk ↗ jump to hunk
+ 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.
[ ... ]
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+ list_del_init(&ws->started_node);
+ tlob_destroy_task(ws->task);
+ }
+}
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1787243842.git.wen.yang@linux.dev?part=6