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

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

[Crystal Wood]

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

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

[Crystal Wood]

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

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

[Crystal Wood]

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

[PATCH 3/4] tracing/osnoise: start_kthread: Always check OSN_WORKLOAD

[Crystal Wood]

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

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

[Crystal Wood]

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

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

[sashiko-bot]

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

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.

> diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
> index 0f27af6ade3e1..3d12ce8a591e7 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);

[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

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

[sashiko-bot]

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

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

[sashiko-bot]

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

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.

> diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
> index 0e1265acd1ccc..e2e1ef3f5a617 100644
> --- a/kernel/trace/trace_osnoise.c
> +++ b/kernel/trace/trace_osnoise.c

[ ... ]

> @@ -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);

[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);
>  }

[ ... ]

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

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

[ ... ]

> @@ -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.

[ ... ]

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

> +
>  	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