| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aX37eRjYBP4WmyS6@google.com>
Date: Sat, 31 Jan 2026 12:54:17 +0000
From: Vincent Donnefort <vdonnefort@...gle.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: mhiramat@...nel.org, mathieu.desnoyers@...icios.com,
linux-trace-kernel@...r.kernel.org, maz@...nel.org,
oliver.upton@...ux.dev, joey.gouly@....com, suzuki.poulose@....com,
yuzenghui@...wei.com, kvmarm@...ts.linux.dev,
linux-arm-kernel@...ts.infradead.org, jstultz@...gle.com,
qperret@...gle.com, will@...nel.org, aneesh.kumar@...nel.org,
kernel-team@...roid.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v10 14/30] tracing: Add a trace remote module for testing
On Thu, Jan 29, 2026 at 11:34:52AM -0500, Steven Rostedt wrote:
> On Mon, 26 Jan 2026 10:44:03 +0000
> Vincent Donnefort <vdonnefort@...gle.com> wrote:
>
> > diff --git a/kernel/trace/remote_test.c b/kernel/trace/remote_test.c
> > new file mode 100644
> > index 000000000000..059127489c99
> > --- /dev/null
> > +++ b/kernel/trace/remote_test.c
> > @@ -0,0 +1,259 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2025 - Google LLC
> > + * Author: Vincent Donnefort <vdonnefort@...gle.com>
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/simple_ring_buffer.h>
> > +#include <linux/trace_remote.h>
> > +#include <linux/tracefs.h>
> > +#include <linux/types.h>
> > +
> > +#define REMOTE_EVENT_INCLUDE_FILE kernel/trace/remote_test_events.h
> > +#include <trace/define_remote_events.h>
> > +
> > +static DEFINE_PER_CPU(struct simple_rb_per_cpu *, simple_rbs);
> > +static struct trace_buffer_desc *remote_test_buffer_desc;
> > +
> > +/*
> > + * The trace_remote lock already serializes accesses from the trace_remote_callbacks.
> > + * However write_event can still race with load/unload.
> > + */
> > +static DEFINE_MUTEX(simple_rbs_lock);
> > +
> > +static int remote_test_load_simple_rb(int cpu, struct ring_buffer_desc *rb_desc)
> > +{
> > + struct simple_rb_per_cpu *cpu_buffer;
> > + struct simple_buffer_page *bpages;
> > + int ret = -ENOMEM;
> > +
> > + cpu_buffer = kmalloc(sizeof(*cpu_buffer), GFP_KERNEL);
> > + if (!cpu_buffer)
> > + return ret;
> > +
> > + bpages = kmalloc_array(rb_desc->nr_page_va, sizeof(*bpages), GFP_KERNEL);
> > + if (!bpages)
> > + goto err_free_cpu_buffer;
> > +
> > + ret = simple_ring_buffer_init(cpu_buffer, bpages, rb_desc);
> > + if (ret)
> > + goto err_free_bpages;
> > +
> > + scoped_guard(mutex, &simple_rbs_lock)
> > + *per_cpu_ptr(&simple_rbs, cpu) = cpu_buffer;
>
> Should there be some kind of check before blindly assigning the cpu_buffer?
> If not, what is the mutex protecting from?
This intends to order accesses with write_event_write(). trace_remote ensures
that load/unload are always balanced, so I don't believe there's anything to
test.
I could add a WARN_ON though. and probably guard(mutex)(&simple_rbs_lock) is
enough.
>
>
> > +
> > + return 0;
> > +
> > +err_free_bpages:
> > + kfree(bpages);
> > +
> > +err_free_cpu_buffer:
> > + kfree(cpu_buffer);
> > +
> > + return ret;
> > +}
> > +
> > +static void remote_test_unload_simple_rb(int cpu)
> > +{
> > + struct simple_rb_per_cpu *cpu_buffer = *per_cpu_ptr(&simple_rbs, cpu);
> > + struct simple_buffer_page *bpages;
> > +
> > + if (!cpu_buffer)
> > + return;
> > +
> > + guard(mutex)(&simple_rbs_lock);
> > +
> > + bpages = cpu_buffer->bpages;
> > + simple_ring_buffer_unload(cpu_buffer);
> > + kfree(bpages);
> > + kfree(cpu_buffer);
> > + *per_cpu_ptr(&simple_rbs, cpu) = NULL;
> > +}
> > +
> > +static struct trace_buffer_desc *remote_test_load(unsigned long size, void *unused)
> > +{
> > + struct ring_buffer_desc *rb_desc;
> > + struct trace_buffer_desc *desc;
> > + size_t desc_size;
> > + int cpu, ret;
> > +
> > + if (WARN_ON(remote_test_buffer_desc))
> > + return ERR_PTR(-EINVAL);
> > +
> > + desc_size = trace_buffer_desc_size(size, num_possible_cpus());
> > + if (desc_size == SIZE_MAX) {
> > + ret = -E2BIG;
> > + goto err_unlock_cpus;
> > + }
> > +
> > + desc = kmalloc(desc_size, GFP_KERNEL);
> > + if (!desc) {
> > + ret = -ENOMEM;
> > + goto err_unlock_cpus;
> > + }
> > +
> > + ret = trace_remote_alloc_buffer(desc, desc_size, size, cpu_possible_mask);
> > + if (ret)
> > + goto err_free_desc;
> > +
> > + for_each_ring_buffer_desc(rb_desc, cpu, desc) {
> > + ret = remote_test_load_simple_rb(rb_desc->cpu, rb_desc);
> > + if (ret)
> > + goto err;
> > + }
> > +
> > + remote_test_buffer_desc = desc;
> > +
> > + return remote_test_buffer_desc;
> > +
> > +err:
> > + for_each_ring_buffer_desc(rb_desc, cpu, remote_test_buffer_desc)
> > + remote_test_unload_simple_rb(rb_desc->cpu);
> > + trace_remote_free_buffer(remote_test_buffer_desc);
> > +
> > +err_free_desc:
> > + kfree(desc);
> > +
> > +err_unlock_cpus:
>
> Where was the cpus_read lock taken?
>
> > + cpus_read_unlock();
> > +
> > + return ERR_PTR(ret);
> > +}
> > +
> > +static void remote_test_unload(struct trace_buffer_desc *desc, void *unused)
> > +{
> > + struct ring_buffer_desc *rb_desc;
> > + int cpu;
> > +
> > + if (WARN_ON(desc != remote_test_buffer_desc))
> > + return;
> > +
> > + for_each_ring_buffer_desc(rb_desc, cpu, desc)
> > + remote_test_unload_simple_rb(rb_desc->cpu);
> > +
> > + remote_test_buffer_desc = NULL;
> > + trace_remote_free_buffer(desc);
> > + kfree(desc);
> > +}
> > +
> > +static int remote_test_enable_tracing(bool enable, void *unused)
> > +{
> > + struct ring_buffer_desc *rb_desc;
> > + int cpu;
> > +
> > + if (!remote_test_buffer_desc)
> > + return -ENODEV;
> > +
> > + for_each_ring_buffer_desc(rb_desc, cpu, remote_test_buffer_desc)
> > + WARN_ON(simple_ring_buffer_enable_tracing(*per_cpu_ptr(&simple_rbs, rb_desc->cpu),
> > + enable));
> > + return 0;
> > +}
> > +
> > +static int remote_test_swap_reader_page(unsigned int cpu, void *unused)
> > +{
> > + struct simple_rb_per_cpu *cpu_buffer;
> > +
> > + if (cpu >= NR_CPUS)
> > + return -EINVAL;
> > +
> > + cpu_buffer = *per_cpu_ptr(&simple_rbs, cpu);
> > + if (!cpu_buffer)
> > + return -EINVAL;
> > +
> > + return simple_ring_buffer_swap_reader_page(cpu_buffer);
> > +}
> > +
> > +static int remote_test_reset(unsigned int cpu, void *unused)
> > +{
> > + struct simple_rb_per_cpu *cpu_buffer;
> > +
> > + if (cpu >= NR_CPUS)
> > + return -EINVAL;
> > +
> > + cpu_buffer = *per_cpu_ptr(&simple_rbs, cpu);
> > + if (!cpu_buffer)
> > + return -EINVAL;
> > +
> > + return simple_ring_buffer_reset(cpu_buffer);
> > +}
> > +
> > +static int remote_test_enable_event(unsigned short id, bool enable, void *unused)
> > +{
> > + if (id != REMOTE_TEST_EVENT_ID)
> > + return -EINVAL;
> > +
> > + /*
> > + * Let's just use the struct remote_event enabled field that is turned on and off by
> > + * trace_remote. This is a bit racy but good enough for a simple test module.
> > + */
> > + return 0;
> > +}
> > +
> > +static ssize_t
> > +write_event_write(struct file *filp, const char __user *ubuf, size_t cnt, loff_t *pos)
> > +{
> > + struct remote_event_format_selftest *evt_test;
> > + struct simple_rb_per_cpu *cpu_buffer;
> > + unsigned long val;
> > + int ret;
> > +
> > + ret = kstrtoul_from_user(ubuf, cnt, 10, &val);
> > + if (ret)
> > + return ret;
> > +
> > + guard(mutex)(&simple_rbs_lock);
> > +
> > + if (!remote_event_selftest.enabled)
> > + return -ENODEV;
> > +
>
> You want a guard(preempt)(); here...
>
> > + cpu_buffer = *this_cpu_ptr(&simple_rbs);
>
> Otherwise this triggers:
>
> BUG: using smp_processor_id() in preemptible [00000000] code: bash/1096
> caller is write_event_write+0xe0/0x230 [remote_test]
>
> -- Steve
>
> > + if (!cpu_buffer)
> > + return -ENODEV;
> > +
> > + evt_test = simple_ring_buffer_reserve(cpu_buffer,
> > + sizeof(struct remote_event_format_selftest),
> > + trace_clock_global());
> > + if (!evt_test)
> > + return -ENODEV;
> > +
> > + evt_test->hdr.id = REMOTE_TEST_EVENT_ID;
> > + evt_test->id = val;
> > +
> > + simple_ring_buffer_commit(cpu_buffer);
> > +
> > + return cnt;
> > +}
> > +
Powered by blists - more mailing lists