[PATCH 0/4] tracing/osnoise: Synchronization fixes

Various improvements to synchronization in osnoise/timerlat.

Crystal Wood (4):
  tracing/osnoise: Per-cpu mutex and fd detachment
  tracing/osnoise: timerlat_main: Disable migration before per-cpu
    access
  tracing/osnoise: start_kthread: Always check OSN_WORKLOAD
  tracing/osnoise: Take trace_types_lock in timerlat_fd_open

 kernel/trace/trace_osnoise.c | 274 +++++++++++++++++++++--------------
 1 file changed, 166 insertions(+), 108 deletions(-)

-- 
2.54.0

Clean up a variety of synchronization issues and related bandaids
by having a per-cpu mutex that guards changes to kthread, and
fd open/close/revoke.

Replace the SIGKILL hack for userspace timerlat threads (that doesn't
even work, because we don't wait for the process to actually die) with
a mutex-protected detachment mechanism.  The mutex should be uncontended
during normal timerlat_fd_read() usage.

Signed-off-by: Crystal Wood <redacted>
---
 kernel/trace/trace_osnoise.c | 239 +++++++++++++++++++++--------------
 1 file changed, 144 insertions(+), 95 deletions(-)

diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
index 0e1265acd1cc..e2e1ef3f5a61 100644
--- a/kernel/trace/trace_osnoise.c
+++ b/kernel/trace/trace_osnoise.c

@@ -222,19 +222,30 @@ struct osn_thread {
 	u64	delta_start;
 };
 
+struct fd_data {
+	struct task_struct *thread;
+	void (*detach)(struct fd_data *fdd);
+	int cpu;
+};
+
 /*
  * Runtime information: this structure saves the runtime information used by
  * one sampling thread.
  */
 struct osnoise_variables {
+	struct mutex		lock; /* covers kthread and fdd changes */
 	struct task_struct	*kthread;
-	bool			sampling;
-	pid_t			pid;
-	struct osn_nmi		nmi;
-	struct osn_irq		irq;
-	struct osn_softirq	softirq;
-	struct osn_thread	thread;
-	local_t			int_counter;
+	struct fd_data		*fdd;
+
+	struct_group(zero,
+		bool			sampling;
+		pid_t			pid;
+		struct osn_nmi		nmi;
+		struct osn_irq		irq;
+		struct osn_softirq	softirq;
+		struct osn_thread	thread;
+		local_t			int_counter;
+	);
 };
 
 /*

@@ -250,6 +261,11 @@ static inline struct osnoise_variables *this_cpu_osn_var(void)
 	return this_cpu_ptr(&per_cpu_osnoise_var);
 }
 
+static inline struct osnoise_variables *cpu_osn_var(int cpu)
+{
+	return per_cpu_ptr(&per_cpu_osnoise_var, cpu);
+}
+
 /*
  * Protect the interface.
  */

@@ -292,9 +308,6 @@ static inline void tlat_var_reset(void)
 	struct timerlat_variables *tlat_var;
 	int cpu;
 
-	/* Synchronize with the timerlat interfaces */
-	mutex_lock(&interface_lock);
-
 	/*
 	 * So far, all the values are initialized as 0, so
 	 * zeroing the structure is perfect.

@@ -310,8 +323,6 @@ static inline void tlat_var_reset(void)
 	 * thread that wakes up, if TIMERLAT_ALIGN is set.
 	 */
 	atomic64_set(&align_next, 0);
-
-	mutex_unlock(&interface_lock);
 }
 #else /* CONFIG_TIMERLAT_TRACER */
 #define tlat_var_reset()	do {} while (0)

@@ -330,8 +341,11 @@ static inline void osn_var_reset(void)
 	 * zeroing the structure is perfect.
 	 */
 	for_each_online_cpu(cpu) {
-		osn_var = per_cpu_ptr(&per_cpu_osnoise_var, cpu);
-		memset(osn_var, 0, sizeof(*osn_var));
+		osn_var = cpu_osn_var(cpu);
+
+		WARN_ON(osn_var->kthread);
+		WARN_ON(osn_var->fdd);
+		memset(&osn_var->zero, 0, sizeof(osn_var->zero));
 	}
 }
 

@@ -1673,6 +1687,8 @@ static void osnoise_sleep(bool skip_period)
  */
 static inline int osnoise_migration_pending(void)
 {
+	struct osnoise_variables *osn = this_cpu_osn_var();
+
 	if (!current->migration_pending)
 		return 0;
 

@@ -1688,10 +1704,10 @@ static inline int osnoise_migration_pending(void)
 	 * The tracers are responsible for cleaning their env before
 	 * exiting.
 	 */
-	mutex_lock(&interface_lock);
+	mutex_lock(&osn->lock);
 	this_cpu_osn_var()->kthread = NULL;
 	cpumask_clear_cpu(smp_processor_id(), &kthread_cpumask);
-	mutex_unlock(&interface_lock);
+	mutex_unlock(&osn->lock);
 
 	return 1;
 }

