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

[sashiko-bot@kernel.org]

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

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.

> diff --git a/Documentation/tools/rtla/common_options.txt b/Documentation/tools/rtla/common_options.txt
> index 38da1cf443a48..6bf0f86393333 100644
> --- a/Documentation/tools/rtla/common_options.txt
> +++ b/Documentation/tools/rtla/common_options.txt
> @@ -26,12 +26,19 @@
>  
>  **--filter** *<filter>*
>  
> -        Filter the previous **-e** *sys:event* event with *<filter>*. For further information about event filtering see https://www.kernel.org/doc/html/latest/trace/events.html#event-filtering.
> +        Filter the previous **-e** *sys:event* event with *<filter>*.
> +
> +        If the same event is specified more than once but with different filters, the last specified filter will be the only one applied.
> +
> +	For further information about event filtering see https://www.kernel.org/doc/html/latest/trace/events.html#event-filtering.
>  
>          This option cannot be unset.
>  
>  **--trigger** *<trigger>*
>          Enable a trace event trigger to the previous **-e** *sys:event*.
> +
> +        If the same event is specified more than once but with different triggers, the last specified trigger will be the only one applied.
> +

[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?

>          If the *hist:* trigger is activated, the output histogram will be automatically saved to a file named *system_event_hist.txt*.
>          For example, the command:
>  
> diff --git a/tools/tracing/rtla/src/trace.c b/tools/tracing/rtla/src/trace.c
> index 17d148f7d5d6e..06204d339f967 100644
> --- a/tools/tracing/rtla/src/trace.c
> +++ b/tools/tracing/rtla/src/trace.c
> @@ -487,6 +487,10 @@ static int trace_event_enable_filter(struct trace_instance *instance,
>  	char filter[MAX_PATH];
>  	int retval;
>  
> +	/* Unconditionally clean any pre-existing filters */
> +	tracefs_event_file_write(instance->inst, tevent->system,
> +				 tevent->event, "filter", "0");
> +

[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