[PATCH v2 0/5] uprobes: misc cleanups/simplifications

[PATCH v2 0/5] uprobes: misc cleanups/simplifications

[Oleg Nesterov]

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(-)

[PATCH v2 1/5] uprobes: document the usage of mm->mmap_lock

[Oleg Nesterov]

The comment above uprobe_write_opcode() is wrong, unapply_uprobe() calls
it under mmap_read_lock() and this is correct.

And it is completely unclear why register_for_each_vma() takes mmap_lock
for writing, add a comment to explain that mmap_write_lock() is needed to
avoid the following race:

	- A task T hits the bp installed by uprobe and calls
	  find_active_uprobe()

	- uprobe_unregister() removes this uprobe/bp

	- T calls find_uprobe() which returns NULL

	- another uprobe_register() installs the bp at the same address

	- T calls is_trap_at_addr() which returns true

	- T returns to handle_swbp() and gets SIGTRAP.

Reported-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 73cc47708679..73dd12b09a7b 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -453,7 +453,7 @@ static int update_ref_ctr(struct uprobe *uprobe, struct mm_struct *mm,
  * @vaddr: the virtual address to store the opcode.
  * @opcode: opcode to be written at @vaddr.
  *
- * Called with mm->mmap_lock held for write.
+ * Called with mm->mmap_lock held for read or write.
  * Return 0 (success) or a negative errno.
  */
 int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
@@ -1046,7 +1046,13 @@ register_for_each_vma(struct uprobe *uprobe, struct uprobe_consumer *new)
 
 		if (err && is_register)
 			goto free;
-
+		/*
+		 * We take mmap_lock for writing to avoid the race with
+		 * find_active_uprobe() which takes mmap_lock for reading.
+		 * Thus this install_breakpoint() can not make
+		 * is_trap_at_addr() true right after find_uprobe()
+		 * returns NULL in find_active_uprobe().
+		 */
 		mmap_write_lock(mm);
 		vma = find_vma(mm, info->vaddr);
 		if (!vma || !valid_vma(vma, is_register) ||
-- 
2.25.1.362.g51ebf55

[PATCH v2 2/5] uprobes: is_trap_at_addr: don't use get_user_pages_remote()

[Oleg Nesterov]

get_user_pages_remote() and the comment above it make no sense.

There is no task_struct passed into get_user_pages_remote() anymore, and
nowadays mm_account_fault() increments the current->min/maj_flt counters
regardless of FAULT_FLAG_REMOTE.

Reported-by: Andrii Nakryiko <andrii@kernel.org>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 73dd12b09a7b..eb71428691bb 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -2035,13 +2035,7 @@ static int is_trap_at_addr(struct mm_struct *mm, unsigned long vaddr)
 	if (likely(result == 0))
 		goto out;
 
-	/*
-	 * The NULL 'tsk' here ensures that any faults that occur here
-	 * will not be accounted to the task.  'mm' *is* current->mm,
-	 * but we treat this as a 'remote' access since it is
-	 * essentially a kernel access to the memory.
-	 */
-	result = get_user_pages_remote(mm, vaddr, 1, FOLL_FORCE, &page, NULL);
+	result = get_user_pages(vaddr, 1, FOLL_FORCE, &page);
 	if (result < 0)
 		return result;
 
-- 
2.25.1.362.g51ebf55

[PATCH v2 3/5] uprobes: simplify error handling for alloc_uprobe()

[Oleg Nesterov]

From: Andrii Nakryiko <andrii@kernel.org>

Return -ENOMEM instead of NULL, which makes caller's error handling just
a touch simpler.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index eb71428691bb..dfe6306a63b1 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -725,7 +725,7 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset,
 
 	uprobe = kzalloc(sizeof(struct uprobe), GFP_KERNEL);
 	if (!uprobe)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	uprobe->inode = inode;
 	uprobe->offset = offset;
@@ -1167,8 +1167,6 @@ static int __uprobe_register(struct inode *inode, loff_t offset,
 
  retry:
 	uprobe = alloc_uprobe(inode, offset, ref_ctr_offset);
-	if (!uprobe)
-		return -ENOMEM;
 	if (IS_ERR(uprobe))
 		return PTR_ERR(uprobe);
 
-- 
2.25.1.362.g51ebf55

[PATCH v2 4/5] uprobes: kill uprobe_register_refctr()

[Oleg Nesterov]

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 <oleg@redhat.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
 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(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index b503fafb7fb3..440316fbf3c6 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -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;
 }
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index dfe6306a63b1..b7f40bad8abc 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -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.
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index cd098846e251..afa909e17824 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -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;
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index c98e3b3386ba..1f590f989c1e 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -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;
 
-- 
2.25.1.362.g51ebf55

Re: [PATCH v2 4/5] uprobes: kill uprobe_register_refctr()

[Masami Hiramatsu]

On Mon, 29 Jul 2024 15:45:30 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> 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 <oleg@redhat.com>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  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(-)

OK, but it seems 

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

is still using uprobe_register_refctr().

That should be updated too.

Thanks,

> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index b503fafb7fb3..440316fbf3c6 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -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;
>  }
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index dfe6306a63b1..b7f40bad8abc 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -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.
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index cd098846e251..afa909e17824 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -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;
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index c98e3b3386ba..1f590f989c1e 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -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;
>  
> -- 
> 2.25.1.362.g51ebf55
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v2 4/5] uprobes: kill uprobe_register_refctr()

[Oleg Nesterov]

On 07/31, Masami Hiramatsu wrote:
>
> OK, but it seems
>
> tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>
> is still using uprobe_register_refctr().
>
> That should be updated too.

OOPS, thanks a lot :/

I'll send v3 with the additional change below in reply to 4/5 in a minute.

Masami, Peter, please let me know if you want me to resend the whole series.

Oleg.

--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -458,8 +458,8 @@ static int testmod_register_uprobe(loff_t offset)
 	if (err)
 		goto out;
 
-	err = uprobe_register_refctr(d_real_inode(uprobe.path.dentry),
-				     offset, 0, &uprobe.consumer);
+	err = uprobe_register(d_real_inode(uprobe.path.dentry),
+			      offset, 0, &uprobe.consumer);
 	if (err)
 		path_put(&uprobe.path);
 	else

[PATCH v3 4/5] uprobes: kill uprobe_register_refctr()

[Oleg Nesterov]

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 <oleg@redhat.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
 include/linux/uprobes.h                       |  9 ++-----
 kernel/events/uprobes.c                       | 24 +++++--------------
 kernel/trace/bpf_trace.c                      |  8 +++----
 kernel/trace/trace_uprobe.c                   |  7 +-----
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  4 ++--
 5 files changed, 15 insertions(+), 37 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index b503fafb7fb3..440316fbf3c6 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -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;
 }
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index dfe6306a63b1..b7f40bad8abc 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -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.
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index cd098846e251..afa909e17824 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -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;
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index c98e3b3386ba..1f590f989c1e 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -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;
 
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index fd28c1157bd3..86babdd6f850 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -458,8 +458,8 @@ static int testmod_register_uprobe(loff_t offset)
 	if (err)
 		goto out;
 
