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 <[email protected]> --- v2: convert rest of function to guards 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 | 43 +++++++++--------------------------- 1 file changed, 10 insertions(+), 33 deletions(-) diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c index 92b1216d5740..eaecd56492e3 100644 --- a/kernel/trace/trace_osnoise.c +++ b/kernel/trace/trace_osnoise.c @@ -2497,28 +2497,23 @@ static int timerlat_fd_open(struct inode *inode, struct file *file) struct osnoise_variables *osn_var; struct timerlat_variables *tlat; 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(); + guard(migrate)(); osn_var = this_cpu_osn_var(); - mutex_lock(&osn_var->lock); + guard(mutex)(&osn_var->lock); - if (osn_var->fdd) { - ret = -EBUSY; - goto err; - } + if (osn_var->fdd) + return -EBUSY; WARN_ON_ONCE(osn_var->kthread); @@ -2529,16 +2524,12 @@ static int timerlat_fd_open(struct inode *inode, struct file *file) * worth doing this basic acceptance test to avoid obviusly wrong * setup. */ - if (current->nr_cpus_allowed > 1 || cpu != smp_processor_id()) { - ret = -EPERM; - goto err; - } + if (current->nr_cpus_allowed > 1 || cpu != smp_processor_id()) + return -EPERM; fdd = kmalloc(sizeof(*fdd), GFP_KERNEL); - if (!fdd) { - ret = -ENOMEM; - goto err; - } + if (!fdd) + return -ENOMEM; /* * From now on, it is good to go. @@ -2553,25 +2544,11 @@ static int timerlat_fd_open(struct inode *inode, struct file *file) 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(); tlat->count = 0; hrtimer_setup(&tlat->timer, timerlat_irq, CLOCK_MONOTONIC, HRTIMER_MODE_ABS_PINNED_HARD); - - migrate_enable(); return 0; - -err: - mutex_unlock(&osn_var->lock); - mutex_unlock(&interface_lock); - migrate_enable(); - return ret; }; /* -- 2.54.0
