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