-	err = uprobe_register_refctr(d_real_inode(uprobe.path.dentry),
-				     offset, 0, &uprobe.consumer);
+	err = uprobe_register(d_real_inode(uprobe.path.dentry),
+			      offset, 0, &uprobe.consumer);
 	if (err)
 		path_put(&uprobe.path);
 	else
-- 
2.25.1.362.g51ebf55

[PATCH v2 5/5] uprobes: make uprobe_register() return struct uprobe *

[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 <oleg@redhat.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
 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(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 440316fbf3c6..137ddfc0b2f8 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -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)
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index b7f40bad8abc..974474680820 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -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;
 }
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index afa909e17824..4e391daafa64 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -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;
 		}
 	}
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 1f590f989c1e..52e76a73fa7c 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -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;
-- 
2.25.1.362.g51ebf55

Re: [PATCH v2 5/5] uprobes: make uprobe_register() return struct uprobe *

[Masami Hiramatsu]

On Mon, 29 Jul 2024 15:45:35 +0200
Oleg Nesterov <oleg@redhat.com> 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.
> 
> 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.

Thank you,

> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  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(-)
> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 440316fbf3c6..137ddfc0b2f8 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -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)
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index b7f40bad8abc..974474680820 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -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;
>  }
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index afa909e17824..4e391daafa64 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -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;
>  		}
>  	}
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 1f590f989c1e..52e76a73fa7c 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -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;
> -- 
> 2.25.1.362.g51ebf55
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v2 5/5] uprobes: make uprobe_register() return struct uprobe *

[Oleg Nesterov]

