Re: [PATCH 07/10] rv: Add throttle deadline monitor

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] rv: timer overflow in runtime_left_ns() allows monitor evasion
- [High] rv: speculative monitor allocation causes memory leak and Use-After-Free
- [High] rv: concurrent hash table modifications in tracepoints corrupt monitor lists
- [Medium] rv: handle_sched_switch() skips switch-out events for background servers
- [Medium] rv: monitor timer is cancelled before syscall validation allowing evasion
--

commit 59f095bf10c27a9ddbf78eaa030f99a4df14f9eb
Author: Gabriele Monaco [off-list ref]

rv: Add throttle deadline monitor
    
This commit adds a monitor to validate the behavior of the deadline
scheduler, specifically ensuring that deadline entities are throttled when
they use up their runtime.

[ ... ]

+static inline u64 runtime_left_ns(struct ha_monitor *ha_mon)
+{
+	return ha_get_target(ha_mon)->runtime + RUNTIME_THRESH;
+}

[Severity: High]
Can the implicit conversion to u64 here cause a timer overflow? 

ha_get_target(ha_mon)->runtime is a signed 64-bit value that can be
negative when a task overruns its budget. Returning it as a u64 casts it
to a huge positive value, which is then passed to ha_start_timer_ns(). 

Will this push the enforcement timer far into the future and effectively
blind the monitor to task overruns?

[ ... ]

+static void handle_sched_switch(void *data, bool preempt,
+				struct task_struct *prev,
+				struct task_struct *next,
+				unsigned int prev_state)
+{

[ ... ]

+	/*
+	 * The server is available in next only if the next task is boosted,
+	 * otherwise we need to retrieve it.
+	 * Here the server continues in the state running/armed until actually
+	 * stopped, this works since we continue expecting a throttle.
+	 */
+	if (next->dl_server) {
+		da_handle_start_event(EXPAND_ID(next->dl_server, cpu,
+						get_server_type(next)),
+				      sched_switch_in_throttle);
+	} else {
+		handle_server_switch(next, cpu, DL_SERVER_FAIR);
+		if (IS_ENABLED(CONFIG_SCHED_CLASS_EXT))
+			handle_server_switch(next, cpu, DL_SERVER_EXT);
+	}
+}

[Severity: Medium]
Does this skip switch-out events for background servers?

If next->dl_server is non-NULL, we enter this branch and send a switch-in
event. However, this skips the else block entirely, meaning the preempted
fair or ext server on that CPU never receives its sched_switch_out_throttle
event.

Won't this leave the fair server incorrectly armed or running, potentially
causing false positive timer failures?

[ ... ]

+static void handle_sys_enter(void *data, struct pt_regs *regs, long id)
+{
+	struct task_struct *p;
+	int new_policy = -1;
+	pid_t pid = 0;
+
+	new_policy = extract_params(regs, id, &pid);
+	if (new_policy < 0)
+		return;
+	guard(rcu)();
+	p = pid ? find_task_by_vpid(pid) : current;
+	if (unlikely(!p) || new_policy == p->policy)
+		return;
+
+	if (p->policy == SCHED_DEADLINE)
+		da_reset(EXPAND_ID_TASK(p));

[Severity: Medium]
Can a user evade the monitor by spamming invalid syscalls?

Calling da_reset() permanently cancels the enforcement timer via
ha_cancel_timer() before the syscall is validated. If the syscall fails
and returns an error code, the task remains in SCHED_DEADLINE but the
monitor's timer stays disabled for the remainder of the execution slice.

+	else if (new_policy == SCHED_DEADLINE)
+		da_create_or_get(EXPAND_ID_TASK(p));
+}

[Severity: High]
Does this speculative allocation create a memory leak and use-after-free?

da_create_or_get() is called here before the sched_setscheduler syscall
completes or validates permissions. If the syscall fails, the task's
policy remains unchanged.

When the task eventually exits, handle_exit() only destroys storage if
the task's current policy is SCHED_DEADLINE, leaving the speculatively
allocated monitor storage permanently leaked.

Furthermore, if the PID is later reused and the new task enters
SCHED_DEADLINE, da_create_or_get() will find the leaked storage without
updating the target pointer, potentially causing a use-after-free when
the monitor accesses target->runtime.

[Severity: High]
Is it safe to concurrently modify the RCU hash table here?

da_create_or_get() adds to the hash table via hash_add_rcu(), but it is
called directly from a tracepoint protected only by a read-side lock
(guard(rcu)()). 

Since tracepoints can trigger concurrently on different CPUs, won't this
race on the hlist pointers and corrupt the monitor lists? The same concern
applies to the hash_del_rcu() call in handle_exit() registered to the
sched_process_exit tracepoint below.

[ ... ]

+static int enable_throttle(void)
+{

[ ... ]

+	rv_attach_trace_probe("throttle", sched_switch, handle_sched_switch);
+	if (!should_skip_syscall_handle())
+		rv_attach_trace_probe("throttle", sys_enter, handle_sys_enter);
+	rv_attach_trace_probe("throttle", task_newtask, handle_newtask);
+	rv_attach_trace_probe("throttle", sched_dl_server_stop_tp, handle_dl_server_stop);
+	rv_attach_trace_probe("throttle", sched_process_exit, handle_exit);
+
+	return 0;
+}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929124908.177676-1-gmonaco@redhat.com?part=7