Re: [RFC 1/4] uprobe: Do not emulate/sstep original instruction when ip is changed

[Jiri Olsa]

On Mon, Aug 04, 2025 at 10:12:15AM +0200, Jiri Olsa wrote:
> On Sat, Aug 02, 2025 at 12:34:27PM +0200, Oleg Nesterov wrote:
> > On 08/01, Jiri Olsa wrote:
> > >
> > > If uprobe handler changes instruction pointer we still execute single
> > > step) or emulate the original instruction and increment the (new) ip
> > > with its length.
> > 
> > Yes... but what if we there are multiple consumers? The 1st one changes
> > instruction_pointer, the next is unaware. Or it may change regs->ip too...
> 
> right, and I think that's already bad in current code
> 
> how about we dd flag to the consumer that ensures it's the only consumer
> on the uprobe.. and we would skip original instruction execution for such
> uprobe if its consumer changes the regs->ip.. I'll try to come up with the
> patch

how about something like below?

jirka


---
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 516217c39094..b2c49a2d5468 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -59,6 +59,7 @@ struct uprobe_consumer {
 	struct list_head cons_node;
 
 	__u64 id;	/* set when uprobe_consumer is registered */
+	bool is_unique; /* the only consumer on uprobe */
 };
 
 #ifdef CONFIG_UPROBES
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index f774367c8e71..b317f9fbbf5c 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1014,14 +1014,32 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset,
 	return uprobe;
 }
 
-static void consumer_add(struct uprobe *uprobe, struct uprobe_consumer *uc)
+static bool consumer_can_add(struct list_head *head, struct uprobe_consumer *uc)
+{
+	/* there's no consumer, free to add one */
+	if (list_empty(head))
+		return true;
+	/* uprobe has consumer(s), can't add unique one */
+	if (uc->is_unique)
+		return false;
+	/* uprobe has consumer(s), we can add one only if it's not unique consumer */
+	return !list_first_entry(head, struct uprobe_consumer, cons_node)->is_unique;
+}
+
+static int consumer_add(struct uprobe *uprobe, struct uprobe_consumer *uc)
 {
 	static atomic64_t id;
+	int ret = -EBUSY;
 
 	down_write(&uprobe->consumer_rwsem);
+	if (!consumer_can_add(&uprobe->consumers, uc))
+		goto unlock;
 	list_add_rcu(&uc->cons_node, &uprobe->consumers);
 	uc->id = (__u64) atomic64_inc_return(&id);
+	ret = 0;
+unlock:
 	up_write(&uprobe->consumer_rwsem);
+	return ret;
 }
 
 /*
@@ -1410,7 +1428,12 @@ struct uprobe *uprobe_register(struct inode *inode,
 		return uprobe;
 
 	down_write(&uprobe->register_rwsem);
-	consumer_add(uprobe, uc);
+	ret = consumer_add(uprobe, uc);
+	if (ret) {
+		put_uprobe(uprobe);
+		up_write(&uprobe->register_rwsem);
+		return ERR_PTR(ret);
+	}
 	ret = register_for_each_vma(uprobe, uc);
 	up_write(&uprobe->register_rwsem);
 
@@ -2522,7 +2545,7 @@ static bool ignore_ret_handler(int rc)
 	return rc == UPROBE_HANDLER_REMOVE || rc == UPROBE_HANDLER_IGNORE;
 }
 
-static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
+static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs, bool *is_unique)
 {
 	struct uprobe_consumer *uc;
 	bool has_consumers = false, remove = true;
@@ -2536,6 +2559,8 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
 		__u64 cookie = 0;
 		int rc = 0;
 
+		*is_unique |= uc->is_unique;
+
 		if (uc->handler) {
 			rc = uc->handler(uc, regs, &cookie);
 			WARN(rc < 0 || rc > 2,
@@ -2685,6 +2710,7 @@ static void handle_swbp(struct pt_regs *regs)
 {
 	struct uprobe *uprobe;
 	unsigned long bp_vaddr;
+	bool is_unique = false;
 	int is_swbp;
 
 	bp_vaddr = uprobe_get_swbp_addr(regs);
@@ -2739,7 +2765,10 @@ static void handle_swbp(struct pt_regs *regs)
 	if (arch_uprobe_ignore(&uprobe->arch, regs))
 		goto out;
 
-	handler_chain(uprobe, regs);
+	handler_chain(uprobe, regs, &is_unique);
+
+	if (is_unique && instruction_pointer(regs) != bp_vaddr)
+		goto out;
 
 	if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
 		goto out;