* Re: [PATCH] tracing: Remove unused variable in tracing_trace_options_show()
From: Andy Shevchenko @ 2025-11-17 17:22 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, Linux Trace Kernel, Masami Hiramatsu, Mathieu Desnoyers
In-Reply-To: <20251117120637.43ef995d@gandalf.local.home>
On Mon, Nov 17, 2025 at 12:06:37PM -0500, Steven Rostedt wrote:
> The flags and opts used in tracing_trace_options_show() now come directly
> from the trace array "current_trace_flags" and not the current_trace. The
> variable "trace" was still being assigned to tr->current_trace but never
> used. This caused a warning in clang.
Tested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH] tracing: Remove unused variable in tracing_trace_options_show()
From: Steven Rostedt @ 2025-11-17 17:06 UTC (permalink / raw)
To: LKML, Linux Trace Kernel
Cc: Masami Hiramatsu, Mathieu Desnoyers, Andy Shevchenko
From: Steven Rostedt <rostedt@goodmis.org>
The flags and opts used in tracing_trace_options_show() now come directly
from the trace array "current_trace_flags" and not the current_trace. The
variable "trace" was still being assigned to tr->current_trace but never
used. This caused a warning in clang.
Reported-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Closes: https://lore.kernel.org/all/aRtHWXzYa8ijUIDa@black.igk.intel.com/
Fixes: 428add559b692 ("tracing: Have tracer option be instance specific")
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
kernel/trace/trace.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 8ae95800592d..59cd4ed8af6d 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -5167,7 +5167,6 @@ static int tracing_trace_options_show(struct seq_file *m, void *v)
struct tracer_opt *trace_opts;
struct trace_array *tr = m->private;
struct tracer_flags *flags;
- struct tracer *trace;
u32 tracer_flags;
int i;
@@ -5184,8 +5183,6 @@ static int tracing_trace_options_show(struct seq_file *m, void *v)
if (!flags || !flags->opts)
return 0;
- trace = tr->current_trace;
-
tracer_flags = flags->val;
trace_opts = flags->opts;
--
2.51.0
^ permalink raw reply related
* Re: [PATCH v4 1/3] tracing: Have tracer option be instance specific
From: Steven Rostedt @ 2025-11-17 16:32 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Steven Rostedt, linux-kernel, linux-trace-kernel,
Masami Hiramatsu, Mark Rutland, Mathieu Desnoyers, Andrew Morton
In-Reply-To: <aRtHWXzYa8ijUIDa@black.igk.intel.com>
On Mon, 17 Nov 2025 17:03:37 +0100
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > @@ -5152,12 +5180,14 @@ static int tracing_trace_options_show(struct seq_file *m, void *v)
> > seq_printf(m, "no%s\n", trace_options[i]);
> > }
> >
> > - trace = tr->current_trace;
> > - if (!trace->flags || !trace->flags->opts)
> > + flags = tr->current_trace_flags;
> > + if (!flags || !flags->opts)
> > return 0;
> >
> > - tracer_flags = tr->current_trace->flags->val;
> > - trace_opts = tr->current_trace->flags->opts;
>
> > + trace = tr->current_trace;
>
> Removing it and this line fixes, but I'm not sure that's correct one, so feel
> free to fold the change or if it will be a fix,
> Reported-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Thanks for the report. Yeah, that variable is no longer used and can be
removed. I'll send a patch.
-- Steve
>
> > + tracer_flags = flags->val;
> > + trace_opts = flags->opts;
^ permalink raw reply
* Re: [PATCH v4 1/3] tracing: Have tracer option be instance specific
From: Andy Shevchenko @ 2025-11-17 16:03 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, linux-trace-kernel, Masami Hiramatsu, Mark Rutland,
Mathieu Desnoyers, Andrew Morton
In-Reply-To: <20251111232429.305317942@kernel.org>
On Tue, Nov 11, 2025 at 06:24:07PM -0500, Steven Rostedt wrote:
> Tracers can add specify options to modify them. This logic was added
> before instances were created and the tracer flags were global variables.
> After instances were created where a tracer may exist in more than one
> instance, the flags were not updated from being global into instance
> specific. This causes confusion with these options. For example, the
> function tracer has an option to enable function arguments:
>
> # cd /sys/kernel/tracing
> # mkdir instances/foo
> # echo function > instances/foo/current_tracer
> # echo 1 > options/func-args
> # echo function > current_tracer
> # cat trace
> [..]
> <idle>-0 [005] d..3. 1050.656187: rcu_needs_cpu() <-tick_nohz_next_event
> <idle>-0 [005] d..3. 1050.656188: get_next_timer_interrupt(basej=0x10002dbad, basem=0xf45fd7d300) <-tick_nohz_next_event
> <idle>-0 [005] d..3. 1050.656189: _raw_spin_lock(lock=0xffff8944bdf5de80) <-__get_next_timer_interrupt
> <idle>-0 [005] d..4. 1050.656190: do_raw_spin_lock(lock=0xffff8944bdf5de80) <-__get_next_timer_interrupt
> <idle>-0 [005] d..4. 1050.656191: _raw_spin_lock_nested(lock=0xffff8944bdf5f140, subclass=1) <-__get_next_timer_interrupt
> # cat instances/foo/options/func-args
> 1
> # cat instances/foo/trace
> [..]
> kworker/4:1-88 [004] ...1. 298.127735: next_zone <-refresh_cpu_vm_stats
> kworker/4:1-88 [004] ...1. 298.127736: first_online_pgdat <-refresh_cpu_vm_stats
> kworker/4:1-88 [004] ...1. 298.127738: next_online_pgdat <-refresh_cpu_vm_stats
> kworker/4:1-88 [004] ...1. 298.127739: fold_diff <-refresh_cpu_vm_stats
> kworker/4:1-88 [004] ...1. 298.127741: round_jiffies_relative <-vmstat_update
> [..]
>
> The above shows that setting "func-args" in the top level instance also
> set it in the instance "foo", but since the interface of the trace flags
> are per instance, the update didn't take affect in the "foo" instance.
>
> Update the infrastructure to allow tracers to add a "default_flags" field
> in the tracer structure that can be set instead of "flags" which will make
> the flags per instance. If a tracer needs to keep the flags global (like
> blktrace), keeping the "flags" field set will keep the old behavior.
>
> This does not update function or the function graph tracers. That will be
> handled later.
This broke clang build.
> {
> struct tracer_opt *trace_opts;
> struct trace_array *tr = m->private;
> + struct tracer_flags *flags;
> struct tracer *trace;
Now this is dangling variable. (Set but not used)
> u32 tracer_flags;
> int i;
> @@ -5152,12 +5180,14 @@ static int tracing_trace_options_show(struct seq_file *m, void *v)
> seq_printf(m, "no%s\n", trace_options[i]);
> }
>
> - trace = tr->current_trace;
> - if (!trace->flags || !trace->flags->opts)
> + flags = tr->current_trace_flags;
> + if (!flags || !flags->opts)
> return 0;
>
> - tracer_flags = tr->current_trace->flags->val;
> - trace_opts = tr->current_trace->flags->opts;
> + trace = tr->current_trace;
Removing it and this line fixes, but I'm not sure that's correct one, so feel
free to fold the change or if it will be a fix,
Reported-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> + tracer_flags = flags->val;
> + trace_opts = flags->opts;
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 2/2] rv: Convert to use __free
From: Nam Cao @ 2025-11-17 15:20 UTC (permalink / raw)
To: Steven Rostedt
Cc: Gabriele Monaco, Masami Hiramatsu, Mathieu Desnoyers,
linux-trace-kernel, linux-kernel
In-Reply-To: <20251117100735.71466d12@gandalf.local.home>
Steven Rostedt <rostedt@goodmis.org> writes:
> On Sun, 16 Nov 2025 15:35:12 +0000
> Nam Cao <namcao@linutronix.de> wrote:
>> + mon->root_d = dir;
>> + retain_and_null_ptr(dir);
>> return 0;
>
> Why the "retain_and_null_ptr() and not just:
>
> mon->root-d = no_free_ptr(dir);
> return 0;
>
> As from my understanding is that the retain_and_null_ptr() is for use of
> passing the variable to a function that will consume it. But for assigning
> to a variable, I usually just use the no_free_ptr().
I wasn't aware that no_free_ptr() exists. Gabriele has pointed this out,
and I fixed it up in v2.
Nam
^ permalink raw reply
* Re: [PATCH v3 1/8] mm: introduce VM_MAYBE_GUARD and make visible in /proc/$pid/smaps
From: David Hildenbrand (Red Hat) @ 2025-11-17 15:11 UTC (permalink / raw)
To: Lorenzo Stoakes, Andrew Morton
Cc: Jonathan Corbet, Liam R . Howlett, Vlastimil Babka, Mike Rapoport,
Suren Baghdasaryan, Michal Hocko, Steven Rostedt,
Masami Hiramatsu, Mathieu Desnoyers, Jann Horn, Pedro Falcato,
Zi Yan, Baolin Wang, Nico Pache, Ryan Roberts, Dev Jain,
Barry Song, Lance Yang, linux-kernel, linux-fsdevel, linux-doc,
linux-mm, linux-trace-kernel, linux-kselftest, Andrei Vagin
In-Reply-To: <94d1e9c6c6dd8a4de1f2a8022ca92e2e320730ff.1762531708.git.lorenzo.stoakes@oracle.com>
On 07.11.25 17:11, Lorenzo Stoakes wrote:
> Currently, if a user needs to determine if guard regions are present in a
> range, they have to scan all VMAs (or have knowledge of which ones might
> have guard regions).
>
> Since commit 8e2f2aeb8b48 ("fs/proc/task_mmu: add guard region bit to
> pagemap") and the related commit a516403787e0 ("fs/proc: extend the
> PAGEMAP_SCAN ioctl to report guard regions"), users can use either
> /proc/$pid/pagemap or the PAGEMAP_SCAN functionality to perform this
> operation at a virtual address level.
>
> This is not ideal, and it gives no visibility at a /proc/$pid/smaps level
> that guard regions exist in ranges.
>
> This patch remedies the situation by establishing a new VMA flag,
> VM_MAYBE_GUARD, to indicate that a VMA may contain guard regions (it is
> uncertain because we cannot reasonably determine whether a
> MADV_GUARD_REMOVE call has removed all of the guard regions in a VMA, and
> additionally VMAs may change across merge/split).
>
> We utilise 0x800 for this flag which makes it available to 32-bit
> architectures also, a flag that was previously used by VM_DENYWRITE, which
> was removed in commit 8d0920bde5eb ("mm: remove VM_DENYWRITE") and hasn't
> bee reused yet.
>
> We also update the smaps logic and documentation to identify these VMAs.
>
> Another major use of this functionality is that we can use it to identify
> that we ought to copy page tables on fork.
>
> We do not actually implement usage of this flag in mm/madvise.c yet as we
> need to allow some VMA flags to be applied atomically under mmap/VMA read
> lock in order to avoid the need to acquire a write lock for this purpose.
>
> Reviewed-by: Pedro Falcato <pfalcato@suse.de>
> Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
> Documentation/filesystems/proc.rst | 5 +++--
> fs/proc/task_mmu.c | 1 +
> include/linux/mm.h | 3 +++
> include/trace/events/mmflags.h | 1 +
> mm/memory.c | 4 ++++
> tools/testing/vma/vma_internal.h | 1 +
> 6 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
> index 0b86a8022fa1..8256e857e2d7 100644
> --- a/Documentation/filesystems/proc.rst
> +++ b/Documentation/filesystems/proc.rst
> @@ -553,7 +553,7 @@ otherwise.
> kernel flags associated with the particular virtual memory area in two letter
> encoded manner. The codes are the following:
>
> - == =======================================
> + == =============================================================
> rd readable
> wr writeable
> ex executable
> @@ -591,7 +591,8 @@ encoded manner. The codes are the following:
> sl sealed
> lf lock on fault pages
> dp always lazily freeable mapping
> - == =======================================
> + gu maybe contains guard regions (if not set, definitely doesn't)
> + == =============================================================
In general LGTM, BUT in the context of this patch where the flag is
never set, that's not entirely correct ;) It made sense after staring at
patch #5.
Acked-by: David Hildenbrand (Red Hat) <david@kernel.org>
--
Cheers
David
^ permalink raw reply
* Re: [PATCH 2/2] rv: Convert to use __free
From: Steven Rostedt @ 2025-11-17 15:07 UTC (permalink / raw)
To: Nam Cao
Cc: Gabriele Monaco, Masami Hiramatsu, Mathieu Desnoyers,
linux-trace-kernel, linux-kernel
In-Reply-To: <6b2a618815b45ac4ac09976ef4fb0bd3635c143d.1763306824.git.namcao@linutronix.de>
On Sun, 16 Nov 2025 15:35:12 +0000
Nam Cao <namcao@linutronix.de> wrote:
> Convert to use __free to tidy up the code.
>
> Signed-off-by: Nam Cao <namcao@linutronix.de>
> ---
> kernel/trace/rv/rv.c | 65 +++++++++++++++--------------------
> kernel/trace/rv/rv.h | 6 ++--
> kernel/trace/rv/rv_reactors.c | 32 ++++++++---------
> 3 files changed, 46 insertions(+), 57 deletions(-)
>
> diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c
> index b1059a3cf4fa..e83dc9f0c7bb 100644
> --- a/kernel/trace/rv/rv.c
> +++ b/kernel/trace/rv/rv.c
> @@ -420,35 +420,28 @@ static const struct file_operations interface_desc_fops = {
> static int create_monitor_dir(struct rv_monitor *mon, struct rv_monitor *parent)
> {
> struct dentry *root = parent ? parent->root_d : get_monitors_root();
> - const char *name = mon->name;
> + struct dentry *dir __free(rv_remove) = rv_create_dir(mon->name, root);
> struct dentry *tmp;
> int retval;
>
> - mon->root_d = rv_create_dir(name, root);
> - if (!mon->root_d)
> + if (!dir)
> return -ENOMEM;
>
> - tmp = rv_create_file("enable", RV_MODE_WRITE, mon->root_d, mon, &interface_enable_fops);
> - if (!tmp) {
> - retval = -ENOMEM;
> - goto out_remove_root;
> - }
> + tmp = rv_create_file("enable", RV_MODE_WRITE, dir, mon, &interface_enable_fops);
> + if (!tmp)
> + return -ENOMEM;
>
> - tmp = rv_create_file("desc", RV_MODE_READ, mon->root_d, mon, &interface_desc_fops);
> - if (!tmp) {
> - retval = -ENOMEM;
> - goto out_remove_root;
> - }
> + tmp = rv_create_file("desc", RV_MODE_READ, dir, mon, &interface_desc_fops);
> + if (!tmp)
> + return -ENOMEM;
>
> - retval = reactor_populate_monitor(mon);
> + retval = reactor_populate_monitor(mon, dir);
> if (retval)
> - goto out_remove_root;
> + return retval;
>
> + mon->root_d = dir;
> + retain_and_null_ptr(dir);
> return 0;
Why the "retain_and_null_ptr() and not just:
mon->root-d = no_free_ptr(dir);
return 0;
As from my understanding is that the retain_and_null_ptr() is for use of
passing the variable to a function that will consume it. But for assigning
to a variable, I usually just use the no_free_ptr().
> -
> -out_remove_root:
> - rv_remove(mon->root_d);
> - return retval;
> }
>
> /*
> @@ -827,39 +820,37 @@ int __init rv_init_interface(void)
> {
> struct dentry *tmp;
> int retval;
> + struct dentry *root_dir __free(rv_remove) = rv_create_dir("rv", NULL);
>
> - rv_root.root_dir = rv_create_dir("rv", NULL);
> - if (!rv_root.root_dir)
> - goto out_err;
> + if (!root_dir)
> + return 1;
>
> - rv_root.monitors_dir = rv_create_dir("monitors", rv_root.root_dir);
> + rv_root.monitors_dir = rv_create_dir("monitors", root_dir);
> if (!rv_root.monitors_dir)
> - goto out_err;
> + return 1;
>
> - tmp = rv_create_file("available_monitors", RV_MODE_READ, rv_root.root_dir, NULL,
> + tmp = rv_create_file("available_monitors", RV_MODE_READ, root_dir, NULL,
> &available_monitors_ops);
> if (!tmp)
> - goto out_err;
> + return 1;
>
> - tmp = rv_create_file("enabled_monitors", RV_MODE_WRITE, rv_root.root_dir, NULL,
> + tmp = rv_create_file("enabled_monitors", RV_MODE_WRITE, root_dir, NULL,
> &enabled_monitors_ops);
> if (!tmp)
> - goto out_err;
> + return 1;
>
> - tmp = rv_create_file("monitoring_on", RV_MODE_WRITE, rv_root.root_dir, NULL,
> + tmp = rv_create_file("monitoring_on", RV_MODE_WRITE, root_dir, NULL,
> &monitoring_on_fops);
> if (!tmp)
> - goto out_err;
> - retval = init_rv_reactors(rv_root.root_dir);
> + return 1;
> + retval = init_rv_reactors(root_dir);
> if (retval)
> - goto out_err;
> + return 1;
>
> turn_monitoring_on();
>
> - return 0;
> + rv_root.root_dir = root_dir;
> + retain_and_null_ptr(root_dir);
Same here.
>
> -out_err:
> - rv_remove(rv_root.root_dir);
> - printk(KERN_ERR "RV: Error while creating the RV interface\n");
> - return 1;
> + return 0;
> }
> @@ -438,30 +439,25 @@ static struct rv_reactor rv_nop = {
>
> int init_rv_reactors(struct dentry *root_dir)
> {
> - struct dentry *available, *reacting;
> int retval;
>
> - available = rv_create_file("available_reactors", RV_MODE_READ, root_dir, NULL,
> - &available_reactors_ops);
> - if (!available)
> - goto out_err;
> + struct dentry *available __free(rv_remove) =
> + rv_create_file("available_reactors", RV_MODE_READ, root_dir,
> + NULL, &available_reactors_ops);
>
> - reacting = rv_create_file("reacting_on", RV_MODE_WRITE, root_dir, NULL, &reacting_on_fops);
> - if (!reacting)
> - goto rm_available;
> + struct dentry *reacting =
> + rv_create_file("reacting_on", RV_MODE_WRITE, root_dir, NULL, &reacting_on_fops);
> +
> + if (!reacting || !available)
> + return -ENOMEM;
>
> retval = __rv_register_reactor(&rv_nop);
> if (retval)
> - goto rm_reacting;
> + return retval;
>
> turn_reacting_on();
>
> + retain_and_null_ptr(available);
> + retain_and_null_ptr(reacting);
Here it makes sense to use retain_and_null_ptr() as the two variables were
passed to another function. But the other two locations should simply use
the no_free_ptr() wrapper.
-- Steve
> return 0;
> -
> -rm_reacting:
> - rv_remove(reacting);
> -rm_available:
> - rv_remove(available);
> -out_err:
> - return -ENOMEM;
> }
^ permalink raw reply
* Re: [PATCH v3 3/8] mm: implement sticky VMA flags
From: Lorenzo Stoakes @ 2025-11-17 13:58 UTC (permalink / raw)
To: Andrew Morton
Cc: Jonathan Corbet, David Hildenbrand, Liam R . Howlett,
Vlastimil Babka, Mike Rapoport, Suren Baghdasaryan, Michal Hocko,
Steven Rostedt, Masami Hiramatsu, Mathieu Desnoyers, Jann Horn,
Pedro Falcato, Zi Yan, Baolin Wang, Nico Pache, Ryan Roberts,
Dev Jain, Barry Song, Lance Yang, linux-kernel, linux-fsdevel,
linux-doc, linux-mm, linux-trace-kernel, linux-kselftest,
Andrei Vagin
In-Reply-To: <1ee529ff912f71b3460d0d21bc5b32ca89d63513.1762531708.git.lorenzo.stoakes@oracle.com>
Hi Andrew,
We need to also update those callers that unconditionally set VMA flags via
vm_flags_reset() and vm_flags_reset_once() to account for sticky flags.
Please apply the attached fix-patch to fix this :)
Thanks, Lorenzo
----8<----
From 15c8661499f118ac8d422a8344c8017f24590ad0 Mon Sep 17 00:00:00 2001
From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Date: Mon, 17 Nov 2025 13:24:37 +0000
Subject: [PATCH] fixup
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
mm/madvise.c | 3 +++
mm/mlock.c | 3 +++
mm/mprotect.c | 3 +++
3 files changed, 9 insertions(+)
diff --git a/mm/madvise.c b/mm/madvise.c
index fb1c86e630b6..e651675eaaaf 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -172,6 +172,9 @@ static int madvise_update_vma(vm_flags_t new_flags,
if (IS_ERR(vma))
return PTR_ERR(vma);
+ /* Account for sticky flags. */
+ new_flags |= vma->vm_flags;
+
madv_behavior->vma = vma;
/* vm_flags is protected by the mmap_lock held in write mode. */
diff --git a/mm/mlock.c b/mm/mlock.c
index bb0776f5ef7c..7455a1f6a98c 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -484,6 +484,9 @@ static int mlock_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
goto out;
}
+ /* Account for sticky flags. */
+ newflags |= vma->vm_flags;
+
/*
* Keep track of amount of locked VM.
*/
diff --git a/mm/mprotect.c b/mm/mprotect.c
index ab4e06cd9a69..145417471201 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -762,6 +762,9 @@ mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb,
goto fail;
}
+ /* Account for sticky flags. */
+ newflags |= vma->vm_flags;
+
*pprev = vma;
/*
--
2.51.0
^ permalink raw reply related
* [RFC PATCH 8/8] selftests/bpf: Add race test for uprobe proglog optimization
From: Jiri Olsa @ 2025-11-17 12:40 UTC (permalink / raw)
To: Oleg Nesterov, Masami Hiramatsu, Peter Zijlstra, Andrii Nakryiko
Cc: bpf, linux-kernel, linux-trace-kernel, x86, Song Liu,
Yonghong Song, John Fastabend, Steven Rostedt, Ingo Molnar,
David Laight
In-Reply-To: <20251117124057.687384-1-jolsa@kernel.org>
Adding uprobe race test on top of prologue instructions.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
.../selftests/bpf/prog_tests/uprobe_syscall.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
index c6a58afc7ace..8793fbd61ffd 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
@@ -654,10 +654,11 @@ static USDT_DEFINE_SEMA(race);
static void *worker_trigger(void *arg)
{
+ trigger_t trigger = (trigger_t) arg;
unsigned long rounds = 0;
while (!race_stop) {
- uprobe_test();
+ trigger();
rounds++;
}
@@ -667,6 +668,7 @@ static void *worker_trigger(void *arg)
static void *worker_attach(void *arg)
{
+ trigger_t trigger = (trigger_t) arg;
LIBBPF_OPTS(bpf_uprobe_opts, opts);
struct uprobe_syscall_executed *skel;
unsigned long rounds = 0, offset;
@@ -677,7 +679,7 @@ static void *worker_attach(void *arg)
unsigned long *ref;
int err;
- offset = get_uprobe_offset(&uprobe_test);
+ offset = get_uprobe_offset(trigger);
if (!ASSERT_GE(offset, 0, "get_uprobe_offset"))
return NULL;
@@ -722,7 +724,7 @@ static useconds_t race_msec(void)
return 500;
}
-static void test_uprobe_race(void)
+static void test_uprobe_race(trigger_t trigger)
{
int err, i, nr_threads;
pthread_t *threads;
@@ -738,7 +740,7 @@ static void test_uprobe_race(void)
for (i = 0; i < nr_threads; i++) {
err = pthread_create(&threads[i], NULL, i % 2 ? worker_trigger : worker_attach,
- NULL);
+ trigger);
if (!ASSERT_OK(err, "pthread_create"))
goto cleanup;
}
@@ -870,8 +872,10 @@ static void __test_uprobe_syscall(void)
test_uprobe_session();
if (test__start_subtest("uprobe_usdt"))
test_uprobe_usdt();
- if (test__start_subtest("uprobe_race"))
- test_uprobe_race();
+ if (test__start_subtest("uprobe_race_nop5"))
+ test_uprobe_race(uprobe_test);
+ if (test__start_subtest("uprobe_race_prologue"))
+ test_uprobe_race(prologue_trigger);
if (test__start_subtest("uprobe_error"))
test_uprobe_error();
if (test__start_subtest("uprobe_regs_equal"))
--
2.51.1
^ permalink raw reply related
* [RFC PATCH 7/8] selftests/bpf: Add test for uprobe prologue optimization
From: Jiri Olsa @ 2025-11-17 12:40 UTC (permalink / raw)
To: Oleg Nesterov, Masami Hiramatsu, Peter Zijlstra, Andrii Nakryiko
Cc: bpf, linux-kernel, linux-trace-kernel, x86, Song Liu,
Yonghong Song, John Fastabend, Steven Rostedt, Ingo Molnar,
David Laight
In-Reply-To: <20251117124057.687384-1-jolsa@kernel.org>
Adding test that places uprobe on top of supported prologue
and checks that the uprobe gets properly optimized.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
.../selftests/bpf/prog_tests/uprobe_syscall.c | 63 ++++++++++++++++---
1 file changed, 53 insertions(+), 10 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
index 27fa6f309188..c6a58afc7ace 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
@@ -428,21 +428,21 @@ static void *check_attach(struct uprobe_syscall_executed *skel, trigger_t trigge
return tramp;
}
-static void check_detach(void *addr, void *tramp)
+static void check_detach(void *addr, void *tramp, unsigned char *orig)
{
/* [uprobes_trampoline] stays after detach */
ASSERT_OK(find_uprobes_trampoline(tramp), "uprobes_trampoline");
- ASSERT_OK(memcmp(addr, nop5, 5), "nop5");
+ ASSERT_OK(memcmp(addr, orig, 5), "orig");
}
static void check(struct uprobe_syscall_executed *skel, struct bpf_link *link,
- trigger_t trigger, void *addr, int executed)
+ trigger_t trigger, void *addr, int executed, unsigned char *orig)
{
void *tramp;
tramp = check_attach(skel, trigger, addr, executed);
bpf_link__destroy(link);
- check_detach(addr, tramp);
+ check_detach(addr, tramp, orig);
}
static void test_uprobe_legacy(void)
@@ -470,7 +470,7 @@ static void test_uprobe_legacy(void)
if (!ASSERT_OK_PTR(link, "bpf_program__attach_uprobe_opts"))
goto cleanup;
- check(skel, link, uprobe_test, uprobe_test, 2);
+ check(skel, link, uprobe_test, uprobe_test, 2, nop5);
/* uretprobe */
skel->bss->executed = 0;
@@ -480,7 +480,7 @@ static void test_uprobe_legacy(void)
if (!ASSERT_OK_PTR(link, "bpf_program__attach_uprobe_opts"))
goto cleanup;
- check(skel, link, uprobe_test, uprobe_test, 2);
+ check(skel, link, uprobe_test, uprobe_test, 2, nop5);
cleanup:
uprobe_syscall_executed__destroy(skel);
@@ -512,7 +512,7 @@ static void test_uprobe_multi(void)
if (!ASSERT_OK_PTR(link, "bpf_program__attach_uprobe_multi"))
goto cleanup;
- check(skel, link, uprobe_test, uprobe_test, 2);
+ check(skel, link, uprobe_test, uprobe_test, 2, nop5);
/* uretprobe.multi */
skel->bss->executed = 0;
@@ -522,7 +522,7 @@ static void test_uprobe_multi(void)
if (!ASSERT_OK_PTR(link, "bpf_program__attach_uprobe_multi"))
goto cleanup;
- check(skel, link, uprobe_test, uprobe_test, 2);
+ check(skel, link, uprobe_test, uprobe_test, 2, nop5);
cleanup:
uprobe_syscall_executed__destroy(skel);
@@ -555,7 +555,7 @@ static void test_uprobe_session(void)
if (!ASSERT_OK_PTR(link, "bpf_program__attach_uprobe_multi"))
goto cleanup;
- check(skel, link, uprobe_test, uprobe_test, 4);
+ check(skel, link, uprobe_test, uprobe_test, 4, nop5);
cleanup:
uprobe_syscall_executed__destroy(skel);
@@ -584,7 +584,7 @@ static void test_uprobe_usdt(void)
if (!ASSERT_OK_PTR(link, "bpf_program__attach_usdt"))
goto cleanup;
- check(skel, link, usdt_test, addr, 2);
+ check(skel, link, usdt_test, addr, 2, nop5);
cleanup:
uprobe_syscall_executed__destroy(skel);
@@ -813,6 +813,47 @@ static void test_emulate(void)
uprobe_syscall__destroy(skel);
}
+__attribute__((aligned(16)))
+__nocf_check __weak __naked void prologue_trigger(void)
+{
+ asm volatile (
+ "pushq %rbp\n"
+ "movq %rsp,%rbp\n"
+ "subq $0xb0,%rsp\n"
+ "addq $0xb0,%rsp\n"
+ "pop %rbp\n"
+ "ret\n"
+ );
+}
+
+static void test_optimize_prologue(void)
+{
+ struct uprobe_syscall_executed *skel = NULL;
+ struct bpf_link *link;
+ unsigned long offset;
+
+ offset = get_uprobe_offset(&prologue_trigger);
+ if (!ASSERT_GE(offset, 0, "get_uprobe_offset"))
+ goto cleanup;
+
+ /* uprobe */
+ skel = uprobe_syscall_executed__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "uprobe_syscall_executed__open_and_load"))
+ return;
+
+ skel->bss->pid = getpid();
+
+ link = bpf_program__attach_uprobe_opts(skel->progs.test_uprobe,
+ 0, "/proc/self/exe", offset, NULL);
+ if (!ASSERT_OK_PTR(link, "bpf_program__attach_uprobe_opts"))
+ goto cleanup;
+
+ check(skel, link, prologue_trigger, prologue_trigger, 2, (unsigned char *) prologue_trigger);
+
+cleanup:
+ uprobe_syscall_executed__destroy(skel);
+}
+
static void __test_uprobe_syscall(void)
{
if (test__start_subtest("uretprobe_regs_equal"))
@@ -839,6 +880,8 @@ static void __test_uprobe_syscall(void)
test_regs_change();
if (test__start_subtest("emulate_mov"))
test_emulate();
+ if (test__start_subtest("optimize_prologue"))
+ test_optimize_prologue();
}
#else
static void __test_uprobe_syscall(void)
--
2.51.1
^ permalink raw reply related
* [RFC PATCH 6/8] selftests/bpf: Add test for mov and sub emulation
From: Jiri Olsa @ 2025-11-17 12:40 UTC (permalink / raw)
To: Oleg Nesterov, Masami Hiramatsu, Peter Zijlstra, Andrii Nakryiko
Cc: bpf, linux-kernel, linux-trace-kernel, x86, Song Liu,
Yonghong Song, John Fastabend, Steven Rostedt, Ingo Molnar,
David Laight
In-Reply-To: <20251117124057.687384-1-jolsa@kernel.org>
Adding test code for mov and sub instructions emulation.
TODO add test for sub flags value emulation.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
.../selftests/bpf/prog_tests/uprobe_syscall.c | 50 +++++++++++++++++++
1 file changed, 50 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
index 955a37751b52..27fa6f309188 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
@@ -765,6 +765,54 @@ static void test_uprobe_error(void)
ASSERT_EQ(errno, ENXIO, "errno");
}
+__attribute__((aligned(16)))
+__nocf_check __weak __naked unsigned long emulate_mov_trigger(void)
+{
+ asm volatile (
+ "pushq %rbp\n"
+ "movq %rsp,%rbp\n"
+ "subq $0xb0,%rsp\n"
+ "addq $0xb0,%rsp\n"
+ "pop %rbp\n"
+ "ret\n"
+ );
+}
+
+static void test_emulate(void)
+{
+ struct uprobe_syscall *skel = NULL;
+ unsigned long offset;
+
+ offset = get_uprobe_offset(&emulate_mov_trigger);
+ if (!ASSERT_GE(offset, 0, "get_uprobe_offset"))
+ return;
+
+ skel = uprobe_syscall__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "uprobe_syscall__open_and_load"))
+ goto cleanup;
+
+ /* mov */
+ skel->links.probe = bpf_program__attach_uprobe_opts(skel->progs.probe,
+ 0, "/proc/self/exe", offset + 1, NULL);
+ if (!ASSERT_OK_PTR(skel->links.probe, "bpf_program__attach_uprobe_opts"))
+ goto cleanup;
+
+ emulate_mov_trigger();
+
+ bpf_link__destroy(skel->links.probe);
+
+ /* sub */
+ skel->links.probe = bpf_program__attach_uprobe_opts(skel->progs.probe,
+ 0, "/proc/self/exe", offset + 4, NULL);
+ if (!ASSERT_OK_PTR(skel->links.probe, "bpf_program__attach_uprobe_opts"))
+ goto cleanup;
+
+ emulate_mov_trigger();
+
+cleanup:
+ uprobe_syscall__destroy(skel);
+}
+
static void __test_uprobe_syscall(void)
{
if (test__start_subtest("uretprobe_regs_equal"))
@@ -789,6 +837,8 @@ static void __test_uprobe_syscall(void)
test_uprobe_regs_equal(false);
if (test__start_subtest("regs_change"))
test_regs_change();
+ if (test__start_subtest("emulate_mov"))
+ test_emulate();
}
#else
static void __test_uprobe_syscall(void)
--
2.51.1
^ permalink raw reply related
* [RFC PATCH 5/8] uprobe/x86: Add support to optimize on top of emulated instructions
From: Jiri Olsa @ 2025-11-17 12:40 UTC (permalink / raw)
To: Oleg Nesterov, Masami Hiramatsu, Peter Zijlstra, Andrii Nakryiko
Cc: bpf, linux-kernel, linux-trace-kernel, x86, Song Liu,
Yonghong Song, John Fastabend, Steven Rostedt, Ingo Molnar,
David Laight
In-Reply-To: <20251117124057.687384-1-jolsa@kernel.org>
Adding support to optimize uprobe on top of instructions that can
be emulated.
The idea is to store instructions on underlying 5 bytes and emulate
them during the int3 and uprobe syscall execution:
- install 'call trampoline' through standard int3 update
- if int3 is hit before we finish optimizing we emulate
all underlying instructions
- when call is installed the uprobe syscall will emulate
all underlying instructions
Adding opt_xol_ops that emulate instructions that are replaced
by 5 bytes call instruction used to optimize the uprobe.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/include/asm/uprobes.h | 13 ++--
arch/x86/kernel/uprobes.c | 106 ++++++++++++++++++++++++++++++++-
include/linux/uprobes.h | 1 +
kernel/events/uprobes.c | 6 ++
4 files changed, 120 insertions(+), 6 deletions(-)
diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index e09aab82b8c1..eaa80dc1c836 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -21,8 +21,9 @@ typedef u8 uprobe_opcode_t;
#define UPROBE_SWBP_INSN_SIZE 1
enum {
- ARCH_UPROBE_FLAG_CAN_OPTIMIZE = 0,
- ARCH_UPROBE_FLAG_OPTIMIZE_FAIL = 1,
+ ARCH_UPROBE_FLAG_CAN_OPTIMIZE = 0,
+ ARCH_UPROBE_FLAG_OPTIMIZE_FAIL = 1,
+ ARCH_UPROBE_FLAG_OPTIMIZE_EMULATE = 2,
};
struct uprobe_xol_ops;
@@ -59,11 +60,15 @@ struct arch_uprobe_xol {
struct arch_uprobe {
union {
- u8 insn[MAX_UINSN_BYTES];
+ u8 insn[5*MAX_UINSN_BYTES];
u8 ixol[MAX_UINSN_BYTES];
};
- struct arch_uprobe_xol xol;
+ struct arch_uprobe_xol xol;
+ struct {
+ struct arch_uprobe_xol xol[5];
+ int cnt;
+ } opt;
unsigned long flags;
};
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 904c423ea81d..7f3f537a6425 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -277,13 +277,14 @@ static bool is_prefix_bad(struct insn *insn)
return false;
}
-static int uprobe_init_insn(struct arch_uprobe *auprobe, struct insn *insn, bool x86_64)
+static int uprobe_init_insn_offset(struct arch_uprobe *auprobe, unsigned long offset,
+ struct insn *insn, bool x86_64)
{
enum insn_mode m = x86_64 ? INSN_MODE_64 : INSN_MODE_32;
u32 volatile *good_insns;
int ret;
- ret = insn_decode(insn, auprobe->insn, sizeof(auprobe->insn), m);
+ ret = insn_decode(insn, auprobe->insn + offset, sizeof(auprobe->insn) - offset, m);
if (ret < 0)
return -ENOEXEC;
@@ -310,6 +311,11 @@ static int uprobe_init_insn(struct arch_uprobe *auprobe, struct insn *insn, bool
return -ENOTSUPP;
}
+static int uprobe_init_insn(struct arch_uprobe *auprobe, struct insn *insn, bool x86_64)
+{
+ return uprobe_init_insn_offset(auprobe, 0, insn, x86_64);
+}
+
#ifdef CONFIG_X86_64
struct uretprobe_syscall_args {
@@ -1462,6 +1468,23 @@ static bool sub_emulate_op(struct arch_uprobe *auprobe, struct arch_uprobe_xol *
#undef EFLAGS_MASK
+static bool optimized_emulate(struct arch_uprobe *auprobe, struct arch_uprobe_xol *xol,
+ struct pt_regs *regs)
+{
+ int i;
+
+ for (i = 0; i < auprobe->opt.cnt; i++) {
+ WARN_ON(!auprobe->opt.xol[i].ops->emulate(auprobe, &auprobe->opt.xol[i], regs));
+ }
+ return true;
+}
+
+void arch_uprobe_optimized_emulate(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+ if (test_bit(ARCH_UPROBE_FLAG_OPTIMIZE_EMULATE, &auprobe->flags))
+ optimized_emulate(auprobe, NULL, regs);
+}
+
static const struct uprobe_xol_ops branch_xol_ops = {
.emulate = branch_emulate_op,
.post_xol = branch_post_xol_op,
@@ -1479,6 +1502,10 @@ static const struct uprobe_xol_ops sub_xol_ops = {
.emulate = sub_emulate_op,
};
+static const struct uprobe_xol_ops opt_xol_ops = {
+ .emulate = optimized_emulate,
+};
+
/* Returns -ENOSYS if branch_xol_ops doesn't handle this insn */
static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
{
@@ -1675,6 +1702,73 @@ static int sub_setup_xol_ops(struct arch_uprobe_xol *xol, struct insn *insn)
xol->ops = &sub_xol_ops;
return 0;
}
+
+static int opt_setup_xol_insns(struct arch_uprobe *auprobe, struct arch_uprobe_xol *xol,
+ struct insn *insn)
+{
+ int ret;
+
+ /*
+ * TODO somehow separate nop emulation out of branch_xol_ops,
+ * so we could emulate nop instructions in here.
+ */
+ ret = push_setup_xol_ops(xol, insn);
+ if (ret != -ENOSYS)
+ return ret;
+ ret = mov_setup_xol_ops(xol, insn);
+ if (ret != -ENOSYS)
+ return ret;
+ ret = sub_setup_xol_ops(xol, insn);
+ if (ret != -ENOSYS)
+ return ret;
+
+ return -1;
+}
+
+static int opt_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
+{
+ unsigned long offset = insn->length;
+ struct insn insnX;
+ int i, ret;
+
+ if (test_bit(ARCH_UPROBE_FLAG_CAN_OPTIMIZE, &auprobe->flags))
+ return -ENOSYS;
+
+ ret = opt_setup_xol_insns(auprobe, &auprobe->opt.xol[0], insn);
+ if (ret)
+ return -ENOSYS;
+
+ auprobe->opt.cnt = 1;
+ if (offset >= 5)
+ goto optimize;
+
+ for (i = 1; i < 5; i++) {
+ ret = uprobe_init_insn_offset(auprobe, offset, &insnX, true);
+ if (ret)
+ break;
+ ret = opt_setup_xol_insns(auprobe, &auprobe->opt.xol[i], &insnX);
+ if (ret)
+ break;
+ offset += insnX.length;
+ auprobe->opt.cnt++;
+ if (offset >= 5)
+ goto optimize;
+ }
+
+ return -ENOSYS;
+
+optimize:
+ set_bit(ARCH_UPROBE_FLAG_CAN_OPTIMIZE, &auprobe->flags);
+ set_bit(ARCH_UPROBE_FLAG_OPTIMIZE_EMULATE, &auprobe->flags);
+ auprobe->xol.ops = &opt_xol_ops;
+
+ /*
+ * TODO perhaps we could 'emulate' nop, so there would be no need for
+ * ARCH_UPROBE_FLAG_OPTIMIZE_EMULATE flag, because we would emulate
+ * allways.
+ */
+ return 0;
+}
#else
static int mov_setup_xol_ops(struct arch_uprobe_xol *xol, struct insn *insn)
{
@@ -1684,6 +1778,10 @@ static int sub_setup_xol_ops(struct arch_uprobe_xol *xol, struct insn *insn)
{
return -ENOSYS;
}
+static int opt_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
+{
+ return -ENOSYS;
+}
#endif
/**
@@ -1706,6 +1804,10 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
if (can_optimize(&insn, addr))
set_bit(ARCH_UPROBE_FLAG_CAN_OPTIMIZE, &auprobe->flags);
+ ret = opt_setup_xol_ops(auprobe, &insn);
+ if (ret != -ENOSYS)
+ return ret;
+
ret = branch_setup_xol_ops(auprobe, &insn);
if (ret != -ENOSYS)
return ret;
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index ee3d36eda45d..4b9f81ad8316 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -242,6 +242,7 @@ extern void arch_uprobe_clear_state(struct mm_struct *mm);
extern void arch_uprobe_init_state(struct mm_struct *mm);
extern void handle_syscall_uprobe(struct pt_regs *regs, unsigned long bp_vaddr);
extern void arch_uprobe_optimize(struct arch_uprobe *auprobe, unsigned long vaddr);
+extern void arch_uprobe_optimized_emulate(struct arch_uprobe *auprobe, struct pt_regs *regs);
#else /* !CONFIG_UPROBES */
struct uprobes_state {
};
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index f11ceb8be8c4..dd893030e32e 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -2701,6 +2701,10 @@ void __weak arch_uprobe_optimize(struct arch_uprobe *auprobe, unsigned long vadd
{
}
+void __weak arch_uprobe_optimized_emulate(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+}
+
/*
* Run handler and ask thread to singlestep.
* Ensure all non-fatal signals cannot interrupt thread while it singlesteps.
@@ -2801,6 +2805,8 @@ void handle_syscall_uprobe(struct pt_regs *regs, unsigned long bp_vaddr)
if (arch_uprobe_ignore(&uprobe->arch, regs))
return;
handler_chain(uprobe, regs);
+
+ arch_uprobe_optimized_emulate(&uprobe->arch, regs);
}
/*
--
2.51.1
^ permalink raw reply related
* [RFC PATCH 4/8] uprobe/x86: Add support to emulate sub imm,reg instructions
From: Jiri Olsa @ 2025-11-17 12:40 UTC (permalink / raw)
To: Oleg Nesterov, Masami Hiramatsu, Peter Zijlstra, Andrii Nakryiko
Cc: bpf, linux-kernel, linux-trace-kernel, x86, Song Liu,
Yonghong Song, John Fastabend, Steven Rostedt, Ingo Molnar,
David Laight
In-Reply-To: <20251117124057.687384-1-jolsa@kernel.org>
Adding support to emulate sub reg, imm instructions, because it's
often part of the function prologue.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/include/asm/uprobes.h | 5 +++
arch/x86/kernel/uprobes.c | 73 ++++++++++++++++++++++++++++++++++
2 files changed, 78 insertions(+)
diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index e6fd87a1cbc3..e09aab82b8c1 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -49,6 +49,11 @@ struct arch_uprobe_xol {
u16 dst; /* to the start of pt_regs */
u8 ilen;
} mov;
+ struct {
+ s32 val;
+ u16 reg; /* to the start of pt_regs */
+ u8 ilen;
+ } sub;
};
};
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 5c44c4b84e99..904c423ea81d 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -1428,6 +1428,40 @@ static bool mov_emulate_op(struct arch_uprobe *auprobe, struct arch_uprobe_xol *
return true;
}
+#define EFLAGS_MASK (X86_EFLAGS_OF|X86_EFLAGS_SF|X86_EFLAGS_ZF|X86_EFLAGS_AF|\
+ X86_EFLAGS_PF|X86_EFLAGS_CF)
+
+static bool sub_emulate_op(struct arch_uprobe *auprobe, struct arch_uprobe_xol *xol,
+ struct pt_regs *regs)
+{
+ unsigned long dst, flags = regs->flags, val = xol->sub.val;
+ unsigned long *reg = (void *) regs + xol->sub.reg;
+
+ dst = *reg;
+
+ /*
+ * Emulate sub with 'sub reg,reg' and get result value and
+ * flags register change. Not sure it's completely equivalent
+ * to sub reg,imm so perhaps there's better way.
+ */
+ asm volatile (
+ "pushf \n\t"
+ "push %[flags]; popf \n\t"
+ "subq %[src], %[dst] \n\t"
+ "pushf; popq %[flags] \n\t"
+ "popf \n\t"
+ : [flags] "+D" (flags), [dst] "+r" (dst)
+ : [src] "r" (val)
+ );
+
+ *reg = dst;
+ regs->flags = (regs->flags & ~EFLAGS_MASK) | (flags & EFLAGS_MASK);
+ regs->ip += xol->sub.ilen;
+ return true;
+}
+
+#undef EFLAGS_MASK
+
static const struct uprobe_xol_ops branch_xol_ops = {
.emulate = branch_emulate_op,
.post_xol = branch_post_xol_op,
@@ -1441,6 +1475,10 @@ static const struct uprobe_xol_ops mov_xol_ops = {
.emulate = mov_emulate_op,
};
+static const struct uprobe_xol_ops sub_xol_ops = {
+ .emulate = sub_emulate_op,
+};
+
/* Returns -ENOSYS if branch_xol_ops doesn't handle this insn */
static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
{
@@ -1610,11 +1648,42 @@ static int mov_setup_xol_ops(struct arch_uprobe_xol *xol, struct insn *insn)
xol->ops = &mov_xol_ops;
return 0;
}
+
+static int sub_setup_xol_ops(struct arch_uprobe_xol *xol, struct insn *insn)
+{
+ u8 opc1 = OPCODE1(insn);
+ int off;
+
+ if (opc1 != 0x81)
+ return -ENOSYS;
+ if (insn->rex_prefix.nbytes != 1 ||
+ insn->rex_prefix.bytes[0] != 0x48)
+ return -ENOSYS;
+ if (X86_MODRM_MOD(insn->modrm.value) != 3)
+ return -ENOSYS;
+ if (X86_MODRM_REG(insn->modrm.value) != 5)
+ return -ENOSYS;
+
+ /* get register offset */
+ off = insn_get_modrm_rm_off(insn);
+ if (off < 0)
+ return off;
+
+ xol->sub.reg = off;
+ xol->sub.val = insn->immediate.value;
+ xol->sub.ilen = insn->length;
+ xol->ops = &sub_xol_ops;
+ return 0;
+}
#else
static int mov_setup_xol_ops(struct arch_uprobe_xol *xol, struct insn *insn)
{
return -ENOSYS;
}
+static int sub_setup_xol_ops(struct arch_uprobe_xol *xol, struct insn *insn)
+{
+ return -ENOSYS;
+}
#endif
/**
@@ -1649,6 +1718,10 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
if (ret != -ENOSYS)
return ret;
+ ret = sub_setup_xol_ops(&auprobe->xol, &insn);
+ if (ret != -ENOSYS)
+ return ret;
+
/*
* Figure out which fixups default_post_xol_op() will need to perform,
* and annotate defparam->fixups accordingly.
--
2.51.1
^ permalink raw reply related
* [RFC PATCH 3/8] uprobe/x86: Add support to emulate mov reg,reg instructions
From: Jiri Olsa @ 2025-11-17 12:40 UTC (permalink / raw)
To: Oleg Nesterov, Masami Hiramatsu, Peter Zijlstra, Andrii Nakryiko
Cc: bpf, linux-kernel, linux-trace-kernel, x86, Song Liu,
Yonghong Song, John Fastabend, Steven Rostedt, Ingo Molnar,
David Laight
In-Reply-To: <20251117124057.687384-1-jolsa@kernel.org>
Adding support to emulate mov reg to reg instructions, because it's
often part of the function prologue.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/include/asm/uprobes.h | 5 +++
arch/x86/kernel/uprobes.c | 61 ++++++++++++++++++++++++++++++++++
2 files changed, 66 insertions(+)
diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index 819e35aa61c4..e6fd87a1cbc3 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -44,6 +44,11 @@ struct arch_uprobe_xol {
u8 reg_offset; /* to the start of pt_regs */
u8 ilen;
} push;
+ struct {
+ u16 src; /* to the start of pt_regs */
+ u16 dst; /* to the start of pt_regs */
+ u8 ilen;
+ } mov;
};
};
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 7d7a5e677472..5c44c4b84e99 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -19,6 +19,7 @@
#include <asm/insn.h>
#include <asm/mmu_context.h>
#include <asm/nops.h>
+#include <asm/insn-eval.h>
/* Post-execution fixups. */
@@ -1414,6 +1415,19 @@ static void branch_clear_offset(struct arch_uprobe *auprobe, struct insn *insn)
0, insn->immediate.nbytes);
}
+static bool mov_emulate_op(struct arch_uprobe *auprobe, struct arch_uprobe_xol *xol,
+ struct pt_regs *regs)
+{
+ unsigned long *dst, *src;
+
+ dst = (void *) regs + xol->mov.dst;
+ src = (void *) regs + xol->mov.src;
+ *dst = *src;
+
+ regs->ip += xol->mov.ilen;
+ return true;
+}
+
static const struct uprobe_xol_ops branch_xol_ops = {
.emulate = branch_emulate_op,
.post_xol = branch_post_xol_op,
@@ -1423,6 +1437,10 @@ static const struct uprobe_xol_ops push_xol_ops = {
.emulate = push_emulate_op,
};
+static const struct uprobe_xol_ops mov_xol_ops = {
+ .emulate = mov_emulate_op,
+};
+
/* Returns -ENOSYS if branch_xol_ops doesn't handle this insn */
static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
{
@@ -1560,6 +1578,45 @@ static int push_setup_xol_ops(struct arch_uprobe_xol *xol, struct insn *insn)
return 0;
}
+#ifdef CONFIG_X86_64
+/* Returns -ENOSYS if mov_xol_ops doesn't handle this insn */
+static int mov_setup_xol_ops(struct arch_uprobe_xol *xol, struct insn *insn)
+{
+ u8 opc1 = OPCODE1(insn);
+ int off_src, off_dst;
+
+ /* validate opcode */
+ if (opc1 != 0x89)
+ return -ENOSYS;
+ if (insn->rex_prefix.nbytes != 1 ||
+ insn->rex_prefix.bytes[0] != 0x48)
+ return -ENOSYS;
+
+ /* only register operands */
+ if (X86_MODRM_MOD(insn->modrm.value) != 3)
+ return -ENOSYS;
+
+ /* get registers offset */
+ off_src = insn_get_modrm_reg_off(insn);
+ if (off_src < 0)
+ return off_src;
+ off_dst = insn_get_modrm_rm_off(insn);
+ if (off_dst < 0)
+ return off_dst;
+
+ xol->mov.src = off_src;
+ xol->mov.dst = off_dst;
+ xol->mov.ilen = insn->length;
+ xol->ops = &mov_xol_ops;
+ return 0;
+}
+#else
+static int mov_setup_xol_ops(struct arch_uprobe_xol *xol, struct insn *insn)
+{
+ return -ENOSYS;
+}
+#endif
+
/**
* arch_uprobe_analyze_insn - instruction analysis including validity and fixups.
* @auprobe: the probepoint information.
@@ -1588,6 +1645,10 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
if (ret != -ENOSYS)
return ret;
+ ret = mov_setup_xol_ops(&auprobe->xol, &insn);
+ if (ret != -ENOSYS)
+ return ret;
+
/*
* Figure out which fixups default_post_xol_op() will need to perform,
* and annotate defparam->fixups accordingly.
--
2.51.1
^ permalink raw reply related
* [RFC PATCH 2/8] uprobe/x86: Use struct arch_uprobe_xol in emulate callback
From: Jiri Olsa @ 2025-11-17 12:40 UTC (permalink / raw)
To: Oleg Nesterov, Masami Hiramatsu, Peter Zijlstra, Andrii Nakryiko
Cc: bpf, linux-kernel, linux-trace-kernel, x86, Song Liu,
Yonghong Song, John Fastabend, Steven Rostedt, Ingo Molnar,
David Laight
In-Reply-To: <20251117124057.687384-1-jolsa@kernel.org>
Using struct arch_uprobe_xol also in emulate callback
which will help in following changes.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/kernel/uprobes.c | 32 +++++++++++++++++---------------
1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index fb9457b29dbc..7d7a5e677472 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -1212,7 +1212,7 @@ static bool can_optimize(struct insn *insn, unsigned long vaddr)
#endif /* CONFIG_X86_64 */
struct uprobe_xol_ops {
- bool (*emulate)(struct arch_uprobe *, struct pt_regs *);
+ bool (*emulate)(struct arch_uprobe*, struct arch_uprobe_xol *, struct pt_regs *);
int (*pre_xol)(struct arch_uprobe *, struct pt_regs *);
int (*post_xol)(struct arch_uprobe *, struct pt_regs *);
void (*abort)(struct arch_uprobe *, struct pt_regs *);
@@ -1291,9 +1291,9 @@ static const struct uprobe_xol_ops default_xol_ops = {
.abort = default_abort_op,
};
-static bool branch_is_call(struct arch_uprobe *auprobe)
+static bool branch_is_call(struct arch_uprobe_xol *xol)
{
- return auprobe->xol.branch.opc1 == 0xe8;
+ return xol->branch.opc1 == 0xe8;
}
#define CASE_COND \
@@ -1325,11 +1325,11 @@ static bool is_cond_jmp_opcode(u8 opcode)
}
}
-static bool check_jmp_cond(struct arch_uprobe *auprobe, struct pt_regs *regs)
+static bool check_jmp_cond(struct arch_uprobe_xol *xol, struct pt_regs *regs)
{
unsigned long flags = regs->flags;
- switch (auprobe->xol.branch.opc1) {
+ switch (xol->branch.opc1) {
#define DO(expr) \
return expr;
CASE_COND
@@ -1344,12 +1344,13 @@ static bool check_jmp_cond(struct arch_uprobe *auprobe, struct pt_regs *regs)
#undef COND
#undef CASE_COND
-static bool branch_emulate_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
+static bool branch_emulate_op(struct arch_uprobe *auprobe, struct arch_uprobe_xol *xol,
+ struct pt_regs *regs)
{
- unsigned long new_ip = regs->ip += auprobe->xol.branch.ilen;
- unsigned long offs = (long)auprobe->xol.branch.offs;
+ unsigned long new_ip = regs->ip += xol->branch.ilen;
+ unsigned long offs = (long)xol->branch.offs;
- if (branch_is_call(auprobe)) {
+ if (branch_is_call(xol)) {
/*
* If it fails we execute this (mangled, see the comment in
* branch_clear_offset) insn out-of-line. In the likely case
@@ -1361,7 +1362,7 @@ static bool branch_emulate_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
*/
if (emulate_push_stack(regs, new_ip))
return false;
- } else if (!check_jmp_cond(auprobe, regs)) {
+ } else if (!check_jmp_cond(xol, regs)) {
offs = 0;
}
@@ -1369,19 +1370,20 @@ static bool branch_emulate_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
return true;
}
-static bool push_emulate_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
+static bool push_emulate_op(struct arch_uprobe *auprobe, struct arch_uprobe_xol *xol,
+ struct pt_regs *regs)
{
- unsigned long *src_ptr = (void *)regs + auprobe->xol.push.reg_offset;
+ unsigned long *src_ptr = (void *)regs + xol->push.reg_offset;
if (emulate_push_stack(regs, *src_ptr))
return false;
- regs->ip += auprobe->xol.push.ilen;
+ regs->ip += xol->push.ilen;
return true;
}
static int branch_post_xol_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
{
- BUG_ON(!branch_is_call(auprobe));
+ BUG_ON(!branch_is_call(&auprobe->xol));
/*
* We can only get here if branch_emulate_op() failed to push the ret
* address _and_ another thread expanded our stack before the (mangled)
@@ -1767,7 +1769,7 @@ void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
static bool __skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
{
if (auprobe->xol.ops->emulate)
- return auprobe->xol.ops->emulate(auprobe, regs);
+ return auprobe->xol.ops->emulate(auprobe, &auprobe->xol, regs);
return false;
}
--
2.51.1
^ permalink raw reply related
* [RFC PATCH 1/8] uprobe/x86: Introduce struct arch_uprobe_xol object
From: Jiri Olsa @ 2025-11-17 12:40 UTC (permalink / raw)
To: Oleg Nesterov, Masami Hiramatsu, Peter Zijlstra, Andrii Nakryiko
Cc: bpf, linux-kernel, linux-trace-kernel, x86, Song Liu,
Yonghong Song, John Fastabend, Steven Rostedt, Ingo Molnar,
David Laight
In-Reply-To: <20251117124057.687384-1-jolsa@kernel.org>
Mov xol data into separate arch_uprobe_xol object so we can
hold more of them in following changes.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/include/asm/uprobes.h | 16 ++++---
arch/x86/kernel/uprobes.c | 78 +++++++++++++++++-----------------
2 files changed, 49 insertions(+), 45 deletions(-)
diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index 1ee2e5115955..819e35aa61c4 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -27,12 +27,7 @@ enum {
struct uprobe_xol_ops;
-struct arch_uprobe {
- union {
- u8 insn[MAX_UINSN_BYTES];
- u8 ixol[MAX_UINSN_BYTES];
- };
-
+struct arch_uprobe_xol {
const struct uprobe_xol_ops *ops;
union {
@@ -50,6 +45,15 @@ struct arch_uprobe {
u8 ilen;
} push;
};
+};
+
+struct arch_uprobe {
+ union {
+ u8 insn[MAX_UINSN_BYTES];
+ u8 ixol[MAX_UINSN_BYTES];
+ };
+
+ struct arch_uprobe_xol xol;
unsigned long flags;
};
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 845aeaf36b8d..fb9457b29dbc 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -560,14 +560,14 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn)
*/
if (reg != 6 && reg2 != 6) {
reg2 = 6;
- auprobe->defparam.fixups |= UPROBE_FIX_RIP_SI;
+ auprobe->xol.defparam.fixups |= UPROBE_FIX_RIP_SI;
} else if (reg != 7 && reg2 != 7) {
reg2 = 7;
- auprobe->defparam.fixups |= UPROBE_FIX_RIP_DI;
+ auprobe->xol.defparam.fixups |= UPROBE_FIX_RIP_DI;
/* TODO (paranoia): force maskmovq to not use di */
} else {
reg2 = 3;
- auprobe->defparam.fixups |= UPROBE_FIX_RIP_BX;
+ auprobe->xol.defparam.fixups |= UPROBE_FIX_RIP_BX;
}
/*
* Point cursor at the modrm byte. The next 4 bytes are the
@@ -586,9 +586,9 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn)
static inline unsigned long *
scratch_reg(struct arch_uprobe *auprobe, struct pt_regs *regs)
{
- if (auprobe->defparam.fixups & UPROBE_FIX_RIP_SI)
+ if (auprobe->xol.defparam.fixups & UPROBE_FIX_RIP_SI)
return ®s->si;
- if (auprobe->defparam.fixups & UPROBE_FIX_RIP_DI)
+ if (auprobe->xol.defparam.fixups & UPROBE_FIX_RIP_DI)
return ®s->di;
return ®s->bx;
}
@@ -599,18 +599,18 @@ scratch_reg(struct arch_uprobe *auprobe, struct pt_regs *regs)
*/
static void riprel_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
{
- if (auprobe->defparam.fixups & UPROBE_FIX_RIP_MASK) {
+ if (auprobe->xol.defparam.fixups & UPROBE_FIX_RIP_MASK) {
struct uprobe_task *utask = current->utask;
unsigned long *sr = scratch_reg(auprobe, regs);
utask->autask.saved_scratch_register = *sr;
- *sr = utask->vaddr + auprobe->defparam.ilen;
+ *sr = utask->vaddr + auprobe->xol.defparam.ilen;
}
}
static void riprel_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
{
- if (auprobe->defparam.fixups & UPROBE_FIX_RIP_MASK) {
+ if (auprobe->xol.defparam.fixups & UPROBE_FIX_RIP_MASK) {
struct uprobe_task *utask = current->utask;
unsigned long *sr = scratch_reg(auprobe, regs);
@@ -1265,16 +1265,16 @@ static int default_post_xol_op(struct arch_uprobe *auprobe, struct pt_regs *regs
struct uprobe_task *utask = current->utask;
riprel_post_xol(auprobe, regs);
- if (auprobe->defparam.fixups & UPROBE_FIX_IP) {
+ if (auprobe->xol.defparam.fixups & UPROBE_FIX_IP) {
long correction = utask->vaddr - utask->xol_vaddr;
regs->ip += correction;
- } else if (auprobe->defparam.fixups & UPROBE_FIX_CALL) {
+ } else if (auprobe->xol.defparam.fixups & UPROBE_FIX_CALL) {
regs->sp += sizeof_long(regs); /* Pop incorrect return address */
- if (emulate_push_stack(regs, utask->vaddr + auprobe->defparam.ilen))
+ if (emulate_push_stack(regs, utask->vaddr + auprobe->xol.defparam.ilen))
return -ERESTART;
}
/* popf; tell the caller to not touch TF */
- if (auprobe->defparam.fixups & UPROBE_FIX_SETF)
+ if (auprobe->xol.defparam.fixups & UPROBE_FIX_SETF)
utask->autask.saved_tf = true;
return 0;
@@ -1293,7 +1293,7 @@ static const struct uprobe_xol_ops default_xol_ops = {
static bool branch_is_call(struct arch_uprobe *auprobe)
{
- return auprobe->branch.opc1 == 0xe8;
+ return auprobe->xol.branch.opc1 == 0xe8;
}
#define CASE_COND \
@@ -1329,7 +1329,7 @@ static bool check_jmp_cond(struct arch_uprobe *auprobe, struct pt_regs *regs)
{
unsigned long flags = regs->flags;
- switch (auprobe->branch.opc1) {
+ switch (auprobe->xol.branch.opc1) {
#define DO(expr) \
return expr;
CASE_COND
@@ -1346,8 +1346,8 @@ static bool check_jmp_cond(struct arch_uprobe *auprobe, struct pt_regs *regs)
static bool branch_emulate_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
{
- unsigned long new_ip = regs->ip += auprobe->branch.ilen;
- unsigned long offs = (long)auprobe->branch.offs;
+ unsigned long new_ip = regs->ip += auprobe->xol.branch.ilen;
+ unsigned long offs = (long)auprobe->xol.branch.offs;
if (branch_is_call(auprobe)) {
/*
@@ -1371,11 +1371,11 @@ static bool branch_emulate_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
static bool push_emulate_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
{
- unsigned long *src_ptr = (void *)regs + auprobe->push.reg_offset;
+ unsigned long *src_ptr = (void *)regs + auprobe->xol.push.reg_offset;
if (emulate_push_stack(regs, *src_ptr))
return false;
- regs->ip += auprobe->push.ilen;
+ regs->ip += auprobe->xol.push.ilen;
return true;
}
@@ -1469,16 +1469,16 @@ static int branch_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
}
setup:
- auprobe->branch.opc1 = opc1;
- auprobe->branch.ilen = insn->length;
- auprobe->branch.offs = insn->immediate.value;
+ auprobe->xol.branch.opc1 = opc1;
+ auprobe->xol.branch.ilen = insn->length;
+ auprobe->xol.branch.offs = insn->immediate.value;
- auprobe->ops = &branch_xol_ops;
+ auprobe->xol.ops = &branch_xol_ops;
return 0;
}
/* Returns -ENOSYS if push_xol_ops doesn't handle this insn */
-static int push_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
+static int push_setup_xol_ops(struct arch_uprobe_xol *xol, struct insn *insn)
{
u8 opc1 = OPCODE1(insn), reg_offset = 0;
@@ -1552,9 +1552,9 @@ static int push_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
}
}
- auprobe->push.reg_offset = reg_offset;
- auprobe->push.ilen = insn->length;
- auprobe->ops = &push_xol_ops;
+ xol->push.reg_offset = reg_offset;
+ xol->push.ilen = insn->length;
+ xol->ops = &push_xol_ops;
return 0;
}
@@ -1582,7 +1582,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
if (ret != -ENOSYS)
return ret;
- ret = push_setup_xol_ops(auprobe, &insn);
+ ret = push_setup_xol_ops(&auprobe->xol, &insn);
if (ret != -ENOSYS)
return ret;
@@ -1592,7 +1592,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
*/
switch (OPCODE1(&insn)) {
case 0x9d: /* popf */
- auprobe->defparam.fixups |= UPROBE_FIX_SETF;
+ auprobe->xol.defparam.fixups |= UPROBE_FIX_SETF;
break;
case 0xc3: /* ret or lret -- ip is correct */
case 0xcb:
@@ -1618,10 +1618,10 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
riprel_analyze(auprobe, &insn);
}
- auprobe->defparam.ilen = insn.length;
- auprobe->defparam.fixups |= fix_ip_or_call;
+ auprobe->xol.defparam.ilen = insn.length;
+ auprobe->xol.defparam.fixups |= fix_ip_or_call;
- auprobe->ops = &default_xol_ops;
+ auprobe->xol.ops = &default_xol_ops;
return 0;
}
@@ -1634,8 +1634,8 @@ int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
{
struct uprobe_task *utask = current->utask;
- if (auprobe->ops->pre_xol) {
- int err = auprobe->ops->pre_xol(auprobe, regs);
+ if (auprobe->xol.ops->pre_xol) {
+ int err = auprobe->xol.ops->pre_xol(auprobe, regs);
if (err)
return err;
}
@@ -1686,8 +1686,8 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
WARN_ON_ONCE(current->thread.trap_nr != UPROBE_TRAP_NR);
current->thread.trap_nr = utask->autask.saved_trap_nr;
- if (auprobe->ops->post_xol) {
- err = auprobe->ops->post_xol(auprobe, regs);
+ if (auprobe->xol.ops->post_xol) {
+ err = auprobe->xol.ops->post_xol(auprobe, regs);
if (err) {
/*
* Restore ->ip for restart or post mortem analysis.
@@ -1754,8 +1754,8 @@ void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
{
struct uprobe_task *utask = current->utask;
- if (auprobe->ops->abort)
- auprobe->ops->abort(auprobe, regs);
+ if (auprobe->xol.ops->abort)
+ auprobe->xol.ops->abort(auprobe, regs);
current->thread.trap_nr = utask->autask.saved_trap_nr;
regs->ip = utask->vaddr;
@@ -1766,8 +1766,8 @@ void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
static bool __skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
{
- if (auprobe->ops->emulate)
- return auprobe->ops->emulate(auprobe, regs);
+ if (auprobe->xol.ops->emulate)
+ return auprobe->xol.ops->emulate(auprobe, regs);
return false;
}
--
2.51.1
^ permalink raw reply related
* [RFC PATCH 0/8] uprobe/x86: Add support to optimize prologue
From: Jiri Olsa @ 2025-11-17 12:40 UTC (permalink / raw)
To: Oleg Nesterov, Masami Hiramatsu, Peter Zijlstra, Andrii Nakryiko
Cc: bpf, linux-kernel, linux-trace-kernel, x86, Song Liu,
Yonghong Song, John Fastabend, Steven Rostedt, Ingo Molnar,
David Laight
hi,
the subject is bit too optimistic, in nutshell the idea is to allow
optimization on top of emulated instructions and then add support to
emulate more instructions with high presence in function prologues.
This patchset adds support to optimize uprobe on top of instruction
that could be emulated and also adds support to emulate particular
versions of mov and sub instructions to cover some of the user space
functions prologues, like:
pushq %rbp
movq %rsp,%rbp
subq $0xb0,%rsp
The idea is to store instructions on underlying 5 bytes and emulate
them during the int3 and uprobe syscall execution:
- install 'call trampoline' through standard int3 update
- if int3 is hit before we finish optimizing we emulate
all underlying instructions
- when call is installed the uprobe syscall will emulate
all underlying instructions
There's an additional issue that single instruction replacement does
not have and it's the possibility of the user space code to jump in the
middle of those 5 bytes. I think it's unlikely to happen at the function
prologue, but uprobe could be placed anywhere. I'm not sure how to
mitigate this other than having some enable/disable switch or config
option, which is unfortunate.
The patchset is based on bpf-next/master with [1] changes merged in.
thanks,
jirka
[1] https://lore.kernel.org/lkml/20251117093137.572132-1-jolsa@kernel.org/T/#m95a3208943ec24c5eee17ad6113002fdc6776cf8
---
Jiri Olsa (8):
uprobe/x86: Introduce struct arch_uprobe_xol object
uprobe/x86: Use struct arch_uprobe_xol in emulate callback
uprobe/x86: Add support to emulate mov reg,reg instructions
uprobe/x86: Add support to emulate sub imm,reg instructions
uprobe/x86: Add support to optimize on top of emulated instructions
selftests/bpf: Add test for mov and sub emulation
selftests/bpf: Add test for uprobe prologue optimization
selftests/bpf: Add race test for uprobe proglog optimization
arch/x86/include/asm/uprobes.h | 35 +++++++---
arch/x86/kernel/uprobes.c | 336 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------
include/linux/uprobes.h | 1 +
kernel/events/uprobes.c | 6 ++
tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c | 129 ++++++++++++++++++++++++++++++++-----
5 files changed, 434 insertions(+), 73 deletions(-)
^ permalink raw reply
* Re: [RFC PATCH 0/8] xfs: single block atomic writes for buffered IO
From: John Garry @ 2025-11-17 10:59 UTC (permalink / raw)
To: Dave Chinner, Ojaswin Mujoo
Cc: Ritesh Harjani, Christoph Hellwig, Christian Brauner, djwong,
tytso, willy, dchinner, linux-xfs, linux-kernel, linux-ext4,
linux-fsdevel, linux-mm, jack, nilay, martin.petersen, rostedt,
axboe, linux-block, linux-trace-kernel
In-Reply-To: <aRmHRk7FGD4nCT0s@dread.disaster.area>
On 16/11/2025 08:11, Dave Chinner wrote:
>> This patch set focuses on HW accelerated single block atomic writes with
>> buffered IO, to get some early reviews on the core design.
> What hardware acceleration? Hardware atomic writes are do not make
> IO faster; they only change IO failure semantics in certain corner
> cases.
I think that he references using REQ_ATOMIC-based bio vs xfs
software-based atomic writes (which reuse the CoW infrastructure). And
the former is considerably faster from my testing (for DIO, obvs). But
the latter has not been optimized.
^ permalink raw reply
* Re: [PATCH v3 13/21] mmc: mmc_test: Switch to use %ptSp
From: Ulf Hansson @ 2025-11-17 11:00 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Corey Minyard, Christian König, Dr. David Alan Gilbert,
Alex Deucher, Thomas Zimmermann, Dmitry Baryshkov, Rob Clark,
Matthew Brost, Aleksandr Loktionov, Vitaly Lifshits,
Manivannan Sadhasivam, Niklas Cassel, Calvin Owens,
Vadim Fedorenko, Sagi Maimon, Martin K. Petersen,
Karan Tilak Kumar, Hans Verkuil, Casey Schaufler, Steven Rostedt,
Petr Mladek, Viacheslav Dubeyko, Max Kellermann, linux-doc,
linux-kernel, openipmi-developer, linux-media, dri-devel,
linaro-mm-sig, amd-gfx, linux-arm-msm, freedreno, intel-xe,
linux-mmc, netdev, intel-wired-lan, linux-pci, linux-s390,
linux-scsi, linux-staging, ceph-devel, linux-trace-kernel,
Rasmus Villemoes, Sergey Senozhatsky, Jonathan Corbet,
Sumit Semwal, Gustavo Padovan, David Airlie, Simona Vetter,
Maarten Lankhorst, Maxime Ripard, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Sean Paul, Marijn Suijten, Konrad Dybcio,
Lucas De Marchi, Thomas Hellström, Rodrigo Vivi,
Vladimir Oltean, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Tony Nguyen, Przemek Kitszel,
Krzysztof Wilczyński, Kishon Vijay Abraham I, Bjorn Helgaas,
Rodolfo Giometti, Jonathan Lemon, Richard Cochran,
Stefan Haberland, Jan Hoeppner, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Satish Kharat, Sesidhar Baddela, James E.J. Bottomley,
Mauro Carvalho Chehab, Greg Kroah-Hartman, Xiubo Li, Ilya Dryomov,
Masami Hiramatsu, Mathieu Desnoyers, Andrew Morton
In-Reply-To: <20251113150217.3030010-14-andriy.shevchenko@linux.intel.com>
On Thu, 13 Nov 2025 at 16:03, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Use %ptSp instead of open coded variants to print content of
> struct timespec64 in human readable format.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
Kind regards
Uffe
> ---
> drivers/mmc/core/mmc_test.c | 20 ++++++++------------
> 1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc_test.c b/drivers/mmc/core/mmc_test.c
> index a74089df4547..01d1e62c2ce7 100644
> --- a/drivers/mmc/core/mmc_test.c
> +++ b/drivers/mmc/core/mmc_test.c
> @@ -586,14 +586,11 @@ static void mmc_test_print_avg_rate(struct mmc_test_card *test, uint64_t bytes,
> rate = mmc_test_rate(tot, &ts);
> iops = mmc_test_rate(count * 100, &ts); /* I/O ops per sec x 100 */
>
> - pr_info("%s: Transfer of %u x %u sectors (%u x %u%s KiB) took "
> - "%llu.%09u seconds (%u kB/s, %u KiB/s, "
> - "%u.%02u IOPS, sg_len %d)\n",
> - mmc_hostname(test->card->host), count, sectors, count,
> - sectors >> 1, (sectors & 1 ? ".5" : ""),
> - (u64)ts.tv_sec, (u32)ts.tv_nsec,
> - rate / 1000, rate / 1024, iops / 100, iops % 100,
> - test->area.sg_len);
> + pr_info("%s: Transfer of %u x %u sectors (%u x %u%s KiB) took %ptSp seconds (%u kB/s, %u KiB/s, %u.%02u IOPS, sg_len %d)\n",
> + mmc_hostname(test->card->host), count, sectors, count,
> + sectors >> 1, (sectors & 1 ? ".5" : ""), &ts,
> + rate / 1000, rate / 1024, iops / 100, iops % 100,
> + test->area.sg_len);
>
> mmc_test_save_transfer_result(test, count, sectors, ts, rate, iops);
> }
> @@ -3074,10 +3071,9 @@ static int mtf_test_show(struct seq_file *sf, void *data)
> seq_printf(sf, "Test %d: %d\n", gr->testcase + 1, gr->result);
>
> list_for_each_entry(tr, &gr->tr_lst, link) {
> - seq_printf(sf, "%u %d %llu.%09u %u %u.%02u\n",
> - tr->count, tr->sectors,
> - (u64)tr->ts.tv_sec, (u32)tr->ts.tv_nsec,
> - tr->rate, tr->iops / 100, tr->iops % 100);
> + seq_printf(sf, "%u %d %ptSp %u %u.%02u\n",
> + tr->count, tr->sectors, &tr->ts, tr->rate,
> + tr->iops / 100, tr->iops % 100);
> }
> }
>
> --
> 2.50.1
>
^ permalink raw reply
* Re: [PATCH v1] cpufreq: Add policy_frequency trace event
From: Christian Loehle @ 2025-11-17 9:18 UTC (permalink / raw)
To: Viresh Kumar, Samuel Wu, peterz, vincent.guittot
Cc: Rafael J. Wysocki, Steven Rostedt, Masami Hiramatsu,
Mathieu Desnoyers, kernel-team, linux-pm, linux-kernel,
linux-trace-kernel, Dietmar Eggemann, Qais Yousef, John Stultz
In-Reply-To: <5pv5tqf24p6ttpydpdegyhyod3m2hmpwbfrzl6otsq3q2gvb2s@gsgcgbcvin3u>
On 11/14/25 05:11, Viresh Kumar wrote:
> On 13-11-25, 19:41, Samuel Wu wrote:
>> On Wed, Nov 12, 2025 at 10:45 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>>
>>> On 12-11-25, 15:51, Samuel Wu wrote:
>>>> The existing cpu_frequency trace_event can be verbose, emitting an event
>>>> for every CPU in the policy even when their frequencies are identical.
>>>>
>>>> This patch adds a new policy_frequency trace event, which provides a
>>>> more efficient alternative to cpu_frequency trace event. This option
>>>> allows users who only need frequency at a policy level more concise logs
>>>> with simpler analysis.
>>>>
>>>> Signed-off-by: Samuel Wu <wusamuel@google.com>
>>>> ---
>>>> drivers/cpufreq/cpufreq.c | 2 ++
>>>> include/trace/events/power.h | 21 +++++++++++++++++++++
>>>> 2 files changed, 23 insertions(+)
>>>>
>>>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>>>> index 4472bb1ec83c..b65534a4fd9a 100644
>>>> --- a/drivers/cpufreq/cpufreq.c
>>>> +++ b/drivers/cpufreq/cpufreq.c
>>>> @@ -345,6 +345,7 @@ static void cpufreq_notify_transition(struct cpufreq_policy *policy,
>>>> pr_debug("FREQ: %u - CPUs: %*pbl\n", freqs->new,
>>>> cpumask_pr_args(policy->cpus));
>>>>
>>>> + trace_policy_frequency(freqs->new, policy->cpu);
>>>> for_each_cpu(cpu, policy->cpus)
>>>> trace_cpu_frequency(freqs->new, cpu);
>>>
>>> I don't see much value in almost duplicate trace events. If we feel that a
>>> per-policy event is a better fit (which makes sens), then we can just drop the
>>> trace_cpu_frequency() events and print policy->cpus (or related_cpus)
>>> information along with the per-policy events.
>>
>> Thank you for the feedback Viresh. Fair enough, I've done some testing
>> and a single trace event should work and would be cleaner. Please let
>> me know what you think of this proposal for v2.
>>
>> We can append a bitmask of policy->cpus field to
>> trace_cpu_frequency(). This way we maintain backwards compatibility:
>> trace_cpu_frequency() is not removed, and its pre-existing fields are
>> not disturbed.
>>
>> Call flow wise, we can delete all the for_each_cpu() loops, and we
>> still retain the benefits of the trace emitting once per policy
>> instead of once per cpu.
>
> Fine by me. I have added Scheduler maintainers in the loop to see if they have a
> different view.
>
And IIUC your proposal is to fold policy_frequency into cpu_frequency but then
only have one cpu_frequency event per policy emitted?
I think from a tooling perspective it would be easier to remove cpu_frequency
entirely, then tools can probe on the presence of policy_frequency / cpu_frequency.
^ permalink raw reply
* Re: [PATCH v1] cpufreq: Add policy_frequency trace event
From: Christian Loehle @ 2025-11-17 9:14 UTC (permalink / raw)
To: Viresh Kumar, Samuel Wu, peterz, vincent.guittot
Cc: Rafael J. Wysocki, Steven Rostedt, Masami Hiramatsu,
Mathieu Desnoyers, kernel-team, linux-pm, linux-kernel,
linux-trace-kernel, Dietmar Eggemann, Qais Yousef, John Stultz
In-Reply-To: <5pv5tqf24p6ttpydpdegyhyod3m2hmpwbfrzl6otsq3q2gvb2s@gsgcgbcvin3u>
On 11/14/25 05:11, Viresh Kumar wrote:
> On 13-11-25, 19:41, Samuel Wu wrote:
>> On Wed, Nov 12, 2025 at 10:45 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>>
>>> On 12-11-25, 15:51, Samuel Wu wrote:
>>>> The existing cpu_frequency trace_event can be verbose, emitting an event
>>>> for every CPU in the policy even when their frequencies are identical.
>>>>
>>>> This patch adds a new policy_frequency trace event, which provides a
>>>> more efficient alternative to cpu_frequency trace event. This option
>>>> allows users who only need frequency at a policy level more concise logs
>>>> with simpler analysis.
>>>>
>>>> Signed-off-by: Samuel Wu <wusamuel@google.com>
>>>> ---
>>>> drivers/cpufreq/cpufreq.c | 2 ++
>>>> include/trace/events/power.h | 21 +++++++++++++++++++++
>>>> 2 files changed, 23 insertions(+)
>>>>
>>>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>>>> index 4472bb1ec83c..b65534a4fd9a 100644
>>>> --- a/drivers/cpufreq/cpufreq.c
>>>> +++ b/drivers/cpufreq/cpufreq.c
>>>> @@ -345,6 +345,7 @@ static void cpufreq_notify_transition(struct cpufreq_policy *policy,
>>>> pr_debug("FREQ: %u - CPUs: %*pbl\n", freqs->new,
>>>> cpumask_pr_args(policy->cpus));
>>>>
>>>> + trace_policy_frequency(freqs->new, policy->cpu);
>>>> for_each_cpu(cpu, policy->cpus)
>>>> trace_cpu_frequency(freqs->new, cpu);
>>>
>>> I don't see much value in almost duplicate trace events. If we feel that a
>>> per-policy event is a better fit (which makes sens), then we can just drop the
>>> trace_cpu_frequency() events and print policy->cpus (or related_cpus)
>>> information along with the per-policy events.
>>
>> Thank you for the feedback Viresh. Fair enough, I've done some testing
>> and a single trace event should work and would be cleaner. Please let
>> me know what you think of this proposal for v2.
>>
>> We can append a bitmask of policy->cpus field to
>> trace_cpu_frequency(). This way we maintain backwards compatibility:
>> trace_cpu_frequency() is not removed, and its pre-existing fields are
>> not disturbed.
>>
>> Call flow wise, we can delete all the for_each_cpu() loops, and we
>> still retain the benefits of the trace emitting once per policy
>> instead of once per cpu.
>
> Fine by me. I have added Scheduler maintainers in the loop to see if they have a
> different view.
>
That's gonna break a lot of tooling but AFAICS that in and of itself isn't
a good enough reason not to do it.
(Added some CCs)
^ permalink raw reply
* [PATCH v2 2/2] rv: Convert to use __free
From: Nam Cao @ 2025-11-17 9:06 UTC (permalink / raw)
To: Gabriele Monaco, Steven Rostedt
Cc: Masami Hiramatsu, Mathieu Desnoyers, linux-trace-kernel,
linux-kernel, Nam Cao
In-Reply-To: <cover.1763370183.git.namcao@linutronix.de>
Convert to use __free to tidy up the code.
Signed-off-by: Nam Cao <namcao@linutronix.de>
---
v2: address Gabriele's comments:
- simply with no_free_ptr()
- add back a removed printk()
- add a NULL check to DEFINE_FREE(rv_remove, ...)
- add a missing __free()
---
kernel/trace/rv/rv.c | 63 +++++++++++++++--------------------
kernel/trace/rv/rv.h | 6 ++--
kernel/trace/rv/rv_reactors.c | 32 ++++++++----------
kernel/trace/trace.c | 3 +-
4 files changed, 46 insertions(+), 58 deletions(-)
diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c
index b1059a3cf4fa..ee4e68102f17 100644
--- a/kernel/trace/rv/rv.c
+++ b/kernel/trace/rv/rv.c
@@ -420,35 +420,27 @@ static const struct file_operations interface_desc_fops = {
static int create_monitor_dir(struct rv_monitor *mon, struct rv_monitor *parent)
{
struct dentry *root = parent ? parent->root_d : get_monitors_root();
- const char *name = mon->name;
+ struct dentry *dir __free(rv_remove) = rv_create_dir(mon->name, root);
struct dentry *tmp;
int retval;
- mon->root_d = rv_create_dir(name, root);
- if (!mon->root_d)
+ if (!dir)
return -ENOMEM;
- tmp = rv_create_file("enable", RV_MODE_WRITE, mon->root_d, mon, &interface_enable_fops);
- if (!tmp) {
- retval = -ENOMEM;
- goto out_remove_root;
- }
+ tmp = rv_create_file("enable", RV_MODE_WRITE, dir, mon, &interface_enable_fops);
+ if (!tmp)
+ return -ENOMEM;
- tmp = rv_create_file("desc", RV_MODE_READ, mon->root_d, mon, &interface_desc_fops);
- if (!tmp) {
- retval = -ENOMEM;
- goto out_remove_root;
- }
+ tmp = rv_create_file("desc", RV_MODE_READ, dir, mon, &interface_desc_fops);
+ if (!tmp)
+ return -ENOMEM;
- retval = reactor_populate_monitor(mon);
+ retval = reactor_populate_monitor(mon, dir);
if (retval)
- goto out_remove_root;
+ return retval;
+ mon->root_d = no_free_ptr(dir);
return 0;
-
-out_remove_root:
- rv_remove(mon->root_d);
- return retval;
}
/*
@@ -827,39 +819,36 @@ int __init rv_init_interface(void)
{
struct dentry *tmp;
int retval;
+ struct dentry *root_dir __free(rv_remove) = rv_create_dir("rv", NULL);
- rv_root.root_dir = rv_create_dir("rv", NULL);
- if (!rv_root.root_dir)
- goto out_err;
+ if (!root_dir)
+ return 1;
- rv_root.monitors_dir = rv_create_dir("monitors", rv_root.root_dir);
+ rv_root.monitors_dir = rv_create_dir("monitors", root_dir);
if (!rv_root.monitors_dir)
- goto out_err;
+ return 1;
- tmp = rv_create_file("available_monitors", RV_MODE_READ, rv_root.root_dir, NULL,
+ tmp = rv_create_file("available_monitors", RV_MODE_READ, root_dir, NULL,
&available_monitors_ops);
if (!tmp)
- goto out_err;
+ return 1;
- tmp = rv_create_file("enabled_monitors", RV_MODE_WRITE, rv_root.root_dir, NULL,
+ tmp = rv_create_file("enabled_monitors", RV_MODE_WRITE, root_dir, NULL,
&enabled_monitors_ops);
if (!tmp)
- goto out_err;
+ return 1;
- tmp = rv_create_file("monitoring_on", RV_MODE_WRITE, rv_root.root_dir, NULL,
+ tmp = rv_create_file("monitoring_on", RV_MODE_WRITE, root_dir, NULL,
&monitoring_on_fops);
if (!tmp)
- goto out_err;
- retval = init_rv_reactors(rv_root.root_dir);
+ return 1;
+ retval = init_rv_reactors(root_dir);
if (retval)
- goto out_err;
+ return 1;
turn_monitoring_on();
- return 0;
+ rv_root.root_dir = no_free_ptr(root_dir);
-out_err:
- rv_remove(rv_root.root_dir);
- printk(KERN_ERR "RV: Error while creating the RV interface\n");
- return 1;
+ return 0;
}
diff --git a/kernel/trace/rv/rv.h b/kernel/trace/rv/rv.h
index 1485a70c1bf4..2c0f51ff9d5c 100644
--- a/kernel/trace/rv/rv.h
+++ b/kernel/trace/rv/rv.h
@@ -17,6 +17,8 @@ struct rv_interface {
#define rv_create_file tracefs_create_file
#define rv_remove tracefs_remove
+DEFINE_FREE(rv_remove, struct dentry *, if (_T) rv_remove(_T));
+
#define MAX_RV_MONITOR_NAME_SIZE 32
#define MAX_RV_REACTOR_NAME_SIZE 32
@@ -30,10 +32,10 @@ bool rv_is_container_monitor(struct rv_monitor *mon);
bool rv_is_nested_monitor(struct rv_monitor *mon);
#ifdef CONFIG_RV_REACTORS
-int reactor_populate_monitor(struct rv_monitor *mon);
+int reactor_populate_monitor(struct rv_monitor *mon, struct dentry *root);
int init_rv_reactors(struct dentry *root_dir);
#else
-static inline int reactor_populate_monitor(struct rv_monitor *mon)
+static inline int reactor_populate_monitor(struct rv_monitor *mon, struct dentry *root)
{
return 0;
}
diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
index 37d1d37a27a3..4a98967d677b 100644
--- a/kernel/trace/rv/rv_reactors.c
+++ b/kernel/trace/rv/rv_reactors.c
@@ -404,14 +404,15 @@ static const struct file_operations reacting_on_fops = {
/**
* reactor_populate_monitor - creates per monitor reactors file
* @mon: The monitor.
+ * @root: The directory of the monitor.
*
* Returns 0 if successful, error otherwise.
*/
-int reactor_populate_monitor(struct rv_monitor *mon)
+int reactor_populate_monitor(struct rv_monitor *mon, struct dentry *root)
{
struct dentry *tmp;
- tmp = rv_create_file("reactors", RV_MODE_WRITE, mon->root_d, mon, &monitor_reactors_ops);
+ tmp = rv_create_file("reactors", RV_MODE_WRITE, root, mon, &monitor_reactors_ops);
if (!tmp)
return -ENOMEM;
@@ -438,30 +439,25 @@ static struct rv_reactor rv_nop = {
int init_rv_reactors(struct dentry *root_dir)
{
- struct dentry *available, *reacting;
int retval;
- available = rv_create_file("available_reactors", RV_MODE_READ, root_dir, NULL,
- &available_reactors_ops);
- if (!available)
- goto out_err;
+ struct dentry *available __free(rv_remove) =
+ rv_create_file("available_reactors", RV_MODE_READ, root_dir,
+ NULL, &available_reactors_ops);
- reacting = rv_create_file("reacting_on", RV_MODE_WRITE, root_dir, NULL, &reacting_on_fops);
- if (!reacting)
- goto rm_available;
+ struct dentry *reacting __free(rv_remove) =
+ rv_create_file("reacting_on", RV_MODE_WRITE, root_dir, NULL, &reacting_on_fops);
+
+ if (!reacting || !available)
+ return -ENOMEM;
retval = __rv_register_reactor(&rv_nop);
if (retval)
- goto rm_reacting;
+ return retval;
turn_reacting_on();
+ retain_and_null_ptr(available);
+ retain_and_null_ptr(reacting);
return 0;
-
-rm_reacting:
- rv_remove(reacting);
-rm_available:
- rv_remove(available);
-out_err:
- return -ENOMEM;
}
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index d1e527cf2aae..cd209598d670 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -10640,7 +10640,8 @@ static __init int tracer_init_tracefs(void)
tracer_init_tracefs_work_func(NULL);
}
- rv_init_interface();
+ if (rv_init_interface())
+ pr_err("RV: Error while creating the RV interface\n");
return 0;
}
--
2.51.0
^ permalink raw reply related
* [PATCH v2 1/2] rv: Convert to use lock guard
From: Nam Cao @ 2025-11-17 9:06 UTC (permalink / raw)
To: Gabriele Monaco, Steven Rostedt
Cc: Masami Hiramatsu, Mathieu Desnoyers, linux-trace-kernel,
linux-kernel, Nam Cao
In-Reply-To: <cover.1763370183.git.namcao@linutronix.de>
Convert to use lock guard to tidy up the code.
Signed-off-by: Nam Cao <namcao@linutronix.de>
---
v2: no change
---
kernel/trace/rv/rv.c | 39 ++++++++++++-----------------------
kernel/trace/rv/rv_reactors.c | 26 +++++++----------------
2 files changed, 20 insertions(+), 45 deletions(-)
diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c
index 43e9ea473cda..b1059a3cf4fa 100644
--- a/kernel/trace/rv/rv.c
+++ b/kernel/trace/rv/rv.c
@@ -375,15 +375,13 @@ static ssize_t monitor_enable_write_data(struct file *filp, const char __user *u
if (retval)
return retval;
- mutex_lock(&rv_interface_lock);
+ guard(mutex)(&rv_interface_lock);
if (val)
retval = rv_enable_monitor(mon);
else
retval = rv_disable_monitor(mon);
- mutex_unlock(&rv_interface_lock);
-
return retval ? : count;
}
@@ -568,7 +566,7 @@ static void disable_all_monitors(void)
struct rv_monitor *mon;
int enabled = 0;
- mutex_lock(&rv_interface_lock);
+ guard(mutex)(&rv_interface_lock);
list_for_each_entry(mon, &rv_monitors_list, list)
enabled += __rv_disable_monitor(mon, false);
@@ -581,8 +579,6 @@ static void disable_all_monitors(void)
*/
tracepoint_synchronize_unregister();
}
-
- mutex_unlock(&rv_interface_lock);
}
static int enabled_monitors_open(struct inode *inode, struct file *file)
@@ -623,7 +619,7 @@ static ssize_t enabled_monitors_write(struct file *filp, const char __user *user
if (!len)
return count;
- mutex_lock(&rv_interface_lock);
+ guard(mutex)(&rv_interface_lock);
retval = -EINVAL;
@@ -644,13 +640,11 @@ static ssize_t enabled_monitors_write(struct file *filp, const char __user *user
else
retval = rv_disable_monitor(mon);
- if (!retval)
- retval = count;
-
- break;
+ if (retval)
+ return retval;
+ return count;
}
- mutex_unlock(&rv_interface_lock);
return retval;
}
@@ -737,7 +731,7 @@ static ssize_t monitoring_on_write_data(struct file *filp, const char __user *us
if (retval)
return retval;
- mutex_lock(&rv_interface_lock);
+ guard(mutex)(&rv_interface_lock);
if (val)
turn_monitoring_on_with_reset();
@@ -750,8 +744,6 @@ static ssize_t monitoring_on_write_data(struct file *filp, const char __user *us
*/
tracepoint_synchronize_unregister();
- mutex_unlock(&rv_interface_lock);
-
return count;
}
@@ -784,28 +776,26 @@ int rv_register_monitor(struct rv_monitor *monitor, struct rv_monitor *parent)
return -EINVAL;
}
- mutex_lock(&rv_interface_lock);
+ guard(mutex)(&rv_interface_lock);
list_for_each_entry(r, &rv_monitors_list, list) {
if (strcmp(monitor->name, r->name) == 0) {
pr_info("Monitor %s is already registered\n", monitor->name);
- retval = -EEXIST;
- goto out_unlock;
+ return -EEXIST;
}
}
if (parent && rv_is_nested_monitor(parent)) {
pr_info("Parent monitor %s is already nested, cannot nest further\n",
parent->name);
- retval = -EINVAL;
- goto out_unlock;
+ return -EINVAL;
}
monitor->parent = parent;
retval = create_monitor_dir(monitor, parent);
if (retval)
- goto out_unlock;
+ return retval;
/* keep children close to the parent for easier visualisation */
if (parent)
@@ -813,9 +803,7 @@ int rv_register_monitor(struct rv_monitor *monitor, struct rv_monitor *parent)
else
list_add_tail(&monitor->list, &rv_monitors_list);
-out_unlock:
- mutex_unlock(&rv_interface_lock);
- return retval;
+ return 0;
}
/**
@@ -826,13 +814,12 @@ int rv_register_monitor(struct rv_monitor *monitor, struct rv_monitor *parent)
*/
int rv_unregister_monitor(struct rv_monitor *monitor)
{
- mutex_lock(&rv_interface_lock);
+ guard(mutex)(&rv_interface_lock);
rv_disable_monitor(monitor);
list_del(&monitor->list);
destroy_monitor_dir(monitor);
- mutex_unlock(&rv_interface_lock);
return 0;
}
diff --git a/kernel/trace/rv/rv_reactors.c b/kernel/trace/rv/rv_reactors.c
index d32859fec238..37d1d37a27a3 100644
--- a/kernel/trace/rv/rv_reactors.c
+++ b/kernel/trace/rv/rv_reactors.c
@@ -232,9 +232,7 @@ monitor_reactors_write(struct file *file, const char __user *user_buf,
seq_f = file->private_data;
mon = seq_f->private;
- mutex_lock(&rv_interface_lock);
-
- retval = -EINVAL;
+ guard(mutex)(&rv_interface_lock);
list_for_each_entry(reactor, &rv_reactors_list, list) {
if (strcmp(ptr, reactor->name) != 0)
@@ -242,13 +240,10 @@ monitor_reactors_write(struct file *file, const char __user *user_buf,
monitor_swap_reactors(mon, reactor);
- retval = count;
- break;
+ return count;
}
- mutex_unlock(&rv_interface_lock);
-
- return retval;
+ return -EINVAL;
}
/*
@@ -309,18 +304,14 @@ static int __rv_register_reactor(struct rv_reactor *reactor)
*/
int rv_register_reactor(struct rv_reactor *reactor)
{
- int retval = 0;
-
if (strlen(reactor->name) >= MAX_RV_REACTOR_NAME_SIZE) {
pr_info("Reactor %s has a name longer than %d\n",
reactor->name, MAX_RV_MONITOR_NAME_SIZE);
return -EINVAL;
}
- mutex_lock(&rv_interface_lock);
- retval = __rv_register_reactor(reactor);
- mutex_unlock(&rv_interface_lock);
- return retval;
+ guard(mutex)(&rv_interface_lock);
+ return __rv_register_reactor(reactor);
}
/**
@@ -331,9 +322,8 @@ int rv_register_reactor(struct rv_reactor *reactor)
*/
int rv_unregister_reactor(struct rv_reactor *reactor)
{
- mutex_lock(&rv_interface_lock);
+ guard(mutex)(&rv_interface_lock);
list_del(&reactor->list);
- mutex_unlock(&rv_interface_lock);
return 0;
}
@@ -389,7 +379,7 @@ static ssize_t reacting_on_write_data(struct file *filp, const char __user *user
if (retval)
return retval;
- mutex_lock(&rv_interface_lock);
+ guard(mutex)(&rv_interface_lock);
if (val)
turn_reacting_on();
@@ -402,8 +392,6 @@ static ssize_t reacting_on_write_data(struct file *filp, const char __user *user
*/
tracepoint_synchronize_unregister();
- mutex_unlock(&rv_interface_lock);
-
return count;
}
--
2.51.0
^ permalink raw reply related
* [PATCH v2 0/2] rv: Tidy up with auto-cleanup
From: Nam Cao @ 2025-11-17 9:06 UTC (permalink / raw)
To: Gabriele Monaco, Steven Rostedt
Cc: Masami Hiramatsu, Mathieu Desnoyers, linux-trace-kernel,
linux-kernel, Nam Cao
This series tidies up rv code with __free and lock guard. This is motivated
by a bug:
https://lore.kernel.org/all/20250903065112.1878330-1-zhen.ni@easystack.cn/
which was caused by a missing mutex_unlock. This series makes such bug less
likely in the future, and also reduces some code lines.
Nam Cao (2):
rv: Convert to use lock guard
rv: Convert to use __free
kernel/trace/rv/rv.c | 102 +++++++++++++---------------------
kernel/trace/rv/rv.h | 6 +-
kernel/trace/rv/rv_reactors.c | 58 +++++++------------
kernel/trace/trace.c | 3 +-
4 files changed, 66 insertions(+), 103 deletions(-)
--
2.51.0
^ permalink raw reply
* Re: [PATCH 2/2] rv: Convert to use __free
From: Nam Cao @ 2025-11-17 8:30 UTC (permalink / raw)
To: Gabriele Monaco, Steven Rostedt
Cc: Masami Hiramatsu, Mathieu Desnoyers, linux-trace-kernel,
linux-kernel
In-Reply-To: <cb44571efe8157f126e519b7cf1932ad7feda7fb.camel@redhat.com>
Gabriele Monaco <gmonaco@redhat.com> writes:
> On Sun, 2025-11-16 at 15:35 +0000, Nam Cao wrote:
>> - reacting = rv_create_file("reacting_on", RV_MODE_WRITE, root_dir,
>> NULL, &reacting_on_fops);
>> - if (!reacting)
>> - goto rm_available;
>> + struct dentry *reacting =
>> + rv_create_file("reacting_on", RV_MODE_WRITE, root_dir, NULL,
>> &reacting_on_fops);
>
> Nothing is removing "reacting_on" in case of successive failure, is it?
> Am I missing anything or couldn't we just set both variables to __free() ?
We can. I overlooked this one :(
Nam
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox