uprobes: misc cleanups/simplifications

Peter, I don't think these changes can really complicate your ongoing work. But again, if you are going to send the next version "soon" I can rebase these cleanups on top of it.

Andrii, I dared to preserve your acks, all the changes are simple.

Changes since v1:

- update the comment in register_for_each_vma()
- remove the now unused "struct path *" arg from bpf_uprobe_unregister()
- forward-declare struct uprobe in include/linux/uprobes.h
- kernel-doc fixes/updates
- fix use-after free in uprobe_unregister(). See "TODO:" in 5/5.

Oleg.
---

 include/linux/uprobes.h     | 22 +++++------
 kernel/events/uprobes.c     | 96 +++++++++++++++++----------------------------
 kernel/trace/bpf_trace.c    | 25 ++++++------
 kernel/trace/trace_uprobe.c | 31 ++++++---------
 4 files changed, 69 insertions(+), 105 deletions(-)

It doesn't make any sense to have 2 versions of _register(). Note that trace_uprobe_enable(), the only user of uprobe_register(), doesn't need to check tu->ref_ctr_offset to decide which one should be used, it could safely pass ref_ctr_offset == 0 to uprobe_register_refctr().

Add this argument to uprobe_register(), update the callers, and kill uprobe_register_refctr().

Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Andrii Nakryiko <[email protected]>

---
 include/linux/uprobes.h     |  9 ++-------
 kernel/events/uprobes.c     | 24 ++++++------------------
 kernel/trace/bpf_trace.c    |  8 ++++----
 kernel/trace/trace_uprobe.c |  7 +------
 4 files changed, 13 insertions(+), 35 deletions(-)

include/linux/uprobes.h +2 -7

index b503fafb7fb3..440316fbf3c6 100644
@@ -110,8 +110,7 @@ extern bool is_trap_insn(uprobe_opcode_t *insn);
 extern unsigned long uprobe_get_swbp_addr(struct pt_regs *regs);
 extern unsigned long uprobe_get_trap_addr(struct pt_regs *regs);
 extern int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_t);
-extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
-extern int uprobe_register_refctr(struct inode *inode, loff_t offset, loff_t ref_ctr_offset, struct uprobe_consumer *uc);
+extern int uprobe_register(struct inode *inode, loff_t offset, loff_t ref_ctr_offset, struct uprobe_consumer *uc);
 extern int uprobe_apply(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, bool);
 extern void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
 extern int uprobe_mmap(struct vm_area_struct *vma);

@@ -152,11 +151,7 @@ static inline void uprobes_init(void)
 #define uprobe_get_trap_addr(regs)	instruction_pointer(regs)
 
 static inline int
-uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc)
-{
-	return -ENOSYS;
-}
-static inline int uprobe_register_refctr(struct inode *inode, loff_t offset, loff_t ref_ctr_offset, struct uprobe_consumer *uc)
+uprobe_register(struct inode *inode, loff_t offset, loff_t ref_ctr_offset, struct uprobe_consumer *uc)
 {
 	return -ENOSYS;
 }

kernel/events/uprobes.c +6 -18

index dfe6306a63b1..b7f40bad8abc 100644
@@ -1121,25 +1121,26 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consume
 EXPORT_SYMBOL_GPL(uprobe_unregister);
 
 /*
- * __uprobe_register - register a probe
+ * uprobe_register - register a probe
  * @inode: the file in which the probe has to be placed.
  * @offset: offset from the start of the file.
+ * @ref_ctr_offset: offset of SDT marker / reference counter
  * @uc: information on howto handle the probe..
  *
- * Apart from the access refcount, __uprobe_register() takes a creation
+ * Apart from the access refcount, uprobe_register() takes a creation
  * refcount (thro alloc_uprobe) if and only if this @uprobe is getting
  * inserted into the rbtree (i.e first consumer for a @inode:@offset
  * tuple).  Creation refcount stops uprobe_unregister from freeing the
  * @uprobe even before the register operation is complete. Creation
  * refcount is released when the last @uc for the @uprobe
- * unregisters. Caller of __uprobe_register() is required to keep @inode
+ * unregisters. Caller of uprobe_register() is required to keep @inode
  * (and the containing mount) referenced.
  *
  * Return errno if it cannot successully install probes
  * else return 0 (success)
  */
