Re: [PATCH RFC v3 09/13] uprobes: SRCU-protect uretprobe lifetime (with timeout)

[Oleg Nesterov <oleg@redhat.com>]

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 <oleg@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