On 07/31, Masami Hiramatsu wrote:
>
> On Mon, 29 Jul 2024 15:45:35 +0200
> Oleg Nesterov <oleg@redhat.com> 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.
> >
> > 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.

3/3 from the next series removes the extra get_uprobe() + put_uprobe().

Initially the change said something like

	This patch adds the additional get_uprobe/put_uprobe into _register,
	the next patch will remove this.

But then decided to split this "next" patch and send it in another series.

Thanks,

Oleg.

Re: [PATCH v2 5/5] uprobes: make uprobe_register() return struct uprobe *

[Andrii Nakryiko]

On Mon, Jul 29, 2024 at 6:45 AM Oleg Nesterov <oleg@redhat.com> 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.
>
> 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 <oleg@redhat.com>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  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 <andrii@kernel.org>
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 <andrii@kernel.org>

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 5f152afdec2f..73a6b041bcce 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -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;
@@ -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);


[...]

Re: [PATCH v2 5/5] uprobes: make uprobe_register() return struct uprobe *

[Peter Zijlstra]

On Wed, Jul 31, 2024 at 09:18:00AM -0700, Andrii Nakryiko wrote:
> On Mon, Jul 29, 2024 at 6:45 AM Oleg Nesterov <oleg@redhat.com> 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.
> >
> > 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 <oleg@redhat.com>
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >  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!

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

> commit 9f739a9997ab833394196459fa7e6dd4d13dd48b (HEAD -> uprobes-oleg-cleanups)
> Author: Andrii Nakryiko <andrii@kernel.org>
> 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 <andrii@kernel.org>
> 
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> index 5f152afdec2f..73a6b041bcce 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -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;
> @@ -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);
> 
> 
> [...]

Re: [PATCH v2 5/5] uprobes: make uprobe_register() return struct uprobe *

[Andrii Nakryiko]

On Wed, Jul 31, 2024 at 9:56 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Jul 31, 2024 at 09:18:00AM -0700, Andrii Nakryiko wrote:
> > On Mon, Jul 29, 2024 at 6:45 AM Oleg Nesterov <oleg@redhat.com> 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.
> > >
> > > 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 <oleg@redhat.com>
> > > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > > ---
> > >  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!
>
> 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!

>
> > commit 9f739a9997ab833394196459fa7e6dd4d13dd48b (HEAD -> uprobes-oleg-cleanups)
> > Author: Andrii Nakryiko <andrii@kernel.org>
> > 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 <andrii@kernel.org>
> >
> > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > index 5f152afdec2f..73a6b041bcce 100644
> > --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> > @@ -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;
> > @@ -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);
> >
> >
> > [...]

Re: [PATCH v2 5/5] uprobes: make uprobe_register() return struct uprobe *

[Peter Zijlstra]

On Wed, Jul 31, 2024 at 10:01:47AM -0700, Andrii Nakryiko wrote:
> On Wed, Jul 31, 2024 at 9:56 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Wed, Jul 31, 2024 at 09:18:00AM -0700, Andrii Nakryiko wrote:
> > > On Mon, Jul 29, 2024 at 6:45 AM Oleg Nesterov <oleg@redhat.com> 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.
> > > >
> > > > 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 <oleg@redhat.com>
> > > > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > > > ---
> > > >  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!
> >
> > 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?

Re: [PATCH v2 5/5] uprobes: make uprobe_register() return struct uprobe *

[Andrii Nakryiko]

On Wed, Jul 31, 2024 at 10:05 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Jul 31, 2024 at 10:01:47AM -0700, Andrii Nakryiko wrote:
> > On Wed, Jul 31, 2024 at 9:56 AM Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > On Wed, Jul 31, 2024 at 09:18:00AM -0700, Andrii Nakryiko wrote:
> > > > On Mon, Jul 29, 2024 at 6:45 AM Oleg Nesterov <oleg@redhat.com> 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.
> > > > >
> > > > > 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 <oleg@redhat.com>
> > > > > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > > > > ---
> > > > >  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!
> > >
> > > 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?

Yep, this one, where Oleg changed uprobe_register/uprobe_unregister API.

But note, I did the lazy thing and just copy/pasted `git show` output
into Gmail. So all the whitespaces are butchered and unlikely you'll
be able to apply that patch as is. My expectation was that Oleg will
just incorporate that by hand and will send the final v4 patch set.

