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 <[email protected]> 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/[email protected]?part=7