-static int __uprobe_register(struct inode *inode, loff_t offset,
-			     loff_t ref_ctr_offset, struct uprobe_consumer *uc)
+int uprobe_register(struct inode *inode, loff_t offset, loff_t ref_ctr_offset,
+		    struct uprobe_consumer *uc)
 {
 	struct uprobe *uprobe;
 	int ret;

@@ -1189,21 +1190,8 @@ static int __uprobe_register(struct inode *inode, loff_t offset,
 		goto retry;
 	return ret;
 }
-
-int uprobe_register(struct inode *inode, loff_t offset,
-		    struct uprobe_consumer *uc)
-{
-	return __uprobe_register(inode, offset, 0, uc);
-}
 EXPORT_SYMBOL_GPL(uprobe_register);
 
-int uprobe_register_refctr(struct inode *inode, loff_t offset,
-			   loff_t ref_ctr_offset, struct uprobe_consumer *uc)
-{
-	return __uprobe_register(inode, offset, ref_ctr_offset, uc);
-}
-EXPORT_SYMBOL_GPL(uprobe_register_refctr);
-
 /*
  * uprobe_apply - unregister an already registered probe.
  * @inode: the file in which the probe has to be removed.

kernel/trace/bpf_trace.c +4 -4

index cd098846e251..afa909e17824 100644
@@ -3480,10 +3480,10 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 		      &bpf_uprobe_multi_link_lops, prog);
 
 	for (i = 0; i < cnt; i++) {
-		err = uprobe_register_refctr(d_real_inode(link->path.dentry),
-					     uprobes[i].offset,
-					     uprobes[i].ref_ctr_offset,
-					     &uprobes[i].consumer);
+		err = uprobe_register(d_real_inode(link->path.dentry),
+				      uprobes[i].offset,
+				      uprobes[i].ref_ctr_offset,
+				      &uprobes[i].consumer);
 		if (err) {
 			bpf_uprobe_unregister(&path, uprobes, i);
 			goto error_free;

kernel/trace/trace_uprobe.c +1 -6

index c98e3b3386ba..1f590f989c1e 100644
@@ -1089,12 +1089,7 @@ static int trace_uprobe_enable(struct trace_uprobe *tu, filter_func_t filter)
 	tu->consumer.filter = filter;
 	tu->inode = d_real_inode(tu->path.dentry);
 
-	if (tu->ref_ctr_offset)
-		ret = uprobe_register_refctr(tu->inode, tu->offset,
-				tu->ref_ctr_offset, &tu->consumer);
-	else
-		ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
-
+	ret = uprobe_register(tu->inode, tu->offset, tu->ref_ctr_offset, &tu->consumer);
 	if (ret)
 		tu->inode = NULL;
 

Signature · 1 line

2.25.1.362.g51ebf55

This way uprobe_unregister() and uprobe_apply() can use "struct uprobe *" rather than inode + offset. This simplifies the code and allows to avoid the unnecessary find_uprobe() + put_uprobe() in these functions.

TODO: uprobe_unregister() still needs get_uprobe/put_uprobe to ensure that this uprobe can't be freed before up_write(&uprobe->register_rwsem).

Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Andrii Nakryiko <[email protected]>

---
 include/linux/uprobes.h     | 15 +++++-----
 kernel/events/uprobes.c     | 56 +++++++++++++++----------------------
 kernel/trace/bpf_trace.c    | 25 ++++++++---------
 kernel/trace/trace_uprobe.c | 26 ++++++++---------
 4 files changed, 55 insertions(+), 67 deletions(-)

include/linux/uprobes.h +8 -7

index 440316fbf3c6..137ddfc0b2f8 100644
@@ -16,6 +16,7 @@
 #include <linux/types.h>
 #include <linux/wait.h>
 
+struct uprobe;
 struct vm_area_struct;
 struct mm_struct;
 struct inode;

@@ -110,9 +111,9 @@ extern bool is_trap_insn(uprobe_opcode_t *insn);
 extern unsigned long uprobe_get_swbp_addr(struct pt_regs *regs);
 extern unsigned long uprobe_get_trap_addr(struct pt_regs *regs);
 extern int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_t);
-extern int uprobe_register(struct inode *inode, loff_t offset, loff_t ref_ctr_offset, struct uprobe_consumer *uc);
-extern int uprobe_apply(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, bool);
-extern void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
+extern struct uprobe *uprobe_register(struct inode *inode, loff_t offset, loff_t ref_ctr_offset, struct uprobe_consumer *uc);
+extern int uprobe_apply(struct uprobe *uprobe, struct uprobe_consumer *uc, bool);
+extern void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc);
 extern int uprobe_mmap(struct vm_area_struct *vma);
 extern void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned long end);
 extern void uprobe_start_dup_mmap(void);

@@ -150,18 +151,18 @@ static inline void uprobes_init(void)
 
 #define uprobe_get_trap_addr(regs)	instruction_pointer(regs)
 
-static inline int
+static inline struct uprobe *
 uprobe_register(struct inode *inode, loff_t offset, loff_t ref_ctr_offset, struct uprobe_consumer *uc)
 {
-	return -ENOSYS;
+	return ERR_PTR(-ENOSYS);
 }
 static inline int
-uprobe_apply(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, bool add)
+uprobe_apply(struct uprobe* uprobe, struct uprobe_consumer *uc, bool add)
 {
 	return -ENOSYS;
 }
 static inline void
-uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc)
+uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
 {
 }
 static inline int uprobe_mmap(struct vm_area_struct *vma)

kernel/events/uprobes.c +22 -34

index b7f40bad8abc..974474680820 100644
@@ -1099,20 +1099,14 @@ __uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
 		delete_uprobe(uprobe);
 }
 
-/*
+/**
  * uprobe_unregister - unregister an already registered probe.
- * @inode: the file in which the probe has to be removed.
- * @offset: offset from the start of the file.
+ * @uprobe: uprobe to remove
  * @uc: identify which probe if multiple probes are colocated.
  */
-void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc)
+void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
 {
-	struct uprobe *uprobe;
-
-	uprobe = find_uprobe(inode, offset);
-	if (WARN_ON(!uprobe))
-		return;
-
+	get_uprobe(uprobe);
 	down_write(&uprobe->register_rwsem);
 	__uprobe_unregister(uprobe, uc);
 	up_write(&uprobe->register_rwsem);

@@ -1120,7 +1114,7 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consume
 }
 EXPORT_SYMBOL_GPL(uprobe_unregister);
 
