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

[sashiko-bot@kernel.org]

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 <gmonaco@redhat.com>

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.

> --- /dev/null
> +++ b/kernel/trace/rv/monitors/throttle/throttle.c
[ ... ]
> +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