Message-ID: <20260129113452.190d4d79@gandalf.local.home>
Date: Thu, 29 Jan 2026 11:34:52 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: Vincent Donnefort <vdonnefort@...gle.com>
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 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?


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