Re: [PATCH 2/4] tracing/osnoise: timerlat_main: Disable migration before per-cpu access

[sashiko-bot@kernel.org]

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

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/20260824211544.3984835-1-crwood@redhat.com?part=2