Re: [PATCH v2 5/5] uprobes: make uprobe_register() return struct uprobe *

[Oleg Nesterov]

On 07/31, Andrii Nakryiko wrote:
>
> My expectation was that Oleg will
> just incorporate that by hand and will send the final v4 patch set.

Will do tomorrow, thanks.

Oleg.

Re: [PATCH v2 5/5] uprobes: make uprobe_register() return struct uprobe *

[Oleg Nesterov]

On 07/31, Peter Zijlstra wrote:
>
> On Wed, Jul 31, 2024 at 10:01:47AM -0700, Andrii Nakryiko wrote:
> > > 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?

Yes...

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

Oleg.

Re: [PATCH v2 5/5] uprobes: make uprobe_register() return struct uprobe *

[Peter Zijlstra]

On Wed, Jul 31, 2024 at 07:17:33PM +0200, Oleg Nesterov wrote:
> On 07/31, Peter Zijlstra wrote:
> >
> > On Wed, Jul 31, 2024 at 10:01:47AM -0700, Andrii Nakryiko wrote:
> > > > 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?
> 
> Yes...
> 
> Or I can resend these 2 series as v3 1-8 with 5/5 updated (thanks Andrii !!!),
> whatever is more convenient for you.

I think I've lost the plot a little, so if you could resent a whole
series that'd probably be easiest.

Re: [PATCH v2 5/5] uprobes: make uprobe_register() return struct uprobe *

[Jiri Olsa]

On Wed, Jul 31, 2024 at 09:18:00AM -0700, Andrii Nakryiko wrote:

SNIP

> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> index 5f152afdec2f..73a6b041bcce 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -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;
> @@ -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;

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

jirka

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

Re: [PATCH v2 5/5] uprobes: make uprobe_register() return struct uprobe *

[Oleg Nesterov]

On 08/01, Jiri Olsa wrote:
>
> > @@ -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;
>
> 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.

--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -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);

Re: [PATCH v2 5/5] uprobes: make uprobe_register() return struct uprobe *

[Jiri Olsa]

On Thu, Aug 01, 2024 at 02:00:18PM +0200, Oleg Nesterov wrote:
> On 08/01, Jiri Olsa wrote:
> >
> > > @@ -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;
> >
> > 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?

looks good, thanks!

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

thanks,
jirka

> 
> Oleg.
> 
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -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);
> 

Re: [PATCH v2 5/5] uprobes: make uprobe_register() return struct uprobe *

[Oleg Nesterov]

On 08/01, Jiri Olsa wrote:
>
> > 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 <oleg@redhat.com>
Date: Thu, 1 Aug 2024 14:21:47 +0200
Subject: [PATCH] selftests/bpf: fix uprobe.path leak in bpf_testmod

From: Jiri Olsa <olsajiri@gmail.com>

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

Signed-off-by: Jiri Olsa <olsajiri@gmail.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 86babdd6f850..55f6905de743 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -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;
 	}
 
-- 
2.25.1.362.g51ebf55

Re: [PATCH v2 5/5] uprobes: make uprobe_register() return struct uprobe *

[Jiri Olsa]

On Thu, Aug 01, 2024 at 02:26:45PM +0200, Oleg Nesterov wrote:
> On 08/01, Jiri Olsa wrote:
> >
> > > 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?

heh, suggested-by would have been fine, but sure ;-)

thanks,
jirka

> 
> Oleg.
> ---
> 
> From f6bf42015048938d826880e3bf4a318bb64a05b4 Mon Sep 17 00:00:00 2001
> From: Oleg Nesterov <oleg@redhat.com>
> Date: Thu, 1 Aug 2024 14:21:47 +0200
> Subject: [PATCH] selftests/bpf: fix uprobe.path leak in bpf_testmod
> 
> From: Jiri Olsa <olsajiri@gmail.com>
> 
> testmod_unregister_uprobe() forgets to path_put(&uprobe.path).
> 
> Signed-off-by: Jiri Olsa <olsajiri@gmail.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> index 86babdd6f850..55f6905de743 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -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;
>  	}
>  
> -- 
> 2.25.1.362.g51ebf55
> 
> 

Re: [PATCH v2 0/5] uprobes: misc cleanups/simplifications

[Jiri Olsa]

On Mon, Jul 29, 2024 at 03:44:44PM +0200, Oleg Nesterov wrote:
> 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.

lgtm

Reviewed-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> 
> 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(-)
>