@@ -1978,32 +1994,22 @@ static int timerlat_main(void *data)
  */
 static void stop_kthread(unsigned int cpu)
 {
-	struct task_struct *kthread;
+	struct osnoise_variables *osn_var = cpu_osn_var(cpu);
 
-	kthread = xchg_relaxed(&(per_cpu(per_cpu_osnoise_var, cpu).kthread), NULL);
-	if (kthread) {
-		if (cpumask_test_and_clear_cpu(cpu, &kthread_cpumask) &&
-		    !WARN_ON(!test_bit(OSN_WORKLOAD, &osnoise_options))) {
-			kthread_stop(kthread);
-		} else if (!WARN_ON(test_bit(OSN_WORKLOAD, &osnoise_options))) {
-			/*
-			 * This is a user thread waiting on the timerlat_fd. We need
-			 * to close all users, and the best way to guarantee this is
-			 * by killing the thread. NOTE: this is a purpose specific file.
-			 */
-			kill_pid(kthread->thread_pid, SIGKILL, 1);
-			put_task_struct(kthread);
-		}
-	} else {
-		/* if no workload, just return */
-		if (!test_bit(OSN_WORKLOAD, &osnoise_options)) {
-			/*
-			 * This is set in the osnoise tracer case.
-			 */
-			per_cpu(per_cpu_osnoise_var, cpu).sampling = false;
-			barrier();
-		}
+	mutex_lock(&osn_var->lock);
+
+	if (osn_var->fdd) {
+		WARN_ON(test_bit(OSN_WORKLOAD, &osnoise_options));
+		WARN_ON(osn_var->kthread);
+		osn_var->fdd->detach(osn_var->fdd);
+	} else if (osn_var->kthread) {
+		WARN_ON(!cpumask_test_and_clear_cpu(cpu, &kthread_cpumask));
+		WARN_ON(!test_bit(OSN_WORKLOAD, &osnoise_options));
+		kthread_stop(osn_var->kthread);
+		osn_var->kthread = NULL;
 	}
+
+	mutex_unlock(&osn_var->lock);
 }
 
 /*

@@ -2029,13 +2035,18 @@ static void stop_per_cpu_kthreads(void)
  */
 static int start_kthread(unsigned int cpu)
 {
+	struct osnoise_variables *osn = cpu_osn_var(cpu);
 	struct task_struct *kthread;
 	void *main = osnoise_main;
 	char comm[24];
+	int ret = 0;
+
+	lockdep_assert_held(&trace_types_lock);
+	mutex_lock(&osn->lock);
 
 	/* Do not start a new thread if it is already running */
-	if (per_cpu(per_cpu_osnoise_var, cpu).kthread)
-		return 0;
+	if (osn->kthread)
+		goto out;
 
 	if (timerlat_enabled()) {
 		snprintf(comm, 24, "timerlat/%d", cpu);

@@ -2045,7 +2056,7 @@ static int start_kthread(unsigned int cpu)
 		if (!test_bit(OSN_WORKLOAD, &osnoise_options)) {
 			per_cpu(per_cpu_osnoise_var, cpu).sampling = true;
 			barrier();
-			return 0;
+			goto out;
 		}
 		snprintf(comm, 24, "osnoise/%d", cpu);
 	}

@@ -2054,13 +2065,16 @@ static int start_kthread(unsigned int cpu)
 
 	if (IS_ERR(kthread)) {
 		pr_err(BANNER "could not start sampling thread\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto out;
 	}
 
-	per_cpu(per_cpu_osnoise_var, cpu).kthread = kthread;
+	osn->kthread = kthread;
 	cpumask_set_cpu(cpu, &kthread_cpumask);
 
-	return 0;
+out:
+	mutex_unlock(&osn->lock);
+	return ret;
 }
 
 /*

@@ -2086,15 +2100,9 @@ static int start_per_cpu_kthreads(void)
 	 */
 	cpumask_and(current_mask, cpu_online_mask, &osnoise_cpumask);
 
-	for_each_possible_cpu(cpu) {
-		if (cpumask_test_and_clear_cpu(cpu, &kthread_cpumask)) {
-			struct task_struct *kthread;
-
-			kthread = xchg_relaxed(&(per_cpu(per_cpu_osnoise_var, cpu).kthread), NULL);
-			if (!WARN_ON(!kthread))
-				kthread_stop(kthread);
-		}
-	}
+	for_each_possible_cpu(cpu)
+		if (cpumask_test_and_clear_cpu(cpu, &kthread_cpumask))
+			stop_kthread(cpu);
 
 	for_each_cpu(cpu, current_mask) {
 		retval = start_kthread(cpu);

@@ -2417,11 +2425,37 @@ osnoise_cpus_write(struct file *filp, const char __user *ubuf, size_t count,
 }
 
 #ifdef CONFIG_TIMERLAT_TRACER
+static void timerlat_fd_detach(struct fd_data *fdd)
+{
+	struct osnoise_variables *osn_var;
+	struct timerlat_variables *tlat_var;
+
+	osn_var = cpu_osn_var(fdd->cpu);
+	tlat_var = per_cpu_ptr(&per_cpu_timerlat_var, fdd->cpu);
+
+	lockdep_assert_held(&osn_var->lock);
+	if (WARN_ON(fdd != osn_var->fdd))
+		return;
+
+	WARN_ON(osn_var->kthread);
+
+	hrtimer_cancel(&tlat_var->timer);
+	memset(tlat_var, 0, sizeof(*tlat_var));
+
+	osn_var->sampling = 0;
+	osn_var->pid = 0;
+
+	put_task_struct(fdd->thread);
+	osn_var->fdd = NULL;
+}
+
 static int timerlat_fd_open(struct inode *inode, struct file *file)
 {
+	struct fd_data *fdd;
 	struct osnoise_variables *osn_var;
 	struct timerlat_variables *tlat;
 	long cpu = (long) inode->i_cdev;
+	int ret = 0;
 
 	mutex_lock(&interface_lock);
 

@@ -2437,16 +2471,15 @@ static int timerlat_fd_open(struct inode *inode, struct file *file)
 	migrate_disable();
 
 	osn_var = this_cpu_osn_var();
+	mutex_lock(&osn_var->lock);
 
-	/*
-	 * The osn_var->pid holds the single access to this file.
-	 */
-	if (osn_var->pid) {
-		mutex_unlock(&interface_lock);
-		migrate_enable();
-		return -EBUSY;
+	if (osn_var->fdd) {
+		ret = -EBUSY;
+		goto err;
 	}
 
+	WARN_ON_ONCE(osn_var->kthread);
+
 	/*
 	 * timerlat tracer is a per-cpu tracer. Check if the user-space too
 	 * is pinned to a single CPU. The tracer laters monitor if the task

@@ -2455,24 +2488,33 @@ static int timerlat_fd_open(struct inode *inode, struct file *file)
 	 * setup.
 	 */
 	if (current->nr_cpus_allowed > 1 ||  cpu != smp_processor_id()) {
-		mutex_unlock(&interface_lock);
-		migrate_enable();
-		return -EPERM;
+		ret = -EPERM;
+		goto err;
+	}
+
+	fdd = kmalloc(sizeof(*fdd), GFP_KERNEL);
+	if (!fdd) {
+		ret = -ENOMEM;
+		goto err;
 	}
 
 	/*
 	 * From now on, it is good to go.
 	 */
-	file->private_data = inode->i_cdev;
+	fdd->thread = current;
+	fdd->detach = timerlat_fd_detach;
+	fdd->cpu = cpu;
+	file->private_data = fdd;
 
 	get_task_struct(current);
 
-	osn_var->kthread = current;
 	osn_var->pid = current->pid;
+	osn_var->fdd = fdd;
 
 	/*
 	 * Setup is done.
 	 */
+	mutex_unlock(&osn_var->lock);
 	mutex_unlock(&interface_lock);
 
 	tlat = this_cpu_tmr_var();

@@ -2482,6 +2524,12 @@ static int timerlat_fd_open(struct inode *inode, struct file *file)
 
 	migrate_enable();
 	return 0;
+
+err:
+	mutex_unlock(&osn_var->lock);
+	mutex_unlock(&interface_lock);
+	migrate_enable();
+	return ret;
 };
 
 /*

@@ -2497,12 +2545,13 @@ static ssize_t
 timerlat_fd_read(struct file *file, char __user *ubuf, size_t count,
 		  loff_t *ppos)
 {
-	long cpu = (long) file->private_data;
+	struct fd_data *fdd = file->private_data;
 	struct osnoise_variables *osn_var;
 	struct timerlat_variables *tlat;
 	struct timerlat_sample s;
 	s64 diff;
 	u64 now;
+	int ret = 0;
 
 	migrate_disable();
 

@@ -2513,13 +2562,13 @@ timerlat_fd_read(struct file *file, char __user *ubuf, size_t count,
 	 * we can do about it.
 	 * So, if the thread is running on another CPU, stop the machinery.
 	 */
-	if (cpu == smp_processor_id()) {
+	if (fdd->cpu == smp_processor_id()) {
 		if (tlat->uthread_migrate) {
 			migrate_enable();
 			return -EINVAL;
 		}
 	} else {
-		per_cpu_ptr(&per_cpu_timerlat_var, cpu)->uthread_migrate = 1;
+		per_cpu_ptr(&per_cpu_timerlat_var, fdd->cpu)->uthread_migrate = 1;
 		osnoise_taint("timerlat user thread migrate\n");
 		osnoise_stop_tracing();
 		migrate_enable();

@@ -2528,6 +2577,13 @@ timerlat_fd_read(struct file *file, char __user *ubuf, size_t count,
 
 	osn_var = this_cpu_osn_var();
 
+	/* In normal usage, this should always be uncontended. */
+	mutex_lock(&osn_var->lock);
+	if (fdd != osn_var->fdd || current != fdd->thread) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	/*
 	 * The timerlat in user-space runs in a different order:
 	 * the read() starts from the execution of the previous occurrence,

@@ -2574,6 +2630,11 @@ timerlat_fd_read(struct file *file, char __user *ubuf, size_t count,
 	/* wait for the next period */
 	wait_next_period(tlat);
 
+	if (fdd != osn_var->fdd) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	/* This is the wakeup from this cycle */
 	now = ktime_to_ns(hrtimer_cb_get_time(&tlat->timer));
 	diff = now - tlat->abs_period;

@@ -2599,39 +2660,24 @@ timerlat_fd_read(struct file *file, char __user *ubuf, size_t count,
 	}
 
 out:
+	mutex_unlock(&osn_var->lock);
 	migrate_enable();
-	return 0;
+	return ret;
 }
 
 static int timerlat_fd_release(struct inode *inode, struct file *file)
 {
+	struct fd_data *fdd = file->private_data;
 	struct osnoise_variables *osn_var;
-	struct timerlat_variables *tlat_var;
-	long cpu = (long) file->private_data;
-
-	migrate_disable();
-	mutex_lock(&interface_lock);
-
-	osn_var = per_cpu_ptr(&per_cpu_osnoise_var, cpu);
-	tlat_var = per_cpu_ptr(&per_cpu_timerlat_var, cpu);
 
-	if (tlat_var->kthread)
-		hrtimer_cancel(&tlat_var->timer);
-	memset(tlat_var, 0, sizeof(*tlat_var));
+	osn_var = per_cpu_ptr(&per_cpu_osnoise_var, fdd->cpu);
 
-	osn_var->sampling = 0;
-	osn_var->pid = 0;
+	mutex_lock(&osn_var->lock);
+	if (fdd == osn_var->fdd)
+		timerlat_fd_detach(fdd);
+	mutex_unlock(&osn_var->lock);
 
-	/*
-	 * We are leaving, not being stopped... see stop_kthread();
-	 */
-	if (osn_var->kthread) {
-		put_task_struct(osn_var->kthread);
-		osn_var->kthread = NULL;
-	}
-
-	mutex_unlock(&interface_lock);
-	migrate_enable();
+	kfree(fdd);
 	return 0;
 }
 #endif

@@ -3157,10 +3203,13 @@ __init static int init_timerlat_tracer(void)
 
 __init static int init_osnoise_tracer(void)
 {
-	int ret;
+	int ret, cpu;
 
 	mutex_init(&interface_lock);
 
+	for_each_online_cpu(cpu)
+		mutex_init(&cpu_osn_var(cpu)->lock);
+
 	cpumask_copy(&osnoise_cpumask, cpu_all_mask);
 
 	ret = register_tracer(&osnoise_tracer);

-- 
2.54.0

Avoid a preemptible-context splat if we get migrated (e.g. from hotplug
activity) before reaching this_cpu_osn_var(), and verify that we're
on the correct CPU after migrate_disable().

Signed-off-by: Crystal Wood <redacted>
---
 kernel/trace/trace_osnoise.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
index e2e1ef3f5a61..a53a07e59197 100644
--- a/kernel/trace/trace_osnoise.c
+++ b/kernel/trace/trace_osnoise.c

@@ -236,6 +236,7 @@ struct osnoise_variables {
 	struct mutex		lock; /* covers kthread and fdd changes */
 	struct task_struct	*kthread;
 	struct fd_data		*fdd;
+	int			cpu;
 
 	struct_group(zero,
 		bool			sampling;

@@ -1912,8 +1913,8 @@ static int wait_next_period(struct timerlat_variables *tlat)
  */
 static int timerlat_main(void *data)
 {
-	struct osnoise_variables *osn_var = this_cpu_osn_var();
-	struct timerlat_variables *tlat = this_cpu_tmr_var();
+	struct osnoise_variables *osn_var = data;
+	struct timerlat_variables *tlat;
 	struct timerlat_sample s;
 	struct sched_param sp;
 	unsigned long flags;

@@ -1933,6 +1934,13 @@ static int timerlat_main(void *data)
 	 * flag.
 	 */
 	migrate_disable();
+	if (osn_var->cpu != smp_processor_id()) {
+		migrate_enable();
+		return 1;
+	}
+
+	tlat = this_cpu_tmr_var();
+
 	raw_spin_lock_irqsave(&current->pi_lock, flags);
 	current->flags &= ~(PF_NO_SETAFFINITY);
 	raw_spin_unlock_irqrestore(&current->pi_lock, flags);

@@ -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);
 
 	if (IS_ERR(kthread)) {
 		pr_err(BANNER "could not start sampling thread\n");

@@ -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;
+	}
 
 	cpumask_copy(&osnoise_cpumask, cpu_all_mask);
 

-- 
2.54.0

This prevents a user fd from being opened when osnoise_options_write()
has already stopped threads but not yet set OSN_WORKLOAD, and matches what
all callers of start_kthread() do.

The original intent was probably for interface_lock to cover things like
this, but it never covered the full race window, and currently can't
due to lock order issues stemming from using interface_lock in
osnoise_sleep().  There does not appear to be any remaining reason
to take interface_lock in timerlat_fd_open(), so remove it.

Signed-off-by: Crystal Wood <redacted>
---
Do we really need that interface_lock in osnoise_sleep()?  Technically
we should probably be grabbing it every time we access osnoise_data, but
we don't seem to do so anywhere else.  Perhaps this was due to concerns
about the atomicity of "period - runtime", but that's just synchronization
theater given that userspace has no way of writing to both atomically.

In any case, we shouldn't use the same lock for protecting osnoise_data
members that we use for major reconfiguration procedures.
---
 kernel/trace/trace_osnoise.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
index 0f27af6ade3e..3d12ce8a591e 100644
--- a/kernel/trace/trace_osnoise.c
+++ b/kernel/trace/trace_osnoise.c

@@ -2466,16 +2466,14 @@ static int timerlat_fd_open(struct inode *inode, struct file *file)
 	long cpu = (long) inode->i_cdev;
 	int ret = 0;
 
-	mutex_lock(&interface_lock);
+	guard(mutex)(&trace_types_lock);
 
 	/*
 	 * This file is accessible only if timerlat is enabled, and
 	 * NO_OSNOISE_WORKLOAD is set.
 	 */
-	if (!timerlat_enabled() || test_bit(OSN_WORKLOAD, &osnoise_options)) {
-		mutex_unlock(&interface_lock);
+	if (!timerlat_enabled() || test_bit(OSN_WORKLOAD, &osnoise_options))
 		return -EINVAL;
-	}
 
 	migrate_disable();
 

@@ -2524,7 +2522,6 @@ static int timerlat_fd_open(struct inode *inode, struct file *file)
 	 * Setup is done.
 	 */
 	mutex_unlock(&osn_var->lock);
-	mutex_unlock(&interface_lock);
 
 	tlat = this_cpu_tmr_var();
 	tlat->count = 0;

@@ -2536,7 +2533,6 @@ static int timerlat_fd_open(struct inode *inode, struct file *file)
 
 err:
 	mutex_unlock(&osn_var->lock);
-	mutex_unlock(&interface_lock);
 	migrate_enable();
 	return ret;
 };

-- 
2.54.0

We can get here on timerlat with OSN_WORKLOAD cleared via
osnoise_hotplug_workfn().

While we're moving this code, remove the pointless compiler barrier
immediately before returning from a function.  Even if it's inlined,
it's not doing anything useful.

Signed-off-by: Crystal Wood <redacted>
---
 kernel/trace/trace_osnoise.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
index a53a07e59197..0f27af6ade3e 100644
--- a/kernel/trace/trace_osnoise.c
+++ b/kernel/trace/trace_osnoise.c

@@ -2056,16 +2056,17 @@ static int start_kthread(unsigned int cpu)
 	if (osn->kthread)
 		goto out;
 
+	/* Can still happen with timerlat via cpu hotplug */
+	if (!test_bit(OSN_WORKLOAD, &osnoise_options)) {
+		if (!timerlat_enabled())
+			osn->sampling = true;
+		goto out;
+	}
+
 	if (timerlat_enabled()) {
 		snprintf(comm, 24, "timerlat/%d", cpu);
 		main = timerlat_main;
 	} else {
-		/* if no workload, just return */
-		if (!test_bit(OSN_WORKLOAD, &osnoise_options)) {
-			per_cpu(per_cpu_osnoise_var, cpu).sampling = true;
-			barrier();
-			goto out;
-		}
 		snprintf(comm, 24, "osnoise/%d", cpu);
 	}
 

-- 
2.54.0

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] Mixing `goto`-based error handling with scope-based `guard()` cleanup in `timerlat_fd_open()`.

Pre-existing issues:
- [High] Uninitialized per-CPU mutex and CPU ID for hotplugged CPUs in `init_osnoise_tracer()`.
--

--- Patch [4]: [PATCH 4/4] tracing/osnoise: Take trace_types_lock in timerlat_fd_open ---

commit d0fbb203fad6d30d667803da9a34c6adb7c10f72
Author: Crystal Wood [off-list ref]

tracing/osnoise: Take trace_types_lock in timerlat_fd_open

This commit replaces the use of interface_lock with trace_types_lock using
a guard(mutex) in timerlat_fd_open. It prevents a user fd from being opened
when osnoise_options_write has already stopped threads but not yet set
OSN_WORKLOAD.

[Severity: Low]
Does this mix goto-based error handling with scope-based guard cleanup?

The function still relies on a goto err; block for error handling (which
unlocks osn_var->lock and calls migrate_enable()), but it now uses guard
for trace_types_lock.

The cleanup guidelines suggest converting all resources to scope-based
cleanup, or none of them, to avoid mixing semantics and ownership rules
in the same routine.

[ ... ]

[Severity: High]
This is a pre-existing issue, but does init_osnoise_tracer fail to
initialize per-CPU data for hotplugged CPUs?

Looking at init_osnoise_tracer in kernel/trace/trace_osnoise.c:

kernel/trace/trace_osnoise.c:init_osnoise_tracer() {
    ...
    for_each_online_cpu(cpu) {
        struct osnoise_variables *osn = cpu_osn_var(cpu);

        mutex_init(&osn->lock);
        osn->cpu = cpu;
    }
    ...
}

Because this iterates over online CPUs rather than possible CPUs, if a CPU
is offline during boot or hot-added later, its mutex and cpu ID will remain
uninitialized.

When that CPU comes online, osnoise_hotplug_workfn calls start_kthread(cpu),
which attempts to lock the uninitialized osn->lock. Could this cause a
lockdep warning or kernel crash due to uninitialized list_head pointers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824211544.3984835-1-crwood@redhat.com?part=4

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 [off-list ref]

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.

[ ... ]

[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();
+

[ ... ]

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

[ ... ]

[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

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Deadlock / Priority Inversion in `timerlat_fd_read()` due to holding a mutex across an indefinite sleep.
- [High] Uninitialized Mutex for offline CPUs leads to crash during CPU hotplug.
- [High] AB-BA deadlock between `stop_kthread()` and `osnoise_migration_pending()`.
- [Medium] Spurious `WARN_ON` due to a contradictory double clear of `kthread_cpumask`.
--

--- Patch [1]: [PATCH 1/4] tracing/osnoise: Per-cpu mutex and fd detachment ---

commit 3d6fdd195012eb0c62c4c6d7f8152a80a0f97897
Author: Crystal Wood [off-list ref]

tracing/osnoise: Per-cpu mutex and fd detachment

Clean up a variety of synchronization issues and related bandaids
by having a per-cpu mutex that guards changes to kthread, and
fd open/close/revoke. Replace the SIGKILL hack with a mutex-protected
detachment mechanism.

[ ... ]

[ ... ]

[Severity: High]
Can holding osn_var->lock while calling kthread_stop() cause a deadlock?

kthread_stop() blocks waiting for the target kthread to exit. However, if
the kthread is currently handling a migration request in
osnoise_migration_pending() (shown slightly earlier in the diff), it will
attempt to acquire this same osn_var->lock before it can clear its state and
exit:

osnoise_migration_pending() {
    ...
    mutex_lock(&osn->lock);
    this_cpu_osn_var()->kthread = NULL;
    ...
}

If stop_kthread() holds the lock while waiting for the thread to exit, and
the kthread waits for the lock to exit, they will deadlock and hang the
system. Is there a way to call kthread_stop() outside of the mutex critical
section?

+		osn_var->kthread = NULL;
 	}
+
+	mutex_unlock(&osn_var->lock);
 }

[ ... ]

[Severity: Medium]
Will this always trigger the WARN_ON() inside stop_kthread()?

Here in start_per_cpu_kthreads(), the kthread_cpumask bit is cleared prior
to calling stop_kthread(cpu). 

But in stop_kthread(cpu) introduced above, it expects the bit to still be
set and asserts:
WARN_ON(!cpumask_test_and_clear_cpu(cpu, &kthread_cpumask));

Since the caller already cleared the bit, won't this result in a guaranteed
warning stack trace whenever this recovery path is executed?

 
 	for_each_cpu(cpu, current_mask) {
 		retval = start_kthread(cpu);

[ ... ]

[Severity: High]
Is it safe to hold osn_var->lock across a sleep in TASK_INTERRUPTIBLE?

The function acquires osn_var->lock and then calls wait_next_period(),
which sets the task state to TASK_INTERRUPTIBLE and calls schedule().

If a user task calling read() is suspended (e.g., via SIGSTOP), it could
remain in TASK_STOPPED and sleep indefinitely while holding the mutex.
This would permanently block any administrator actions like CPU hotplug
or stopping the tracer, since stop_kthread() attempts to acquire this same
lock.

[ ... ]

[Severity: High]
Does this skip possible CPUs that are offline during initialization?

If a CPU is offline at boot time, its mutex won't be initialized here.
Later, if an administrator hotplugs this CPU, osnoise_cpu_init() will
schedule a workqueue to invoke start_kthread() on the newly online CPU.
start_kthread() will immediately attempt to acquire the uninitialized lock,
leading to a system crash or lockdep splat.

Should this use for_each_possible_cpu(cpu) instead to ensure all CPUs
have their mutex properly initialized for hotplug?

+
 	cpumask_copy(&osnoise_cpumask, cpu_all_mask);
 
 	ret = register_tracer(&osnoise_tracer);

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

On Mon, 24 Aug 2026 21:30:37 +0000
sashiko-bot@kernel.org wrote:

[Severity: High]
Can holding osn_var->lock while calling kthread_stop() cause a deadlock?

kthread_stop() blocks waiting for the target kthread to exit. However, if
the kthread is currently handling a migration request in
osnoise_migration_pending() (shown slightly earlier in the diff), it will
attempt to acquire this same osn_var->lock before it can clear its state and
exit:

This code has a lot of nastiness in the locking with regard to the thread.
I haven't looked deeply at your patch, but the fact that Sashiko pointed
out a possible deadlock I think you may be hitting the mess I was hitting.

Have you looked at this thread: https://lore.kernel.org/all/20240820130001.124768-1-tglozar@redhat.com/

It has some tests I ran along with tracing that detected issues. Of course,
everything needs to be run with lockdep enabled too.

-- Steve

On Wed, 2026-08-26 at 15:31 -0400, Steven Rostedt wrote:

On Mon, 24 Aug 2026 21:30:37 +0000
sashiko-bot@kernel.org wrote:

quoted

[Severity: High]
Can holding osn_var->lock while calling kthread_stop() cause a deadlock?

kthread_stop() blocks waiting for the target kthread to exit. However, if
the kthread is currently handling a migration request in
osnoise_migration_pending() (shown slightly earlier in the diff), it will
attempt to acquire this same osn_var->lock before it can clear its state and
exit:

This code has a lot of nastiness in the locking with regard to the thread.
I haven't looked deeply at your patch, but the fact that Sashiko pointed
out a possible deadlock I think you may be hitting the mess I was hitting.

Have you looked at this thread: https://lore.kernel.org/all/20240820130001.124768-1-tglozar@redhat.com/

That thread is about user fd sync, which is what this patch is trying to
address.

This particular deadlock is with kernel threads, in a corner case of
getting migrated even though we try to pin them to one cpu.  We can get
rid of this migration code and just have stop_kthread() take care of it,
adding a get/put_task_struct() so that it's OK for the thread to die
early.  This way we can also handle any other abnormal thread exits.

I'll respond to the rest of the Sashiko comments soon.

It has some tests I ran along with tracing that detected issues. Of course,
everything needs to be run with lockdep enabled too.

I did run with lockdep (and some custom hacky state tracking) but
forcing migration is a hole in my test coverage.

Is there a particular test you're suggesting?

-Crystal

On Mon, 2026-08-24 at 21:30 +0000, sashiko-bot@kernel.org wrote:

quoted

@@ -2086,15 +2100,9 @@ static int start_per_cpu_kthreads(void)
 	 */
 	cpumask_and(current_mask, cpu_online_mask, &osnoise_cpumask);
 
-	for_each_possible_cpu(cpu) {
-		if (cpumask_test_and_clear_cpu(cpu, &kthread_cpumask)) {
-			struct task_struct *kthread;
-
-			kthread = xchg_relaxed(&(per_cpu(per_cpu_osnoise_var, cpu).kthread), NULL);
-			if (!WARN_ON(!kthread))
-				kthread_stop(kthread);
-		}
-	}
+	for_each_possible_cpu(cpu)
+		if (cpumask_test_and_clear_cpu(cpu, &kthread_cpumask))
+			stop_kthread(cpu);

[Severity: Medium]
Will this always trigger the WARN_ON() inside stop_kthread()?

Here in start_per_cpu_kthreads(), the kthread_cpumask bit is cleared prior
to calling stop_kthread(cpu). 

But in stop_kthread(cpu) introduced above, it expects the bit to still be
set and asserts:
WARN_ON(!cpumask_test_and_clear_cpu(cpu, &kthread_cpumask));

Since the caller already cleared the bit, won't this result in a guaranteed
warning stack trace whenever this recovery path is executed?

This should just be replaced by a warning, as we shouldn't be able to get
here with an active kthread.  All callers of kthread_start() and
start_per_cpu_kthreads() are under trace_types_lock.  All callers of the
latter stop all threads under the same lock.before restarting them --
except osnoise_workload_start(), which still holds the lock from when
the isntance was first initialized.

quoted

@@ -2528,6 +2577,13 @@ timerlat_fd_read(struct file *file, char __user *ubuf, size_t count,
 
 	osn_var = this_cpu_osn_var();
 
+	/* In normal usage, this should always be uncontended. */
+	mutex_lock(&osn_var->lock);
+	if (fdd != osn_var->fdd || current != fdd->thread) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	/*
 	 * The timerlat in user-space runs in a different order:
 	 * the read() starts from the execution of the previous occurrence,

@@ -2574,6 +2630,11 @@ timerlat_fd_read(struct file *file, char __user *ubuf, size_t count,
 	/* wait for the next period */
 	wait_next_period(tlat);
 
+	if (fdd != osn_var->fdd) {
+		ret = -EINVAL;
+		goto out;
+	}

[Severity: High]
Is it safe to hold osn_var->lock across a sleep in TASK_INTERRUPTIBLE?

The function acquires osn_var->lock and then calls wait_next_period(),
which sets the task state to TASK_INTERRUPTIBLE and calls schedule().

If a user task calling read() is suspended (e.g., via SIGSTOP), it could
remain in TASK_STOPPED and sleep indefinitely while holding the mutex.
This would permanently block any administrator actions like CPU hotplug
or stopping the tracer, since stop_kthread() attempts to acquire this same
lock.

That's not how signals work.

[ ... ]

quoted

@@ -3157,10 +3203,13 @@ __init static int init_timerlat_tracer(void)
 
 __init static int init_osnoise_tracer(void)
 {
-	int ret;
+	int ret, cpu;
 
 	mutex_init(&interface_lock);
 
+	for_each_online_cpu(cpu)
+		mutex_init(&cpu_osn_var(cpu)->lock);

[Severity: High]
Does this skip possible CPUs that are offline during initialization?

If a CPU is offline at boot time, its mutex won't be initialized here.
Later, if an administrator hotplugs this CPU, osnoise_cpu_init() will
schedule a workqueue to invoke start_kthread() on the newly online CPU.
start_kthread() will immediately attempt to acquire the uninitialized lock,
leading to a system crash or lockdep splat.

Should this use for_each_possible_cpu(cpu) instead to ensure all CPUs
have their mutex properly initialized for hotplug?

Yes, will fix.  We should probably do the same in osn_var_reset() and
tlat_var_reset().

-Crystal

On Wed, 26 Aug 2026 16:19:29 -0500
Crystal Wood [off-list ref] wrote:

quoted

It has some tests I ran along with tracing that detected issues. Of course,
everything needs to be run with lockdep enabled too.  

I did run with lockdep (and some custom hacky state tracking) but
forcing migration is a hole in my test coverage.

Is there a particular test you're suggesting?

I think I mostly did:

  $ while true; do rtla timerlat top -u -q & PID=$!; sleep 5;

or variations of it. IIRC, that would cause a lockdep splat here or
there.

-- Steve

st 26. 8. 2026 v 23:21 odesílatel Crystal Wood [off-list ref] napsal:

On Wed, 2026-08-26 at 15:31 -0400, Steven Rostedt wrote:

quoted

On Mon, 24 Aug 2026 21:30:37 +0000
sashiko-bot@kernel.org wrote:

quoted

[Severity: High]
Can holding osn_var->lock while calling kthread_stop() cause a deadlock?

kthread_stop() blocks waiting for the target kthread to exit. However, if
the kthread is currently handling a migration request in
osnoise_migration_pending() (shown slightly earlier in the diff), it will
attempt to acquire this same osn_var->lock before it can clear its state and
exit:

This code has a lot of nastiness in the locking with regard to the thread.
I haven't looked deeply at your patch, but the fact that Sashiko pointed
out a possible deadlock I think you may be hitting the mess I was hitting.

Have you looked at this thread: https://lore.kernel.org/all/20240820130001.124768-1-tglozar@redhat.com/

That thread is about user fd sync, which is what this patch is trying to
address.

This particular deadlock is with kernel threads, in a corner case of
getting migrated even though we try to pin them to one cpu.  We can get
rid of this migration code and just have stop_kthread() take care of it,
adding a get/put_task_struct() so that it's OK for the thread to die
early.  This way we can also handle any other abnormal thread exits.

I'll respond to the rest of the Sashiko comments soon.

quoted

It has some tests I ran along with tracing that detected issues. Of course,
everything needs to be run with lockdep enabled too.

I did run with lockdep (and some custom hacky state tracking) but
forcing migration is a hole in my test coverage.

Is there a particular test you're suggesting?

-Crystal

The race window is practically really small. I had success of
reproducing the migration deadlock in a virtual machine with a bash
script [1] that deliberately slows down the kernel/osnoise tracer with
perf, ftrace, and bpftrace hooks:

[root@cs9 tglozar]#
/home/tglozar/dev/linux/tools/tracing/rtla/osnoise_migration_race.sh
13 10000
cpu=13 mask=0,1,2,3,4,5,6,7,8,9,10,11,12 iters=10000 stop=nop
iter 1 ok (kpid 265080)
iter 2 ok (kpid 265086)
...
iter 422 ok (kpid 267814)
*** hang iter 423  kthread=267821  affiner=267823  stopper=267827
    cat /proc/267821/stack /proc/267827/stack
    do not rmdir instances/ or write current_tracer/online; reboot
[root@cs9 tglozar]# cat /proc/267821/stack; echo -------; cat /proc/267827/stack
[<0>] timerlat_main+0x266/0x3f0
[<0>] kthread+0xe6/0x120
[<0>] ret_from_fork+0x1be/0x250
[<0>] ret_from_fork_asm+0x1a/0x30
-------
[<0>] kthread_stop+0x6b/0x180
[<0>] stop_kthread+0x86/0xc0
[<0>] stop_per_cpu_kthreads+0xf/0x40
[<0>] osnoise_workload_stop.part.0+0xc/0xa0
[<0>] timerlat_tracer_reset+0x9/0x40
[<0>] tracing_set_tracer+0x14d/0x300
[<0>] tracing_set_trace_write+0x7f/0xe0
[<0>] vfs_write+0xf8/0x410
[<0>] ksys_write+0x65/0xe0
[<0>] do_syscall_64+0xc1/0x460
[<0>] entry_SYSCALL_64_after_hwframe+0x76/0x7e

[1] https://gitlab.com/-/snippets/6045758

Note that any path that is calling kthread_stop() can trigger the
deadlock (together with the migration), that is, both making the CPU
offline and stopping the osnoise tracer. Sashiko flagged the former,
my reproducer used the latter (the script can do both).



Tomas

On Mon, Aug 24, 2026 at 11:16 PM Crystal Wood [off-list ref] wrote:

Clean up a variety of synchronization issues and related bandaids
by having a per-cpu mutex that guards changes to kthread, and
fd open/close/revoke.

Replace the SIGKILL hack for userspace timerlat threads (that doesn't
even work, because we don't wait for the process to actually die) with
a mutex-protected detachment mechanism.  The mutex should be uncontended
during normal timerlat_fd_read() usage.

I agree with detaching the file descriptor, that seems to be the right
pattern. An alternative would be to make the user process to block the
tracer until it detaches, but that would make it less consistent with
kernel thread mode - which owns (creates/stops) the threads - as well
to make it more prone to locking issues during user process exit.

Can we perhaps clean up the synchronization issues without using a
mutex? If one thread is switching timerlat between no thread, user
thread, and kernel thread, it could mark the osn percpu structure
"busy" and reject all other switching operations until it is finished.
As you say, the resource should be uncontended during normal usage, so
the user shouldn't care about being returned an error instead of
waiting. Or am I missing something?

Signed-off-by: Crystal Wood <redacted>
---
 kernel/trace/trace_osnoise.c | 239 +++++++++++++++++++++--------------
 1 file changed, 144 insertions(+), 95 deletions(-)

[truncated]

Tomas

On Tue, 2026-09-01 at 15:32 +0200, Tomas Glozar wrote:

On Mon, Aug 24, 2026 at 11:16 PM Crystal Wood [off-list ref] wrote:

quoted

Clean up a variety of synchronization issues and related bandaids
by having a per-cpu mutex that guards changes to kthread, and
fd open/close/revoke.

Replace the SIGKILL hack for userspace timerlat threads (that doesn't
even work, because we don't wait for the process to actually die) with
a mutex-protected detachment mechanism.  The mutex should be uncontended
during normal timerlat_fd_read() usage.

I agree with detaching the file descriptor, that seems to be the right
pattern. An alternative would be to make the user process to block the
tracer until it detaches, but that would make it less consistent with
kernel thread mode - which owns (creates/stops) the threads - as well
to make it more prone to locking issues during user process exit.

Can we perhaps clean up the synchronization issues without using a0
mutex? If one thread is switching timerlat between no thread, user
thread, and kernel thread, it could mark the osn percpu structure
"busy" and reject all other switching operations until it is finished.
As you say, the resource should be uncontended during normal usage, so
the user shouldn't care about being returned an error instead of
waiting. Or am I missing something?

The issue is waiting for the completion of an fd operation that has
already passed the "busy" check.  Maybe something custom could be done,
but a mutex seemed simplest at the time.

That said, there is another issue with this patch in its current state,
in that you need to wait for the timer to wake the timerlat thread in
order to get the mutex to detach (and maybe longer depending on mutex
fairness issues).  So it could take a long time if the user sets a long
period.

I've also recently seen some additional issues that I'm in the process
of debugging.

-Crystal