-/*
+/**
  * uprobe_register - register a probe
  * @inode: the file in which the probe has to be placed.
  * @offset: offset from the start of the file.

@@ -1136,40 +1130,40 @@ EXPORT_SYMBOL_GPL(uprobe_unregister);
  * unregisters. Caller of uprobe_register() is required to keep @inode
  * (and the containing mount) referenced.
  *
- * Return errno if it cannot successully install probes
- * else return 0 (success)
+ * Return: pointer to the new uprobe on success or an ERR_PTR on failure.
  */
-int uprobe_register(struct inode *inode, loff_t offset, loff_t ref_ctr_offset,
-		    struct uprobe_consumer *uc)
+struct uprobe *uprobe_register(struct inode *inode,
+				loff_t offset, loff_t ref_ctr_offset,
+				struct uprobe_consumer *uc)
 {
 	struct uprobe *uprobe;
 	int ret;
 
 	/* Uprobe must have at least one set consumer */
 	if (!uc->handler && !uc->ret_handler)
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 
 	/* copy_insn() uses read_mapping_page() or shmem_read_mapping_page() */
 	if (!inode->i_mapping->a_ops->read_folio &&
 	    !shmem_mapping(inode->i_mapping))
-		return -EIO;
+		return ERR_PTR(-EIO);
 	/* Racy, just to catch the obvious mistakes */
 	if (offset > i_size_read(inode))
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 
 	/*
 	 * This ensures that copy_from_page(), copy_to_page() and
 	 * __update_ref_ctr() can't cross page boundary.
 	 */
 	if (!IS_ALIGNED(offset, UPROBE_SWBP_INSN_SIZE))
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 	if (!IS_ALIGNED(ref_ctr_offset, sizeof(short)))
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 
  retry:
 	uprobe = alloc_uprobe(inode, offset, ref_ctr_offset);
 	if (IS_ERR(uprobe))
-		return PTR_ERR(uprobe);
+		return uprobe;
 
 	/*
 	 * We can race with uprobe_unregister()->delete_uprobe().

@@ -1188,35 +1182,29 @@ int uprobe_register(struct inode *inode, loff_t offset, loff_t ref_ctr_offset,
 
 	if (unlikely(ret == -EAGAIN))
 		goto retry;
-	return ret;
+
+	return ret ? ERR_PTR(ret) : uprobe;
 }
 EXPORT_SYMBOL_GPL(uprobe_register);
 
-/*
- * uprobe_apply - unregister an already registered probe.
- * @inode: the file in which the probe has to be removed.
- * @offset: offset from the start of the file.
+/**
+ * uprobe_apply - add or remove the breakpoints according to @uc->filter
+ * @uprobe: uprobe which "owns" the breakpoint
  * @uc: consumer which wants to add more or remove some breakpoints
  * @add: add or remove the breakpoints
+ * Return: 0 on success or negative error code.
  */
-int uprobe_apply(struct inode *inode, loff_t offset,
-			struct uprobe_consumer *uc, bool add)
+int uprobe_apply(struct uprobe *uprobe, struct uprobe_consumer *uc, bool add)
 {
-	struct uprobe *uprobe;
 	struct uprobe_consumer *con;
 	int ret = -ENOENT;
 
-	uprobe = find_uprobe(inode, offset);
-	if (WARN_ON(!uprobe))
-		return ret;
-
 	down_write(&uprobe->register_rwsem);
 	for (con = uprobe->consumers; con && con != uc ; con = con->next)
 		;
 	if (con)
 		ret = register_for_each_vma(uprobe, add ? uc : NULL);
 	up_write(&uprobe->register_rwsem);
-	put_uprobe(uprobe);
 
 	return ret;
 }

kernel/trace/bpf_trace.c +12 -13

index afa909e17824..4e391daafa64 100644
@@ -3160,6 +3160,7 @@ struct bpf_uprobe {
 	loff_t offset;
 	unsigned long ref_ctr_offset;
 	u64 cookie;
+	struct uprobe *uprobe;
 	struct uprobe_consumer consumer;
 };
 

@@ -3178,15 +3179,12 @@ struct bpf_uprobe_multi_run_ctx {
 	struct bpf_uprobe *uprobe;
 };
 
-static void bpf_uprobe_unregister(struct path *path, struct bpf_uprobe *uprobes,
-				  u32 cnt)
+static void bpf_uprobe_unregister(struct bpf_uprobe *uprobes, u32 cnt)
 {
 	u32 i;
 
-	for (i = 0; i < cnt; i++) {
-		uprobe_unregister(d_real_inode(path->dentry), uprobes[i].offset,
-				  &uprobes[i].consumer);
-	}
+	for (i = 0; i < cnt; i++)
+		uprobe_unregister(uprobes[i].uprobe, &uprobes[i].consumer);
 }
 
 static void bpf_uprobe_multi_link_release(struct bpf_link *link)

@@ -3194,7 +3192,7 @@ static void bpf_uprobe_multi_link_release(struct bpf_link *link)
 	struct bpf_uprobe_multi_link *umulti_link;
 
 	umulti_link = container_of(link, struct bpf_uprobe_multi_link, link);
-	bpf_uprobe_unregister(&umulti_link->path, umulti_link->uprobes, umulti_link->cnt);
+	bpf_uprobe_unregister(umulti_link->uprobes, umulti_link->cnt);
 	if (umulti_link->task)
 		put_task_struct(umulti_link->task);
 	path_put(&umulti_link->path);

@@ -3480,12 +3478,13 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 		      &bpf_uprobe_multi_link_lops, prog);
 
 	for (i = 0; i < cnt; i++) {
-		err = uprobe_register(d_real_inode(link->path.dentry),
-				      uprobes[i].offset,
-				      uprobes[i].ref_ctr_offset,
-				      &uprobes[i].consumer);
-		if (err) {
-			bpf_uprobe_unregister(&path, uprobes, i);
+		uprobes[i].uprobe = uprobe_register(d_real_inode(link->path.dentry),
+						    uprobes[i].offset,
+						    uprobes[i].ref_ctr_offset,
+						    &uprobes[i].consumer);
+		if (IS_ERR(uprobes[i].uprobe)) {
+			err = PTR_ERR(uprobes[i].uprobe);
+			bpf_uprobe_unregister(uprobes, i);
 			goto error_free;
 		}
 	}

kernel/trace/trace_uprobe.c +13 -13

index 1f590f989c1e..52e76a73fa7c 100644
@@ -58,8 +58,8 @@ struct trace_uprobe {
 	struct dyn_event		devent;
 	struct uprobe_consumer		consumer;
 	struct path			path;
-	struct inode			*inode;
 	char				*filename;
+	struct uprobe			*uprobe;
 	unsigned long			offset;
 	unsigned long			ref_ctr_offset;
 	unsigned long			nhit;

@@ -1084,16 +1084,16 @@ typedef bool (*filter_func_t)(struct uprobe_consumer *self,
 
 static int trace_uprobe_enable(struct trace_uprobe *tu, filter_func_t filter)
 {
-	int ret;
+	struct inode *inode = d_real_inode(tu->path.dentry);
+	struct uprobe *uprobe;
 
 	tu->consumer.filter = filter;
-	tu->inode = d_real_inode(tu->path.dentry);
-
-	ret = uprobe_register(tu->inode, tu->offset, tu->ref_ctr_offset, &tu->consumer);
-	if (ret)
-		tu->inode = NULL;
+	uprobe = uprobe_register(inode, tu->offset, tu->ref_ctr_offset, &tu->consumer);
+	if (IS_ERR(uprobe))
+		return PTR_ERR(uprobe);
 
-	return ret;
+	tu->uprobe = uprobe;
+	return 0;
 }
 
 static void __probe_event_disable(struct trace_probe *tp)

@@ -1104,11 +1104,11 @@ static void __probe_event_disable(struct trace_probe *tp)
 	WARN_ON(!uprobe_filter_is_empty(tu->tp.event->filter));
 
 	list_for_each_entry(tu, trace_probe_probe_list(tp), tp.list) {
-		if (!tu->inode)
+		if (!tu->uprobe)
 			continue;
 
-		uprobe_unregister(tu->inode, tu->offset, &tu->consumer);
-		tu->inode = NULL;
+		uprobe_unregister(tu->uprobe, &tu->consumer);
+		tu->uprobe = NULL;
 	}
 }
 

@@ -1305,7 +1305,7 @@ static int uprobe_perf_close(struct trace_event_call *call,
 		return 0;
 
 	list_for_each_entry(tu, trace_probe_probe_list(tp), tp.list) {
-		ret = uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
+		ret = uprobe_apply(tu->uprobe, &tu->consumer, false);
 		if (ret)
 			break;
 	}

@@ -1329,7 +1329,7 @@ static int uprobe_perf_open(struct trace_event_call *call,
 		return 0;
 
 	list_for_each_entry(tu, trace_probe_probe_list(tp), tp.list) {
-		err = uprobe_apply(tu->inode, tu->offset, &tu->consumer, true);
+		err = uprobe_apply(tu->uprobe, &tu->consumer, true);
 		if (err) {
 			uprobe_perf_close(call, event);
 			break;

Signature · 1 line

2.25.1.362.g51ebf55

Quoted 4 lines · On Mon, 29 Jul 2024 15:45:35 +0200 Oleg Nesterov <[email protected]> wrote:

This way uprobe_unregister() and uprobe_apply() can use "struct uprobe *" rather than inode + offset. This simplifies the code and allows to avoid the unnecessary find_uprobe() + put_uprobe() in these functions.

Quoted 2 lines

TODO: uprobe_unregister() still needs get_uprobe/put_uprobe to ensure that this uprobe can't be freed before up_write(&uprobe->register_rwsem).

Is this TODO item, or just a note? At this moment, this is natural to use get_uprobe() to protect uprobe itself.

OK, but it seems

tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c

is still using uprobe_register_refctr().

That should be updated too.

This way uprobe_unregister() and uprobe_apply() can use "struct uprobe *" rather than inode + offset. This simplifies the code and allows to avoid the unnecessary find_uprobe() + put_uprobe() in these functions.

TODO: uprobe_unregister() still needs get_uprobe/put_uprobe to ensure that this uprobe can't be freed before up_write(&uprobe->register_rwsem).

Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Andrii Nakryiko <[email protected]>

---
 include/linux/uprobes.h     | 15 +++++-----
 kernel/events/uprobes.c     | 56 +++++++++++++++----------------------
 kernel/trace/bpf_trace.c    | 25 ++++++++---------
 kernel/trace/trace_uprobe.c | 26 ++++++++---------
 4 files changed, 55 insertions(+), 67 deletions(-)

Quoted 8 lines · Oleg Nesterov

This way uprobe_unregister() and uprobe_apply() can use "struct uprobe *" rather than inode + offset. This simplifies the code and allows to avoid the unnecessary find_uprobe() + put_uprobe() in these functions.

TODO: uprobe_unregister() still needs get_uprobe/put_uprobe to ensure that this uprobe can't be freed before up_write(&uprobe->register_rwsem).

Quoted 9 lines

Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Andrii Nakryiko <[email protected]>

---
 include/linux/uprobes.h     | 15 +++++-----
 kernel/events/uprobes.c     | 56 +++++++++++++++----------------------
 kernel/trace/bpf_trace.c    | 25 ++++++++---------
 kernel/trace/trace_uprobe.c | 26 ++++++++---------
 4 files changed, 55 insertions(+), 67 deletions(-)

You'll need something like below to not break our bpf_testmod. And please send pull patch sets, not individually updated patches, it's a PITA to deal with. Thanks!

commit 9f739a9997ab833394196459fa7e6dd4d13dd48b (HEAD -> uprobes-oleg-cleanups)

Author: Andrii Nakryiko <[email protected]>
Date:   Wed Jul 31 09:15:46 2024 -0700

uprobes: fix bpf_testmod after uprobe_register/uprobe_unregister API change

Signed-off-by Andrii Nakryiko <[email protected]>

tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +1 -0

index 5f152afdec2f..73a6b041bcce 100644
@@ -431,6 +431,7 @@ uprobe_ret_handler(struct uprobe_consumer *self,
 unsigned long func,
 }
 
 struct testmod_uprobe {
+       struct uprobe *uprobe;
        struct path path;
        loff_t offset;

        struct uprobe_consumer consumer;

tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +9 -7

@@ -458,12 +459,14 @@ static int testmod_register_uprobe(loff_t offset)
        if (err)
                goto out;
 
-       err = uprobe_register(d_real_inode(uprobe.path.dentry),
-                             offset, 0, &uprobe.consumer);
-       if (err)
+       uprobe.uprobe = uprobe_register(d_real_inode(uprobe.path.dentry),
+                                       offset, 0, &uprobe.consumer);
+       if (IS_ERR(uprobe.uprobe)) {
                path_put(&uprobe.path);
-       else
+               uprobe.uprobe = NULL;
+       } else {
                uprobe.offset = offset;
+       }
 
 out:
        mutex_unlock(&testmod_uprobe_mutex);

@@ -474,10 +477,10 @@ static void testmod_unregister_uprobe(void)
 {
        mutex_lock(&testmod_uprobe_mutex);
 
-       if (uprobe.offset) {
-               uprobe_unregister(d_real_inode(uprobe.path.dentry),
-                                 uprobe.offset, &uprobe.consumer);
+       if (uprobe.uprobe) {
+               uprobe_unregister(uprobe.uprobe, &uprobe.consumer);
                uprobe.offset = 0;
+               uprobe.uprobe = NULL;
        }
 
        mutex_unlock(&testmod_uprobe_mutex);
 
 
[...]

Quoted 19 lines · Andrii Nakryiko

Quoted 17 lines · Oleg Nesterov

This way uprobe_unregister() and uprobe_apply() can use "struct uprobe *" rather than inode + offset. This simplifies the code and allows to avoid the unnecessary find_uprobe() + put_uprobe() in these functions.

TODO: uprobe_unregister() still needs get_uprobe/put_uprobe to ensure that this uprobe can't be freed before up_write(&uprobe->register_rwsem).

Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Andrii Nakryiko <[email protected]>

---
 include/linux/uprobes.h     | 15 +++++-----
 kernel/events/uprobes.c     | 56 +++++++++++++++----------------------
 kernel/trace/bpf_trace.c    | 25 ++++++++---------
 kernel/trace/trace_uprobe.c | 26 ++++++++---------
 4 files changed, 55 insertions(+), 67 deletions(-)

Quoted 3 lines

You'll need something like below to not break our bpf_testmod. And please send pull patch sets, not individually updated patches, it's a PITA to deal with. Thanks!

Do I stuff this on top of Oleg's patch or do you want me to fold it in one of them?

Quoted 59 lines

commit 9f739a9997ab833394196459fa7e6dd4d13dd48b (HEAD -> uprobes-oleg-cleanups)

Author: Andrii Nakryiko <[email protected]>
Date:   Wed Jul 31 09:15:46 2024 -0700

uprobes: fix bpf_testmod after uprobe_register/uprobe_unregister API change

Signed-off-by Andrii Nakryiko <[email protected]>

tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +1 -0

index 5f152afdec2f..73a6b041bcce 100644
@@ -431,6 +431,7 @@ uprobe_ret_handler(struct uprobe_consumer *self,
 unsigned long func,
 }
 
 struct testmod_uprobe {
+       struct uprobe *uprobe;
        struct path path;
        loff_t offset;

        struct uprobe_consumer consumer;

tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +9 -7

@@ -458,12 +459,14 @@ static int testmod_register_uprobe(loff_t offset)
        if (err)
                goto out;
 
-       err = uprobe_register(d_real_inode(uprobe.path.dentry),
-                             offset, 0, &uprobe.consumer);
-       if (err)
+       uprobe.uprobe = uprobe_register(d_real_inode(uprobe.path.dentry),
+                                       offset, 0, &uprobe.consumer);
+       if (IS_ERR(uprobe.uprobe)) {
                path_put(&uprobe.path);
-       else
+               uprobe.uprobe = NULL;
+       } else {
                uprobe.offset = offset;
+       }
 
 out:
        mutex_unlock(&testmod_uprobe_mutex);

@@ -474,10 +477,10 @@ static void testmod_unregister_uprobe(void)
 {
        mutex_lock(&testmod_uprobe_mutex);
 
-       if (uprobe.offset) {
-               uprobe_unregister(d_real_inode(uprobe.path.dentry),
-                                 uprobe.offset, &uprobe.consumer);
+       if (uprobe.uprobe) {
+               uprobe_unregister(uprobe.uprobe, &uprobe.consumer);
                uprobe.offset = 0;
+               uprobe.uprobe = NULL;
        }
 
        mutex_unlock(&testmod_uprobe_mutex);
 
 
[...]

Quoted 25 lines · Peter Zijlstra

Quoted 22 lines · Andrii Nakryiko

Quoted 8 lines · Oleg Nesterov

This way uprobe_unregister() and uprobe_apply() can use "struct uprobe *" rather than inode + offset. This simplifies the code and allows to avoid the unnecessary find_uprobe() + put_uprobe() in these functions.

TODO: uprobe_unregister() still needs get_uprobe/put_uprobe to ensure that this uprobe can't be freed before up_write(&uprobe->register_rwsem).

Quoted 9 lines

Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Andrii Nakryiko <[email protected]>

---
 include/linux/uprobes.h     | 15 +++++-----
 kernel/events/uprobes.c     | 56 +++++++++++++++----------------------
 kernel/trace/bpf_trace.c    | 25 ++++++++---------
 kernel/trace/trace_uprobe.c | 26 ++++++++---------
 4 files changed, 55 insertions(+), 67 deletions(-)

You'll need something like below to not break our bpf_testmod. And please send pull patch sets, not individually updated patches, it's a PITA to deal with. Thanks!

Quoted 2 lines

Do I stuff this on top of Oleg's patch or do you want me to fold it in one of them?

Please fold so we have better (potential) bisectability of BPF selftests, thanks!

Quoted 26 lines · Andrii Nakryiko

Quoted 25 lines · Peter Zijlstra

Quoted 22 lines · Andrii Nakryiko

Quoted 8 lines · Oleg Nesterov

This way uprobe_unregister() and uprobe_apply() can use "struct uprobe *" rather than inode + offset. This simplifies the code and allows to avoid the unnecessary find_uprobe() + put_uprobe() in these functions.

TODO: uprobe_unregister() still needs get_uprobe/put_uprobe to ensure that this uprobe can't be freed before up_write(&uprobe->register_rwsem).

Quoted 9 lines

Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Andrii Nakryiko <[email protected]>

---
 include/linux/uprobes.h     | 15 +++++-----
 kernel/events/uprobes.c     | 56 +++++++++++++++----------------------
 kernel/trace/bpf_trace.c    | 25 ++++++++---------
 kernel/trace/trace_uprobe.c | 26 ++++++++---------
 4 files changed, 55 insertions(+), 67 deletions(-)

You'll need something like below to not break our bpf_testmod. And please send pull patch sets, not individually updated patches, it's a PITA to deal with. Thanks!

Quoted 5 lines

Quoted 2 lines

Do I stuff this on top of Oleg's patch or do you want me to fold it in one of them?

Please fold so we have better (potential) bisectability of BPF selftests, thanks!

Fold where, patch 5?

Quoted 5 lines · Andrii Nakryiko

Quoted 2 lines

Do I stuff this on top of Oleg's patch or do you want me to fold it in one of them?

Please fold so we have better (potential) bisectability of BPF selftests, thanks!

Fold where, patch 5?

My expectation was that Oleg will
just incorporate that by hand and will send the final v4 patch set.

tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +1 -0

index 5f152afdec2f..73a6b041bcce 100644
@@ -431,6 +431,7 @@ uprobe_ret_handler(struct uprobe_consumer *self,
 unsigned long func,
 }
 
 struct testmod_uprobe {
+       struct uprobe *uprobe;
        struct path path;
        loff_t offset;

        struct uprobe_consumer consumer;

tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +6 -4

@@ -458,12 +459,14 @@ static int testmod_register_uprobe(loff_t offset)
        if (err)
                goto out;
 
-       err = uprobe_register(d_real_inode(uprobe.path.dentry),
-                             offset, 0, &uprobe.consumer);
-       if (err)
+       uprobe.uprobe = uprobe_register(d_real_inode(uprobe.path.dentry),
+                                       offset, 0, &uprobe.consumer);
+       if (IS_ERR(uprobe.uprobe)) {
                path_put(&uprobe.path);
-       else
+               uprobe.uprobe = NULL;
+       } else {
                uprobe.offset = offset;
+       }
 
 out:
        mutex_unlock(&testmod_uprobe_mutex);

tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +0 -0

        }
 
        mutex_unlock(&testmod_uprobe_mutex);
 
 
[...]

Quoted 5 lines

(quoted diff) +0 -0

@@ -474,10 +477,10 @@ static void testmod_unregister_uprobe(void)
 {
        mutex_lock(&testmod_uprobe_mutex);
 

Review static void testmod_unregister_uprobe(void) · line 483

Quoted 7 lines

(quoted diff) +3 -3

-       if (uprobe.offset) {
-               uprobe_unregister(d_real_inode(uprobe.path.dentry),
-                                 uprobe.offset, &uprobe.consumer);
+       if (uprobe.uprobe) {
+               uprobe_unregister(uprobe.uprobe, &uprobe.consumer);
                uprobe.offset = 0;
+               uprobe.uprobe = NULL;

ugh, I think we leak &uprobe.path.. I can send follow up fix if needed

Quoted 5 lines · Jiri Olsa

Quoted 5 lines

tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +0 -0

@@ -474,10 +477,10 @@ static void testmod_unregister_uprobe(void)
 {
        mutex_lock(&testmod_uprobe_mutex);
 

Quoted 9 lines

tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c static void testmod_unregister_uprobe(void) · line 480

Quoted 7 lines

tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +3 -3

-       if (uprobe.offset) {
-               uprobe_unregister(d_real_inode(uprobe.path.dentry),
-                                 uprobe.offset, &uprobe.consumer);
+       if (uprobe.uprobe) {
+               uprobe_unregister(uprobe.uprobe, &uprobe.consumer);
                uprobe.offset = 0;
+               uprobe.uprobe = NULL;

ugh, I think we leak &uprobe.path.. I can send follow up fix if needed

Yeah, with or without this change. And with this change we do not need uprobe.offset.

Please see the patch below, this is what I've added to 5/5.

Do you see any problems?

Note the additional path_put() in testmod_unregister_uprobe(). Does it need a separate patch or can it come with 5/5 ?

Oleg.

tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +13 -13

@@ -432,7 +432,7 @@ uprobe_ret_handler(struct uprobe_consumer *self, unsigned long func,
 
 struct testmod_uprobe {
 	struct path path;
-	loff_t offset;
+	struct uprobe *uprobe;
 	struct uprobe_consumer consumer;
 };
 

@@ -446,25 +446,25 @@ static int testmod_register_uprobe(loff_t offset)
 {
 	int err = -EBUSY;
 
-	if (uprobe.offset)
+	if (uprobe.uprobe)
 		return -EBUSY;
 
 	mutex_lock(&testmod_uprobe_mutex);
 
-	if (uprobe.offset)
+	if (uprobe.uprobe)
 		goto out;
 
 	err = kern_path("/proc/self/exe", LOOKUP_FOLLOW, &uprobe.path);
 	if (err)
 		goto out;
 
-	err = uprobe_register(d_real_inode(uprobe.path.dentry),
-			      offset, 0, &uprobe.consumer);
-	if (err)
+	uprobe.uprobe = uprobe_register(d_real_inode(uprobe.path.dentry),
+					offset, 0, &uprobe.consumer);
+	if (IS_ERR(uprobe.uprobe)) {
+		err = PTR_ERR(uprobe.uprobe);
 		path_put(&uprobe.path);
-	else
-		uprobe.offset = offset;
-
+		uprobe.uprobe = NULL;
+	}
 out:
 	mutex_unlock(&testmod_uprobe_mutex);
 	return err;

@@ -474,10 +474,10 @@ static void testmod_unregister_uprobe(void)
 {
 	mutex_lock(&testmod_uprobe_mutex);
 
-	if (uprobe.offset) {
-		uprobe_unregister(d_real_inode(uprobe.path.dentry),
-				  uprobe.offset, &uprobe.consumer);
-		uprobe.offset = 0;
+	if (uprobe.uprobe) {
+		uprobe_unregister(uprobe.uprobe, &uprobe.consumer);
+		path_put(&uprobe.path);
+		uprobe.uprobe = NULL;
 	}
 
 	mutex_unlock(&testmod_uprobe_mutex);

Quoted 3 lines

Note the additional path_put() in testmod_unregister_uprobe(). Does it need a separate patch or can it come with 5/5 ?

I think it'd be better to have it separately, the test is already released.. so people might want to backport just the fix

Quoted 6 lines · Jiri Olsa

Quoted 3 lines

Note the additional path_put() in testmod_unregister_uprobe(). Does it need a separate patch or can it come with 5/5 ?

I think it'd be better to have it separately, the test is already released.. so people might want to backport just the fix

OK, I'll rebase and add the patch below to v4. OK?

Oleg.
---

From f6bf42015048938d826880e3bf4a318bb64a05b4 Mon Sep 17 00:00:00 2001
From: Oleg Nesterov <[email protected]>
Date: Thu, 1 Aug 2024 14:21:47 +0200
Subject: [PATCH] selftests/bpf: fix uprobe.path leak in bpf_testmod

From: Jiri Olsa <[email protected]>

testmod_unregister_uprobe() forgets to path_put(&uprobe.path).

Signed-off-by Jiri Olsa <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

---
 tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c | 1 +
 1 file changed, 1 insertion(+)

tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +1 -0

index 86babdd6f850..55f6905de743 100644
@@ -477,6 +477,7 @@ static void testmod_unregister_uprobe(void)
 	if (uprobe.offset) {
 		uprobe_unregister(d_real_inode(uprobe.path.dentry),
 				  uprobe.offset, &uprobe.consumer);
+		path_put(&uprobe.path);
 		uprobe.offset = 0;
 	}
 

Signature · 1 line

2.25.1.362.g51ebf55

Quoted 8 lines · Peter Zijlstra

Quoted 5 lines · Andrii Nakryiko

Quoted 2 lines

Do I stuff this on top of Oleg's patch or do you want me to fold it in one of them?

Please fold so we have better (potential) bisectability of BPF selftests, thanks!

Quoted 1 line

Fold where, patch 5?

Yes...

Or I can resend these 2 series as v3 1-8 with 5/5 updated (thanks Andrii !!!), whatever is more convenient for you.