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.g51ebf55On 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);
[...]
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);
>
>
> [...]
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);
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);
>
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
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
>
>
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);
>
>
> [...]
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);
> >
> >
> > [...]
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?
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.
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.
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.
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.
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>
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.
© 2016 - 2026 Red Hat, Inc.