Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [Critical] Early exit in `timerlat_main` leaves a dangling task_struct pointer, causing a Use-After-Free when the tracer is stopped. - [High] `osn->cpu` is only initialized for online CPUs, permanently breaking hotplug functionality and triggering the aforementioned Use-After-Free. Pre-existing issues: - [High] Uninitialized mutex `osn->lock` used on CPUs hotplugged after boot. - [High] `osnoise_main` ignores its `data` argument and lacks the CPU migration validation, leading to per-CPU data corruption if migrated. - [High] ABBA deadlock between `stop_kthread()` and `osnoise_migration_pending()`. -- --- Patch [2]: [PATCH 2/4] tracing/osnoise: timerlat_main: Disable migration before per-cpu access --- commit 1b15cf4d4c5a1c36335ab647f7c93d428b13f4fc Author: Crystal Wood <[email protected]> tracing/osnoise: timerlat_main: Disable migration before per-cpu access This commit attempts to fix a preemptible-context splat in timerlat_main by disabling migration before accessing per-CPU variables. It verifies the CPU assignment after calling migrate_disable and exits early if a migration occurred. It also initializes the CPU ID for each online CPU in the tracer setup. > diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c > index e2e1ef3f5a617..a53a07e59197c 100644 > --- a/kernel/trace/trace_osnoise.c > +++ b/kernel/trace/trace_osnoise.c [ ... ] > @@ -1933,6 +1934,13 @@ static int timerlat_main(void *data) > * flag. > */ > migrate_disable(); > + if (osn_var->cpu != smp_processor_id()) { > + migrate_enable(); > + return 1; > + } [Severity: Critical] Does this early return leave a dangling task_struct pointer? If timerlat_main exits here, the thread terminates without clearing osn_var->kthread. Because the creator in start_kthread does not hold an extra reference, the task_struct is freed upon exit. When the tracer is later disabled or the CPU goes offline, stop_kthread will unconditionally call kthread_stop(osn_var->kthread) on this freed pointer. Can this cause a use-after-free crash? [Severity: High] This is a pre-existing issue, but does a deadlock exist between stopping the kthread and handling pending migrations? If a user disables the tracer, stop_kthread successfully takes osn_var->lock and calls kthread_stop. Concurrently, if userspace alters the kthread's CPU affinity, the scheduler sets current->migration_pending = 1 because the kthread is pinned by migrate_disable. The kthread loop calls osnoise_migration_pending, detects this flag, and attempts to acquire mutex_lock(&osn->lock). Will kthread_stop block indefinitely waiting for the kthread to exit, while the kthread blocks indefinitely on the mutex held by stop_kthread? > + > + tlat = this_cpu_tmr_var(); > + [ ... ] > @@ -2061,7 +2069,7 @@ static int start_kthread(unsigned int cpu) > snprintf(comm, 24, "osnoise/%d", cpu); > } > > - kthread = kthread_run_on_cpu(main, NULL, cpu, comm); > + kthread = kthread_run_on_cpu(main, cpu_osn_var(cpu), cpu, comm); [Severity: High] This is a pre-existing issue, but does osnoise_main have the same migration vulnerability that was just addressed in timerlat_main? start_kthread now passes cpu_osn_var(cpu) to both timerlat_main and osnoise_main via the data parameter. However, osnoise_main ignores its data argument and blindly pins itself without validating the CPU: kernel/trace/trace_osnoise.c:osnoise_main() { unsigned long flags; ... migrate_disable(); ... } If osnoise_main is migrated by hotplug activity before migrate_disable, does it pin itself to the wrong CPU, causing two tracing threads to concurrently overwrite the same tracking data without synchronization? [ ... ] > @@ -3207,8 +3215,12 @@ __init static int init_osnoise_tracer(void) > > mutex_init(&interface_lock); > > - for_each_online_cpu(cpu) > - mutex_init(&cpu_osn_var(cpu)->lock); > + for_each_online_cpu(cpu) { > + struct osnoise_variables *osn = cpu_osn_var(cpu); > + > + mutex_init(&osn->lock); > + osn->cpu = cpu; > + } [Severity: High] Is osn->cpu correctly initialized for CPUs that are hotplugged after boot? Because this loop uses for_each_online_cpu, CPUs that are offline during tracer initialization will have osn->cpu left as 0. When timerlat_main later starts on a newly hotplugged CPU, the migration check (osn_var->cpu != smp_processor_id()) will incorrectly evaluate to true. Will this cause the kthread to immediately abort upon creation, breaking hotplug functionality and triggering the dangling kthread regression? [Severity: High] This is a pre-existing issue, but is the mutex initialization skipping offline CPUs? Because mutex_init(&osn->lock) is strictly within the for_each_online_cpu loop, CPUs brought online after boot will have a zero-initialized lock. When start_kthread runs on a hotplugged CPU, it attempts to lock this uninitialized mutex. Could this trigger a lockdep splat or a null pointer dereference? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
