On 08/12, Andrii Nakryiko wrote:
>
> Avoid taking refcount on uprobe in prepare_uretprobe(), instead take
> uretprobe-specific SRCU lock and keep it active as kernel transfers
> control back to user space.
...
> include/linux/uprobes.h | 49 ++++++-
> kernel/events/uprobes.c | 294 ++++++++++++++++++++++++++++++++++------
> 2 files changed, 301 insertions(+), 42 deletions(-)
Oh. To be honest I don't like this patch.
I would like to know what other reviewers think, but to me it adds too many
complications that I can't even fully understand...
And how much does it help performance-wise?
I'll try to take another look, and I'll try to think about other approaches,
not that I have something better in mind...
But lets forgets this patch for the moment. The next one adds even more
complications, and I think it doesn't make sense.
As I have already mentioned in the previous discussions, we can simply kill
utask->active_uprobe. And utask->auprobe.
So can't we start with the patch below? On top of your 08/13. It doesn't kill
utask->auprobe yet, this needs a bit more trivial changes.
What do you think?
Oleg.
-------------------------------------------------------------------------------
>From d7cb674eb6f7bb891408b2b6a5fb872a6c2f0f6c Mon Sep 17 00:00:00 2001
From: Oleg Nesterov <o...@redhat.com>
Date: Mon, 19 Aug 2024 15:34:55 +0200
Subject: [RFC PATCH] uprobe: kill uprobe_task->active_uprobe
Untested, not for inclusion yet, and I need to split it into 2 changes.
It does 2 simple things:
1. active_uprobe != NULL is possible if and only if utask->state != 0,
so it turns the active_uprobe checks into the utask->state checks.
2. handle_singlestep() doesn't really need ->active_uprobe, it only
needs uprobe->arch which is "const" after prepare_uprobe().
So this patch adds the new "arch_uprobe uarch" member into utask
and changes pre_ssout() to do memcpy(&utask->uarch, &uprobe->arch).
---
include/linux/uprobes.h | 2 +-
kernel/events/uprobes.c | 37 +++++++++++--------------------------
2 files changed, 12 insertions(+), 27 deletions(-)
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 3a3154b74fe0..df6f3dab032c 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -56,6 +56,7 @@ struct uprobe_task {
union {
struct {
+ struct arch_uprobe uarch;
struct arch_uprobe_task autask;
unsigned long vaddr;
};
@@ -66,7 +67,6 @@ struct uprobe_task {
};
};
- struct uprobe *active_uprobe;
unsigned long xol_vaddr;
struct arch_uprobe *auprobe;
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index acc73c1bc54c..9689b557a5cf 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1721,7 +1721,7 @@ unsigned long uprobe_get_trap_addr(struct pt_regs *regs)
{
struct uprobe_task *utask = current->utask;
- if (unlikely(utask && utask->active_uprobe))
+ if (unlikely(utask && utask->state))
return utask->vaddr;
return instruction_pointer(regs);
@@ -1747,9 +1747,6 @@ void uprobe_free_utask(struct task_struct *t)
if (!utask)
return;
- if (utask->active_uprobe)
- put_uprobe(utask->active_uprobe);
-
ri = utask->return_instances;
while (ri)
ri = free_ret_instance(ri);
@@ -1965,14 +1962,9 @@ pre_ssout(struct uprobe *uprobe, struct pt_regs *regs,
unsigned long bp_vaddr)
if (!utask)
return -ENOMEM;
- if (!try_get_uprobe(uprobe))
- return -EINVAL;
-
xol_vaddr = xol_get_insn_slot(uprobe);
- if (!xol_vaddr) {
- err = -ENOMEM;
- goto err_out;
- }
+ if (!xol_vaddr)
+ return -ENOMEM;
utask->xol_vaddr = xol_vaddr;
utask->vaddr = bp_vaddr;
@@ -1980,15 +1972,12 @@ pre_ssout(struct uprobe *uprobe, struct pt_regs *regs,
unsigned long bp_vaddr)
err = arch_uprobe_pre_xol(&uprobe->arch, regs);
if (unlikely(err)) {
xol_free_insn_slot(current);
- goto err_out;
+ return err;
}
- utask->active_uprobe = uprobe;
+ memcpy(&utask->uarch, &uprobe->arch, sizeof(utask->uarch));
utask->state = UTASK_SSTEP;
return 0;
-err_out:
- put_uprobe(uprobe);
- return err;
}
/*
@@ -2005,7 +1994,7 @@ bool uprobe_deny_signal(void)
struct task_struct *t = current;
struct uprobe_task *utask = t->utask;
- if (likely(!utask || !utask->active_uprobe))
+ if (likely(!utask || !utask->state))
return false;
WARN_ON_ONCE(utask->state != UTASK_SSTEP);
@@ -2313,19 +2302,15 @@ static void handle_swbp(struct pt_regs *regs)
*/
static void handle_singlestep(struct uprobe_task *utask, struct pt_regs *regs)
{
- struct uprobe *uprobe;
int err = 0;
- uprobe = utask->active_uprobe;
if (utask->state == UTASK_SSTEP_ACK)
- err = arch_uprobe_post_xol(&uprobe->arch, regs);
+ err = arch_uprobe_post_xol(&utask->uarch, regs);
else if (utask->state == UTASK_SSTEP_TRAPPED)
- arch_uprobe_abort_xol(&uprobe->arch, regs);
+ arch_uprobe_abort_xol(&utask->uarch, regs);
else
WARN_ON_ONCE(1);
- put_uprobe(uprobe);
- utask->active_uprobe = NULL;
utask->state = UTASK_RUNNING;
xol_free_insn_slot(current);
@@ -2342,7 +2327,7 @@ static void handle_singlestep(struct uprobe_task *utask,
struct pt_regs *regs)
/*
* On breakpoint hit, breakpoint notifier sets the TIF_UPROBE flag and
* allows the thread to return from interrupt. After that handle_swbp()
- * sets utask->active_uprobe.
+ * sets utask->state != 0.
*
* On singlestep exception, singlestep notifier sets the TIF_UPROBE flag
* and allows the thread to return from interrupt.
@@ -2357,7 +2342,7 @@ void uprobe_notify_resume(struct pt_regs *regs)
clear_thread_flag(TIF_UPROBE);
utask = current->utask;
- if (utask && utask->active_uprobe)
+ if (utask && utask->state)
handle_singlestep(utask, regs);
else
handle_swbp(regs);
@@ -2388,7 +2373,7 @@ int uprobe_post_sstep_notifier(struct pt_regs *regs)
{
struct uprobe_task *utask = current->utask;
- if (!current->mm || !utask || !utask->active_uprobe)
+ if (!current->mm || !utask || !utask->state)
/* task is currently not uprobed */
return 0;
--
2.25.1.362.g51ebf55
