Re: [PATCH v5 7/8] rtla: Unconditionally clean any pre-existing filters for user-provided events

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Logic error in command-line trigger processing causing triggers to accumulate instead of overriding.
- [Medium] Unconditionally clearing filters during event processing destroys previously configured specific event filters when a subsystem is enabled.
- [Medium] Out-of-bounds read in osnoise_ipi_cpumask_handler due to using glibc CPU_AND on a dynamically sized trace event field.
- [Medium] Missing bounds check on `dst_cpu` parsed from trace record leads to an out-of-bounds array write.
--

commit 5cb00dbb92aa356c89071b1755fbef9a7743360f
Author: Valentin Schneider [off-list ref]

rtla: Unconditionally clean any pre-existing filters for user-provided events

This commit unconditionally cleans up pre-existing filters when enabling an
event provided via the '-e' command line argument to prevent confusion. It
also documents that when the same event is specified multiple times, only
the last specified filter or trigger applies.

[Severity: Medium]
Does the command-line trigger processing actually implement this override
behavior?

Looking at trace_event_enable_trigger() in tools/tracing/rtla/src/trace.c,
the new trigger is written without clearing prior triggers for the same
event:

	retval = tracefs_event_file_write(instance->inst, tevent->system,
					  tevent->event, "trigger", trigger);

Since tracefs 'trigger' files are cumulative by default, could this cause
triggers to accumulate instead of replacing the previous one?

[Severity: Medium]
Will this destroy previously configured specific event filters when a broader
subsystem is enabled?

If tevent->event is NULL (e.g., when enabling an entire subsystem like
'-e sched'), unconditionally writing '0' to the subsystem's filter file
might wipe out all specific event filters previously configured within
that subsystem.

 	if (!tevent->filter)
 		return 0;
 

While reviewing this series, I also noticed a couple of issues introduced
in osnoise_top.c:

[Severity: Medium]
Does osnoise_ipi_cpumask_handler() have an out-of-bounds read when parsing
the cpumask field?

When processing a broadcast IPI trace event, the dynamically sized 'cpumask'
field is extracted into event_cpus:

tools/tracing/rtla/src/osnoise_top.c:osnoise_ipi_cpumask_handler() {
    ...
    event_cpus = tep_get_field_raw(s, event, "cpumask", record, &len, 1);
    ...
    CPU_AND(&cpumask_tmp_cpus, event_cpus, &params->common.monitored_cpus);
    ...
}

Since glibc's CPU_AND() unconditionally reads 128 bytes (sizeof(cpu_set_t))
from event_cpus, and the raw buffer size is only 'len' bytes, could this
read past the end of the buffer?


[Severity: Medium]
Is there a missing bounds check on dst_cpu in osnoise_ipi_cpu_handler()
that could lead to an out-of-bounds write?

The 'cpu' field is parsed from the trace record directly into dst_cpu and
used as an array index:

tools/tracing/rtla/src/osnoise_top.c:osnoise_ipi_cpu_handler() {
    ...
    tep_get_field_val(s, event, "cpu", record, &dst_cpu, 1);
    account_ipi(tool, dst_cpu);
    ...
}

tools/tracing/rtla/src/osnoise_top.c:account_ipi() {
    ...
    cpu_data = &data->cpu_data[dst_cpu];
    update_sum(&cpu_data->ipi_count, &inc);
    ...
}

If the trace record contains an unexpectedly high CPU ID (e.g., due to hotplug
or trace buffer corruption), could this write beyond the bounds of the
data->cpu_data array since there's no verification that dst_cpu < nr_cpus?

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