uprobes: misc cleanups/simplifications
Subsystems (9)
Oleg Nesterov
maintainerv4 cover uprobes: misc cleanups/simplifications
(Andrii, I'll try to look at your new series on Weekend).
Changes:
- added the acks I got from Andrii, Masami, and Jiri
- new 4/9 patch from Jiri, fixes the unrelated bug in bpf_testmod
- adapt tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c to the API changes in 5/9 an 6/9
see the interdiff below.
Oleg.
---
include/linux/uprobes.h | 22 ++-- kernel/events/uprobes.c | 137 +++++++++------------ kernel/trace/bpf_trace.c | 25 ++-- kernel/trace/trace_uprobe.c | 31 ++--- .../selftests/bpf/bpf_testmod/bpf_testmod.c | 26 ++-- 5 files changed, 103 insertions(+), 138 deletions(-)
-------------------------------------------------------------------------------
tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +13 -13
@@ -432,7 +432,7 @@ uprobe_ret_handler(struct uprobe_consumer *self, unsigned long func,struct testmod_uprobe {struct path path; -loff_t offset; +struct uprobe *uprobe;struct uprobe_consumer consumer; };
@@ -446,25 +446,25 @@ static int testmod_register_uprobe(loff_t offset) {int err = -EBUSY ; -if (uprobe.offset) +if (uprobe.uprobe)return -EBUSY ;mutex_lock (&testmod_uprobe_mutex); -if (uprobe.offset) +if (uprobe.uprobe) goto out; err =kern_path ("/proc/self/exe" ,LOOKUP_FOLLOW , &uprobe.path);if (err) goto out; - err =uprobe_register_refctr (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);
Oleg Nesterov
maintainerPATCH v4 1/9 uprobes: document the usage of mm->mmap_lock
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 <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>
Acked-by Andrii Nakryiko <[email protected]>
Reviewed-by Jiri Olsa <[email protected]>
--- kernel/events/uprobes.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
kernel/events/uprobes.c +8 -2
@@ -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) ||
Signature · 1 line
2.25.1.362.g51ebf55
[tip: perf/core] uprobes: document the usage of mm->mmap_lock
The following commit has been merged into the perf/core branch of tip:
The following commit has been merged into the perf/core branch of tip:
Commit-ID: 84455e6923c79a37812930787aaa141e82afe315 Gitweb: https://git.kernel.org/tip/84455e6923c79a37812930787aaa141e82afe315 Author: Oleg Nesterov <[email protected]> AuthorDate: Thu, 01 Aug 2024 15:27:09 +02:00 Committer: Peter Zijlstra <[email protected]> CommitterDate: Fri, 02 Aug 2024 11:30:30 +02:00
uprobes: document the usage of mm->mmap_lock
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 <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Reviewed-by Jiri Olsa <[email protected]>
Acked-by Andrii Nakryiko <[email protected]>
--- kernel/events/uprobes.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
kernel/events/uprobes.c +8 -2
@@ -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) ||
Oleg Nesterov
maintainerPATCH v4 2/9 uprobes: is_trap_at_addr: don't use get_user_pages_remote()
In reply to Oleg Nesterovget_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 <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>
Acked-by Andrii Nakryiko <[email protected]>
Reviewed-by Jiri Olsa <[email protected]>
--- kernel/events/uprobes.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
kernel/events/uprobes.c +1 -7
@@ -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;
Signature · 1 line
2.25.1.362.g51ebf55
[tip: perf/core] uprobes: is_trap_at_addr: don't use get_user_pages_remote()
The following commit has been merged into the perf/core branch of tip:
The following commit has been merged into the perf/core branch of tip:
Commit-ID: 300b05621a3f85621130e356ca6ae90f6a4eec0e Gitweb: https://git.kernel.org/tip/300b05621a3f85621130e356ca6ae90f6a4eec0e Author: Oleg Nesterov <[email protected]> AuthorDate: Thu, 01 Aug 2024 15:27:14 +02:00 Committer: Peter Zijlstra <[email protected]> CommitterDate: Fri, 02 Aug 2024 11:30:30 +02:00
uprobes: is_trap_at_addr: don't use get_user_pages_remote()
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 <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Reviewed-by Jiri Olsa <[email protected]>
Acked-by Andrii Nakryiko <[email protected]>
--- kernel/events/uprobes.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
kernel/events/uprobes.c +1 -7
@@ -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;
Oleg Nesterov
maintainerPATCH v4 3/9 uprobes: simplify error handling for alloc_uprobe()
In reply to Oleg NesterovFrom: Andrii Nakryiko <[email protected]>
Return -ENOMEM instead of NULL, which makes caller's error handling just a touch simpler.
Signed-off-by Andrii Nakryiko <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>
Reviewed-by Masami Hiramatsu (Google) <[email protected]>
Reviewed-by Jiri Olsa <[email protected]>
--- kernel/events/uprobes.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
kernel/events/uprobes.c +1 -3
@@ -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);
Signature · 1 line
2.25.1.362.g51ebf55
[tip: perf/core] uprobes: simplify error handling for alloc_uprobe()
The following commit has been merged into the perf/core branch of tip:
The following commit has been merged into the perf/core branch of tip:
Commit-ID: 7c2bae2d9c27a89280b63ff3567d2dac2d89db28 Gitweb: https://git.kernel.org/tip/7c2bae2d9c27a89280b63ff3567d2dac2d89db28 Author: Andrii Nakryiko <[email protected]> AuthorDate: Thu, 01 Aug 2024 15:27:19 +02:00 Committer: Peter Zijlstra <[email protected]> CommitterDate: Fri, 02 Aug 2024 11:30:31 +02:00
uprobes: simplify error handling for alloc_uprobe()
Return -ENOMEM instead of NULL, which makes caller's error handling just a touch simpler.
Signed-off-by Andrii Nakryiko <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Reviewed-by "Masami Hiramatsu (Google)" <[email protected]>
Reviewed-by Jiri Olsa <[email protected]>
--- kernel/events/uprobes.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
kernel/events/uprobes.c +1 -3
@@ -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);
Oleg Nesterov
maintainerPATCH v4 4/9 selftests/bpf: fix uprobe.path leak in bpf_testmod
In reply to Oleg NesterovFrom: Jiri Olsa <[email protected]>
testmod_unregister_uprobe() forgets to path_put(&uprobe.path).
Signed-off-by Jiri Olsa <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>
--- tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c | 1 + 1 file changed, 1 insertion(+)
tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +1 -0
@@ -477,6 +477,7 @@ static void testmod_unregister_uprobe(void)if (uprobe.offset) {uprobe_unregister (d_real_inode (uprobe.path.dentry), uprobe.offset, &uprobe.consumer); +path_put (&uprobe.path); uprobe.offset =0 ; }
Signature · 1 line
2.25.1.362.g51ebf55
[tip: perf/core] selftests/bpf: fix uprobe.path leak in bpf_testmod
The following commit has been merged into the perf/core branch of tip:
The following commit has been merged into the perf/core branch of tip:
Commit-ID: db61e6a4eee5a7884b2cafeaf407895f253bbaa7 Gitweb: https://git.kernel.org/tip/db61e6a4eee5a7884b2cafeaf407895f253bbaa7 Author: Jiri Olsa <[email protected]> AuthorDate: Thu, 01 Aug 2024 15:27:24 +02:00 Committer: Peter Zijlstra <[email protected]> CommitterDate: Fri, 02 Aug 2024 11:30:31 +02:00
selftests/bpf: fix uprobe.path leak in bpf_testmod
testmod_unregister_uprobe() forgets to path_put(&uprobe.path).
Signed-off-by Jiri Olsa <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
--- tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c | 1 + 1 file changed, 1 insertion(+)
tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +1 -0
@@ -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 ; }
Oleg Nesterov
maintainerPATCH v4 5/9 uprobes: kill uprobe_register_refctr()
In reply to Oleg NesterovIt doesn't make any sense to have 2 versions of _register(). Note that trace_uprobe_enable(), the only user of uprobe_register(), doesn't need to check tu->ref_ctr_offset to decide which one should be used, it could safely pass ref_ctr_offset == 0 to uprobe_register_refctr().
Add this argument to uprobe_register(), update the callers, and kill uprobe_register_refctr().
Signed-off-by Oleg Nesterov <[email protected]>
Acked-by Andrii Nakryiko <[email protected]>
Reviewed-by Jiri Olsa <[email protected]>
--- 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(-)
include/linux/uprobes.h +2 -7
@@ -110,8 +110,7 @@ extern bool is_trap_insn(uprobe_opcode_t *insn);extern unsigned long uprobe_get_swbp_addr (struct pt_regs *regs);extern unsigned long uprobe_get_trap_addr (struct pt_regs *regs);extern int uprobe_write_opcode (struct arch_uprobe *auprobe,struct mm_struct *mm,unsigned long vaddr,uprobe_opcode_t ); -extern int uprobe_register (struct inode *inode,loff_t offset,struct uprobe_consumer *uc); -extern int uprobe_register_refctr (struct inode *inode,loff_t offset,loff_t ref_ctr_offset,struct uprobe_consumer *uc); +extern int uprobe_register (struct inode *inode,loff_t offset,loff_t ref_ctr_offset,struct uprobe_consumer *uc);extern int uprobe_apply (struct inode *inode,loff_t offset,struct uprobe_consumer *uc,bool );extern void uprobe_unregister (struct inode *inode,loff_t offset,struct uprobe_consumer *uc);extern int uprobe_mmap (struct vm_area_struct *vma);
@@ -152,11 +151,7 @@ static inline void uprobes_init(void)#define uprobe_get_trap_addr (regs) instruction_pointer(regs)static inline int -uprobe_register (struct inode *inode,loff_t offset,struct uprobe_consumer *uc) -{ -return -ENOSYS ; -} -static inline int uprobe_register_refctr (struct inode *inode,loff_t offset,loff_t ref_ctr_offset,struct uprobe_consumer *uc) +uprobe_register (struct inode *inode,loff_t offset,loff_t ref_ctr_offset,struct uprobe_consumer *uc) {return -ENOSYS ; }
kernel/events/uprobes.c +6 -18
@@ -1121,25 +1121,26 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumeEXPORT_SYMBOL_GPL (uprobe_unregister);/* -* __uprobe_register - register a probe +* uprobe_register - register a probe * @inode: the file in which the probe has to be placed. * @offset: offset from the start of the file. +* @ref_ctr_offset: offset of SDT marker / reference counter * @uc: information on howto handle the probe.. * -* Apart from the access refcount, __uprobe_register() takes a creation +* Apart from the access refcount, uprobe_register() takes a creation * refcount (thro alloc_uprobe) if and only if this @uprobe is getting * inserted into the rbtree (i.e first consumer for a @inode:@offset * tuple). Creation refcount stops uprobe_unregister from freeing the * @uprobe even before the register operation is complete. Creation * refcount is released when the last @uc for the @uprobe -* unregisters. Caller of __uprobe_register() is required to keep @inode +* unregisters. Caller of uprobe_register() is required to keep @inode * (and the containing mount) referenced. * * Return errno if it cannot successully install probes * else return 0 (success) */ -static int __uprobe_register (struct inode *inode,loff_t offset, -loff_t ref_ctr_offset,struct uprobe_consumer *uc) +int uprobe_register (struct inode *inode,loff_t offset,loff_t ref_ctr_offset, +struct uprobe_consumer *uc) {struct uprobe *uprobe;int ret;
@@ -1189,21 +1190,8 @@ static int __uprobe_register(struct inode *inode, loff_t offset, goto retry;return ret; } - -int uprobe_register (struct inode *inode,loff_t offset, -struct uprobe_consumer *uc) -{ -return __uprobe_register (inode, offset,0 , uc); -}EXPORT_SYMBOL_GPL (uprobe_register); -int uprobe_register_refctr (struct inode *inode,loff_t offset, -loff_t ref_ctr_offset,struct uprobe_consumer *uc) -{ -return __uprobe_register (inode, offset, ref_ctr_offset, uc); -} -EXPORT_SYMBOL_GPL (uprobe_register_refctr); -/* * uprobe_apply - unregister an already registered probe. * @inode: the file in which the probe has to be removed.
kernel/trace/bpf_trace.c +4 -4
@@ -3480,10 +3480,10 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr &bpf_uprobe_multi_link_lops, prog);for (i =0 ; i < cnt; i++) { - err =uprobe_register_refctr (d_real_inode (link->path.dentry), - uprobes[i].offset, - uprobes[i].ref_ctr_offset, - &uprobes[i].consumer); + err =uprobe_register (d_real_inode (link->path.dentry), + uprobes[i].offset, + uprobes[i].ref_ctr_offset, + &uprobes[i].consumer);if (err) {bpf_uprobe_unregister (&path, uprobes, i); goto error_free;
kernel/trace/trace_uprobe.c +1 -6
@@ -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 ;
tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +2 -2
@@ -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
Signature · 1 line
2.25.1.362.g51ebf55
[tip: perf/core] uprobes: kill uprobe_register_refctr()
The following commit has been merged into the perf/core branch of tip:
The following commit has been merged into the perf/core branch of tip:
Commit-ID: e04332ebc8ac128fa551e83f1161ab1c094d13a9 Gitweb: https://git.kernel.org/tip/e04332ebc8ac128fa551e83f1161ab1c094d13a9 Author: Oleg Nesterov <[email protected]> AuthorDate: Thu, 01 Aug 2024 15:27:28 +02:00 Committer: Peter Zijlstra <[email protected]> CommitterDate: Fri, 02 Aug 2024 11:30:31 +02:00
uprobes: kill uprobe_register_refctr()
It doesn't make any sense to have 2 versions of _register(). Note that trace_uprobe_enable(), the only user of uprobe_register(), doesn't need to check tu->ref_ctr_offset to decide which one should be used, it could safely pass ref_ctr_offset == 0 to uprobe_register_refctr().
Add this argument to uprobe_register(), update the callers, and kill uprobe_register_refctr().
Signed-off-by Oleg Nesterov <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Reviewed-by Jiri Olsa <[email protected]>
Acked-by Andrii Nakryiko <[email protected]>
--- include/linux/uprobes.h | 9 +---- kernel/events/uprobes.c | 24 ++-------- kernel/trace/bpf_trace.c | 8 +-- kernel/trace/trace_uprobe.c | 7 +--- tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c | 4 +- 5 files changed, 15 insertions(+), 37 deletions(-)
include/linux/uprobes.h +2 -7
@@ -112,8 +112,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);
@@ -154,11 +153,7 @@ static inline void uprobes_init(void)#define uprobe_get_trap_addr (regs) instruction_pointer(regs)static inline int -uprobe_register (struct inode *inode,loff_t offset,struct uprobe_consumer *uc) -{ -return -ENOSYS ; -} -static inline int uprobe_register_refctr (struct inode *inode,loff_t offset,loff_t ref_ctr_offset,struct uprobe_consumer *uc) +uprobe_register (struct inode *inode,loff_t offset,loff_t ref_ctr_offset,struct uprobe_consumer *uc) {return -ENOSYS ; }
kernel/events/uprobes.c +6 -18
@@ -1121,25 +1121,26 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumeEXPORT_SYMBOL_GPL (uprobe_unregister);/* -* __uprobe_register - register a probe +* uprobe_register - register a probe * @inode: the file in which the probe has to be placed. * @offset: offset from the start of the file. +* @ref_ctr_offset: offset of SDT marker / reference counter * @uc: information on howto handle the probe.. * -* Apart from the access refcount, __uprobe_register() takes a creation +* Apart from the access refcount, uprobe_register() takes a creation * refcount (thro alloc_uprobe) if and only if this @uprobe is getting * inserted into the rbtree (i.e first consumer for a @inode:@offset * tuple). Creation refcount stops uprobe_unregister from freeing the * @uprobe even before the register operation is complete. Creation * refcount is released when the last @uc for the @uprobe -* unregisters. Caller of __uprobe_register() is required to keep @inode +* unregisters. Caller of uprobe_register() is required to keep @inode * (and the containing mount) referenced. * * Return errno if it cannot successully install probes * else return 0 (success) */ -static int __uprobe_register (struct inode *inode,loff_t offset, -loff_t ref_ctr_offset,struct uprobe_consumer *uc) +int uprobe_register (struct inode *inode,loff_t offset,loff_t ref_ctr_offset, +struct uprobe_consumer *uc) {struct uprobe *uprobe;int ret;
@@ -1189,21 +1190,8 @@ static int __uprobe_register(struct inode *inode, loff_t offset, goto retry;return ret; } - -int uprobe_register (struct inode *inode,loff_t offset, -struct uprobe_consumer *uc) -{ -return __uprobe_register (inode, offset,0 , uc); -}EXPORT_SYMBOL_GPL (uprobe_register); -int uprobe_register_refctr (struct inode *inode,loff_t offset, -loff_t ref_ctr_offset,struct uprobe_consumer *uc) -{ -return __uprobe_register (inode, offset, ref_ctr_offset, uc); -} -EXPORT_SYMBOL_GPL (uprobe_register_refctr); -/* * uprobe_apply - unregister an already registered probe. * @inode: the file in which the probe has to be removed.
kernel/trace/bpf_trace.c +4 -4
@@ -3480,10 +3480,10 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr &bpf_uprobe_multi_link_lops, prog);for (i =0 ; i < cnt; i++) { - err =uprobe_register_refctr (d_real_inode (link->path.dentry), - uprobes[i].offset, - uprobes[i].ref_ctr_offset, - &uprobes[i].consumer); + err =uprobe_register (d_real_inode (link->path.dentry), + uprobes[i].offset, + uprobes[i].ref_ctr_offset, + &uprobes[i].consumer);if (err) {bpf_uprobe_unregister (&path, uprobes, i); goto error_free;
kernel/trace/trace_uprobe.c +1 -6
@@ -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 ;
tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +2 -2
@@ -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
Oleg Nesterov
maintainerPATCH v4 6/9 uprobes: make uprobe_register() return struct uprobe *
In reply to Oleg NesterovThis 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).
Co-developed-by Andrii Nakryiko <[email protected]>
Signed-off-by Andrii Nakryiko <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>
Reviewed-by Jiri Olsa <[email protected]>
--- include/linux/uprobes.h | 15 ++--- kernel/events/uprobes.c | 56 ++++++++----------- kernel/trace/bpf_trace.c | 25 ++++----- kernel/trace/trace_uprobe.c | 26 ++++----- .../selftests/bpf/bpf_testmod/bpf_testmod.c | 25 ++++----- 5 files changed, 67 insertions(+), 80 deletions(-)
include/linux/uprobes.h +8 -7
@@ -16,6 +16,7 @@#include <linux/types.h> #include <linux/wait.h> +struct uprobe ;struct vm_area_struct ;struct mm_struct ;struct inode ;
@@ -110,9 +111,9 @@ extern bool is_trap_insn(uprobe_opcode_t *insn);extern unsigned long uprobe_get_swbp_addr (struct pt_regs *regs);extern unsigned long uprobe_get_trap_addr (struct pt_regs *regs);extern int uprobe_write_opcode (struct arch_uprobe *auprobe,struct mm_struct *mm,unsigned long vaddr,uprobe_opcode_t ); -extern int uprobe_register (struct inode *inode,loff_t offset,loff_t ref_ctr_offset,struct uprobe_consumer *uc); -extern int uprobe_apply (struct inode *inode,loff_t offset,struct uprobe_consumer *uc,bool ); -extern void uprobe_unregister (struct inode *inode,loff_t offset,struct uprobe_consumer *uc); +extern struct uprobe *uprobe_register (struct inode *inode,loff_t offset,loff_t ref_ctr_offset,struct uprobe_consumer *uc); +extern int uprobe_apply (struct uprobe *uprobe,struct uprobe_consumer *uc,bool ); +extern void uprobe_unregister (struct uprobe *uprobe,struct uprobe_consumer *uc);extern int uprobe_mmap (struct vm_area_struct *vma);extern void uprobe_munmap (struct vm_area_struct *vma,unsigned long start,unsigned long end);extern void uprobe_start_dup_mmap (void );
@@ -150,18 +151,18 @@ static inline void uprobes_init(void)#define uprobe_get_trap_addr (regs) instruction_pointer(regs) -static inline int +static inline struct uprobe *uprobe_register (struct inode *inode,loff_t offset,loff_t ref_ctr_offset,struct uprobe_consumer *uc) { -return -ENOSYS ; +return ERR_PTR (-ENOSYS ); }static inline int -uprobe_apply (struct inode *inode,loff_t offset,struct uprobe_consumer *uc,bool add) +uprobe_apply (struct uprobe * uprobe,struct uprobe_consumer *uc,bool add) {return -ENOSYS ; }static inline void -uprobe_unregister (struct inode *inode,loff_t offset,struct uprobe_consumer *uc) +uprobe_unregister (struct uprobe *uprobe,struct uprobe_consumer *uc) { }static inline int uprobe_mmap (struct vm_area_struct *vma)
kernel/events/uprobes.c +22 -34
124 lines not on this page · raw message
kernel/trace/bpf_trace.c +12 -13
@@ -3160,6 +3160,7 @@ struct bpf_uprobe {loff_t offset;unsigned long ref_ctr_offset;u64 cookie; +struct uprobe *uprobe;struct uprobe_consumer consumer; };
@@ -3178,15 +3179,12 @@ struct bpf_uprobe_multi_run_ctx {struct bpf_uprobe *uprobe; }; -static void bpf_uprobe_unregister (struct path *path,struct bpf_uprobe *uprobes, -u32 cnt) +static void bpf_uprobe_unregister (struct bpf_uprobe *uprobes,u32 cnt) {u32 i; -for (i =0 ; i < cnt; i++) { -uprobe_unregister (d_real_inode (path->dentry), uprobes[i].offset, - &uprobes[i].consumer); - } +for (i =0 ; i < cnt; i++) +uprobe_unregister (uprobes[i].uprobe, &uprobes[i].consumer); }static void bpf_uprobe_multi_link_release (struct bpf_link *link)
@@ -3194,7 +3192,7 @@ static void bpf_uprobe_multi_link_release(struct bpf_link *link)struct bpf_uprobe_multi_link *umulti_link; umulti_link = container_of(link, struct bpf_uprobe_multi_link, link); -bpf_uprobe_unregister (&umulti_link->path, umulti_link->uprobes, umulti_link->cnt); +bpf_uprobe_unregister (umulti_link->uprobes, umulti_link->cnt);if (umulti_link->task)put_task_struct (umulti_link->task);path_put (&umulti_link->path);
@@ -3480,12 +3478,13 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr &bpf_uprobe_multi_link_lops, prog);for (i =0 ; i < cnt; i++) { - err =uprobe_register (d_real_inode (link->path.dentry), - uprobes[i].offset, - uprobes[i].ref_ctr_offset, - &uprobes[i].consumer); -if (err) { -bpf_uprobe_unregister (&path, uprobes, i); + uprobes[i].uprobe =uprobe_register (d_real_inode (link->path.dentry), + uprobes[i].offset, + uprobes[i].ref_ctr_offset, + &uprobes[i].consumer); +if (IS_ERR (uprobes[i].uprobe)) { + err =PTR_ERR (uprobes[i].uprobe); +bpf_uprobe_unregister (uprobes, i); goto error_free; } }
kernel/trace/trace_uprobe.c +13 -13
@@ -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 ;
tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +12 -13
@@ -432,7 +432,7 @@ uprobe_ret_handler(struct uprobe_consumer *self, unsigned long func,struct testmod_uprobe {struct path path; -loff_t offset; +struct uprobe *uprobe;struct uprobe_consumer consumer; };
@@ -446,25 +446,25 @@ static int testmod_register_uprobe(loff_t offset) {int err = -EBUSY ; -if (uprobe.offset) +if (uprobe.uprobe)return -EBUSY ;mutex_lock (&testmod_uprobe_mutex); -if (uprobe.offset) +if (uprobe.uprobe) goto out; err =kern_path ("/proc/self/exe" ,LOOKUP_FOLLOW , &uprobe.path);if (err) goto out; - err =uprobe_register (d_real_inode (uprobe.path.dentry), - offset,0 , &uprobe.consumer); -if (err) + uprobe.uprobe =uprobe_register (d_real_inode (uprobe.path.dentry), + offset,0 , &uprobe.consumer); +if (IS_ERR (uprobe.uprobe)) { + err =PTR_ERR (uprobe.uprobe);path_put (&uprobe.path); -else - uprobe.offset = offset; - + uprobe.uprobe =NULL ; + } out:mutex_unlock (&testmod_uprobe_mutex);return err;
@@ -474,11 +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); +if (uprobe.uprobe) { +uprobe_unregister (uprobe.uprobe, &uprobe.consumer);path_put (&uprobe.path); - uprobe.offset =0 ; + uprobe.uprobe =NULL ; }mutex_unlock (&testmod_uprobe_mutex);
Signature · 1 line
2.25.1.362.g51ebf55
[tip: perf/core] uprobes: make uprobe_register() return struct uprobe *
The following commit has been merged into the perf/core branch of tip:
The following commit has been merged into the perf/core branch of tip:
Commit-ID: 3c83a9ad0295eb63bdeb81d821b8c3b9417fbcac Gitweb: https://git.kernel.org/tip/3c83a9ad0295eb63bdeb81d821b8c3b9417fbcac Author: Oleg Nesterov <[email protected]> AuthorDate: Thu, 01 Aug 2024 15:27:34 +02:00 Committer: Peter Zijlstra <[email protected]> CommitterDate: Fri, 02 Aug 2024 11:30:31 +02:00
uprobes: make uprobe_register() return struct uprobe *
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).
Co-developed-by Andrii Nakryiko <[email protected]>
Signed-off-by Andrii Nakryiko <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Reviewed-by Jiri Olsa <[email protected]>
--- include/linux/uprobes.h | 15 +-- kernel/events/uprobes.c | 56 +++------- kernel/trace/bpf_trace.c | 25 +--- kernel/trace/trace_uprobe.c | 26 ++--- tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c | 25 +--- 5 files changed, 67 insertions(+), 80 deletions(-)
include/linux/uprobes.h +8 -7
@@ -16,6 +16,7 @@#include <linux/types.h> #include <linux/wait.h> +struct uprobe ;struct vm_area_struct ;struct mm_struct ;struct inode ;
@@ -112,9 +113,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 );
@@ -152,18 +153,18 @@ static inline void uprobes_init(void)#define uprobe_get_trap_addr (regs) instruction_pointer(regs) -static inline int +static inline struct uprobe *uprobe_register (struct inode *inode,loff_t offset,loff_t ref_ctr_offset,struct uprobe_consumer *uc) { -return -ENOSYS ; +return ERR_PTR (-ENOSYS ); }static inline int -uprobe_apply (struct inode *inode,loff_t offset,struct uprobe_consumer *uc,bool add) +uprobe_apply (struct uprobe * uprobe,struct uprobe_consumer *uc,bool add) {return -ENOSYS ; }static inline void -uprobe_unregister (struct inode *inode,loff_t offset,struct uprobe_consumer *uc) +uprobe_unregister (struct uprobe *uprobe,struct uprobe_consumer *uc) { }static inline int uprobe_mmap (struct vm_area_struct *vma)
kernel/events/uprobes.c +22 -34
124 lines not on this page · raw message
kernel/trace/bpf_trace.c +12 -13
@@ -3160,6 +3160,7 @@ struct bpf_uprobe {loff_t offset;unsigned long ref_ctr_offset;u64 cookie; +struct uprobe *uprobe;struct uprobe_consumer consumer; };
@@ -3178,15 +3179,12 @@ struct bpf_uprobe_multi_run_ctx {struct bpf_uprobe *uprobe; }; -static void bpf_uprobe_unregister (struct path *path,struct bpf_uprobe *uprobes, -u32 cnt) +static void bpf_uprobe_unregister (struct bpf_uprobe *uprobes,u32 cnt) {u32 i; -for (i =0 ; i < cnt; i++) { -uprobe_unregister (d_real_inode (path->dentry), uprobes[i].offset, - &uprobes[i].consumer); - } +for (i =0 ; i < cnt; i++) +uprobe_unregister (uprobes[i].uprobe, &uprobes[i].consumer); }static void bpf_uprobe_multi_link_release (struct bpf_link *link)
@@ -3194,7 +3192,7 @@ static void bpf_uprobe_multi_link_release(struct bpf_link *link)struct bpf_uprobe_multi_link *umulti_link; umulti_link = container_of(link, struct bpf_uprobe_multi_link, link); -bpf_uprobe_unregister (&umulti_link->path, umulti_link->uprobes, umulti_link->cnt); +bpf_uprobe_unregister (umulti_link->uprobes, umulti_link->cnt);if (umulti_link->task)put_task_struct (umulti_link->task);path_put (&umulti_link->path);
@@ -3480,12 +3478,13 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr &bpf_uprobe_multi_link_lops, prog);for (i =0 ; i < cnt; i++) { - err =uprobe_register (d_real_inode (link->path.dentry), - uprobes[i].offset, - uprobes[i].ref_ctr_offset, - &uprobes[i].consumer); -if (err) { -bpf_uprobe_unregister (&path, uprobes, i); + uprobes[i].uprobe =uprobe_register (d_real_inode (link->path.dentry), + uprobes[i].offset, + uprobes[i].ref_ctr_offset, + &uprobes[i].consumer); +if (IS_ERR (uprobes[i].uprobe)) { + err =PTR_ERR (uprobes[i].uprobe); +bpf_uprobe_unregister (uprobes, i); goto error_free; } }
kernel/trace/trace_uprobe.c +13 -13
@@ -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 ;
tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +12 -13
@@ -432,7 +432,7 @@ uprobe_ret_handler(struct uprobe_consumer *self, unsigned long func,struct testmod_uprobe {struct path path; -loff_t offset; +struct uprobe *uprobe;struct uprobe_consumer consumer; };
@@ -446,25 +446,25 @@ static int testmod_register_uprobe(loff_t offset) {int err = -EBUSY ; -if (uprobe.offset) +if (uprobe.uprobe)return -EBUSY ;mutex_lock (&testmod_uprobe_mutex); -if (uprobe.offset) +if (uprobe.uprobe) goto out; err =kern_path ("/proc/self/exe" ,LOOKUP_FOLLOW , &uprobe.path);if (err) goto out; - err =uprobe_register (d_real_inode (uprobe.path.dentry), - offset,0 , &uprobe.consumer); -if (err) + uprobe.uprobe =uprobe_register (d_real_inode (uprobe.path.dentry), + offset,0 , &uprobe.consumer); +if (IS_ERR (uprobe.uprobe)) { + err =PTR_ERR (uprobe.uprobe);path_put (&uprobe.path); -else - uprobe.offset = offset; - + uprobe.uprobe =NULL ; + } out:mutex_unlock (&testmod_uprobe_mutex);return err;
@@ -474,11 +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); +if (uprobe.uprobe) { +uprobe_unregister (uprobe.uprobe, &uprobe.consumer);path_put (&uprobe.path); - uprobe.offset =0 ; + uprobe.uprobe =NULL ; }mutex_unlock (&testmod_uprobe_mutex);
Oleg Nesterov
maintainerPATCH v4 7/9 uprobes: change uprobe_register() to use uprobe_unregister() instead of __uprobe_unregister()
In reply to Oleg NesterovIf register_for_each_vma() fails uprobe_register() can safely drop uprobe->register_rwsem and use uprobe_unregister(). There is no worry about the races with another register/unregister, consumer_add() was already called so this case doesn't differ from _unregister() right after the successful _register().
Yes this means the extra up_write() + down_write(), but this is the slow and unlikely case anyway.
Signed-off-by Oleg Nesterov <[email protected]>
Acked-by Andrii Nakryiko <[email protected]>
Reviewed-by Jiri Olsa <[email protected]>
--- kernel/events/uprobes.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
kernel/events/uprobes.c +7 -5
@@ -1174,16 +1174,18 @@ struct uprobe *uprobe_register(struct inode *inode,if (likely (uprobe_is_active (uprobe))) {consumer_add (uprobe, uc); ret =register_for_each_vma (uprobe, uc); -if (ret) -__uprobe_unregister (uprobe, uc); }up_write (&uprobe->register_rwsem);put_uprobe (uprobe); -if (unlikely (ret == -EAGAIN )) - goto retry; +if (ret) { +if (unlikely (ret == -EAGAIN )) + goto retry; +uprobe_unregister (uprobe, uc); +return ERR_PTR (ret); + } -return ret ?ERR_PTR (ret) : uprobe; +return uprobe; }EXPORT_SYMBOL_GPL (uprobe_register);
Signature · 1 line
2.25.1.362.g51ebf55
[tip: perf/core] uprobes: change uprobe_register() to use uprobe_unregister() instead of __uprobe_unregister()
The following commit has been merged into the perf/core branch of tip:
The following commit has been merged into the perf/core branch of tip:
Commit-ID: bb18c5de1c288050ef8bd4af4ca16896ad4cd3fc Gitweb: https://git.kernel.org/tip/bb18c5de1c288050ef8bd4af4ca16896ad4cd3fc Author: Oleg Nesterov <[email protected]> AuthorDate: Thu, 01 Aug 2024 15:27:39 +02:00 Committer: Peter Zijlstra <[email protected]> CommitterDate: Fri, 02 Aug 2024 11:30:32 +02:00
uprobes: change uprobe_register() to use uprobe_unregister() instead of __uprobe_unregister()
If register_for_each_vma() fails uprobe_register() can safely drop uprobe->register_rwsem and use uprobe_unregister(). There is no worry about the races with another register/unregister, consumer_add() was already called so this case doesn't differ from _unregister() right after the successful _register().
Yes this means the extra up_write() + down_write(), but this is the slow and unlikely case anyway.
Signed-off-by Oleg Nesterov <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Reviewed-by Jiri Olsa <[email protected]>
Acked-by Andrii Nakryiko <[email protected]>
--- kernel/events/uprobes.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
kernel/events/uprobes.c +7 -5
@@ -1174,16 +1174,18 @@ struct uprobe *uprobe_register(struct inode *inode,if (likely (uprobe_is_active (uprobe))) {consumer_add (uprobe, uc); ret =register_for_each_vma (uprobe, uc); -if (ret) -__uprobe_unregister (uprobe, uc); }up_write (&uprobe->register_rwsem);put_uprobe (uprobe); -if (unlikely (ret == -EAGAIN )) - goto retry; +if (ret) { +if (unlikely (ret == -EAGAIN )) + goto retry; +uprobe_unregister (uprobe, uc); +return ERR_PTR (ret); + } -return ret ?ERR_PTR (ret) : uprobe; +return uprobe; }EXPORT_SYMBOL_GPL (uprobe_register);
Oleg Nesterov
maintainerPATCH v4 8/9 uprobes: fold __uprobe_unregister() into uprobe_unregister()
In reply to Oleg NesterovFold __uprobe_unregister() into its single caller, uprobe_unregister(). A separate patch to simplify the next change.
Signed-off-by Oleg Nesterov <[email protected]>
Acked-by Andrii Nakryiko <[email protected]>
Reviewed-by Jiri Olsa <[email protected]>
--- kernel/events/uprobes.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-)
kernel/events/uprobes.c +10 -15
@@ -1085,20 +1085,6 @@ register_for_each_vma(struct uprobe *uprobe, struct uprobe_consumer *new)return err; } -static void -__uprobe_unregister (struct uprobe *uprobe,struct uprobe_consumer *uc) -{ -int err; - -if (WARN_ON (!consumer_del (uprobe, uc))) -return ; - - err =register_for_each_vma (uprobe,NULL ); -/* TODO : cant unregister? schedule a worker thread */ -if (!uprobe->consumers && !err) -delete_uprobe (uprobe); -} -/** * uprobe_unregister - unregister an already registered probe. * @uprobe: uprobe to remove
@@ -1106,9 +1092,18 @@ __uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)*/ void uprobe_unregister (struct uprobe *uprobe,struct uprobe_consumer *uc) { +int err; +get_uprobe (uprobe);down_write (&uprobe->register_rwsem); -__uprobe_unregister (uprobe, uc); +if (WARN_ON (!consumer_del (uprobe, uc))) + err = -ENOENT ; +else + err =register_for_each_vma (uprobe,NULL ); + +/* TODO : cant unregister? schedule a worker thread */ +if (!err && !uprobe->consumers) +delete_uprobe (uprobe);up_write (&uprobe->register_rwsem);put_uprobe (uprobe); }
Signature · 1 line
2.25.1.362.g51ebf55
[tip: perf/core] uprobes: fold __uprobe_unregister() into uprobe_unregister()
The following commit has been merged into the perf/core branch of tip:
The following commit has been merged into the perf/core branch of tip:
Commit-ID: 70408bebba94a63ea11471ed00168cd8606a9328 Gitweb: https://git.kernel.org/tip/70408bebba94a63ea11471ed00168cd8606a9328 Author: Oleg Nesterov <[email protected]> AuthorDate: Thu, 01 Aug 2024 15:27:44 +02:00 Committer: Peter Zijlstra <[email protected]> CommitterDate: Fri, 02 Aug 2024 11:30:32 +02:00
uprobes: fold __uprobe_unregister() into uprobe_unregister()
Fold __uprobe_unregister() into its single caller, uprobe_unregister(). A separate patch to simplify the next change.
Signed-off-by Oleg Nesterov <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Reviewed-by Jiri Olsa <[email protected]>
Acked-by Andrii Nakryiko <[email protected]>
--- kernel/events/uprobes.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-)
kernel/events/uprobes.c +10 -15
@@ -1085,20 +1085,6 @@ register_for_each_vma(struct uprobe *uprobe, struct uprobe_consumer *new)return err; } -static void -__uprobe_unregister (struct uprobe *uprobe,struct uprobe_consumer *uc) -{ -int err; - -if (WARN_ON (!consumer_del (uprobe, uc))) -return ; - - err =register_for_each_vma (uprobe,NULL ); -/* TODO : cant unregister? schedule a worker thread */ -if (!uprobe->consumers && !err) -delete_uprobe (uprobe); -} -/** * uprobe_unregister - unregister an already registered probe. * @uprobe: uprobe to remove
@@ -1106,9 +1092,18 @@ __uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)*/ void uprobe_unregister (struct uprobe *uprobe,struct uprobe_consumer *uc) { +int err; +get_uprobe (uprobe);down_write (&uprobe->register_rwsem); -__uprobe_unregister (uprobe, uc); +if (WARN_ON (!consumer_del (uprobe, uc))) + err = -ENOENT ; +else + err =register_for_each_vma (uprobe,NULL ); + +/* TODO : cant unregister? schedule a worker thread */ +if (!err && !uprobe->consumers) +delete_uprobe (uprobe);up_write (&uprobe->register_rwsem);put_uprobe (uprobe); }
Oleg Nesterov
maintainerPATCH v4 9/9 uprobes: shift put_uprobe() from delete_uprobe() to uprobe_unregister()
In reply to Oleg NesterovKill the extra get_uprobe() + put_uprobe() in uprobe_unregister() and move the possibly final put_uprobe() from delete_uprobe() to its only caller, uprobe_unregister().
Signed-off-by Oleg Nesterov <[email protected]>
Acked-by Andrii Nakryiko <[email protected]>
Reviewed-by Jiri Olsa <[email protected]>
--- kernel/events/uprobes.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
kernel/events/uprobes.c +9 -5
@@ -939,7 +939,6 @@ static void delete_uprobe(struct uprobe *uprobe)rb_erase (&uprobe->rb_node, &uprobes_tree);write_unlock (&uprobes_treelock);RB_CLEAR_NODE (&uprobe->rb_node);/* for uprobe_is_active() */ -put_uprobe (uprobe); }struct map_info {
@@ -1094,7 +1093,6 @@ void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc) {int err; -get_uprobe (uprobe);down_write (&uprobe->register_rwsem);if (WARN_ON (!consumer_del (uprobe, uc))) err = -ENOENT ;
@@ -1102,10 +1100,16 @@ void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc) err =register_for_each_vma (uprobe,NULL );/* TODO : cant unregister? schedule a worker thread */ -if (!err && !uprobe->consumers) -delete_uprobe (uprobe); +if (!err) { +if (!uprobe->consumers) +delete_uprobe (uprobe); +else + err = -EBUSY ; + }up_write (&uprobe->register_rwsem); -put_uprobe (uprobe); + +if (!err) +put_uprobe (uprobe); }EXPORT_SYMBOL_GPL (uprobe_unregister);
Signature · 1 line
2.25.1.362.g51ebf55
[tip: perf/core] uprobes: shift put_uprobe() from delete_uprobe() to uprobe_unregister()
The following commit has been merged into the perf/core branch of tip:
The following commit has been merged into the perf/core branch of tip:
Commit-ID: 12026d2034dfeb575e0eb28f33431cbf03dc732c Gitweb: https://git.kernel.org/tip/12026d2034dfeb575e0eb28f33431cbf03dc732c Author: Oleg Nesterov <[email protected]> AuthorDate: Thu, 01 Aug 2024 15:27:49 +02:00 Committer: Peter Zijlstra <[email protected]> CommitterDate: Fri, 02 Aug 2024 11:30:32 +02:00
uprobes: shift put_uprobe() from delete_uprobe() to uprobe_unregister()
Kill the extra get_uprobe() + put_uprobe() in uprobe_unregister() and move the possibly final put_uprobe() from delete_uprobe() to its only caller, uprobe_unregister().
Signed-off-by Oleg Nesterov <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Reviewed-by Jiri Olsa <[email protected]>
Acked-by Andrii Nakryiko <[email protected]>
--- kernel/events/uprobes.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
kernel/events/uprobes.c +9 -5
@@ -939,7 +939,6 @@ static void delete_uprobe(struct uprobe *uprobe)rb_erase (&uprobe->rb_node, &uprobes_tree);write_unlock (&uprobes_treelock);RB_CLEAR_NODE (&uprobe->rb_node);/* for uprobe_is_active() */ -put_uprobe (uprobe); }struct map_info {
@@ -1094,7 +1093,6 @@ void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc) {int err; -get_uprobe (uprobe);down_write (&uprobe->register_rwsem);if (WARN_ON (!consumer_del (uprobe, uc))) err = -ENOENT ;
@@ -1102,10 +1100,16 @@ void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc) err =register_for_each_vma (uprobe,NULL );/* TODO : cant unregister? schedule a worker thread */ -if (!err && !uprobe->consumers) -delete_uprobe (uprobe); +if (!err) { +if (!uprobe->consumers) +delete_uprobe (uprobe); +else + err = -EBUSY ; + }up_write (&uprobe->register_rwsem); -put_uprobe (uprobe); + +if (!err) +put_uprobe (uprobe); }EXPORT_SYMBOL_GPL (uprobe_unregister);
Peter Zijlstra
maintainerQuoted 1 line · Oleg Nesterov
(Andrii, I'll try to look at your new series on Weekend).
OK, I dropped all your previous patches and stuffed these in.
They should all be visible in queue/perf/core, and provided the robot doesn't scream, I'll push them into tip/perf/core soonish.
+ bpf
Quoted 8 lines · Peter Zijlstra
Quoted 1 line · Oleg Nesterov
(Andrii, I'll try to look at your new series on Weekend).
OK, I dropped all your previous patches and stuffed these in.
They should all be visible in queue/perf/core, and provided the robot doesn't scream, I'll push them into tip/perf/core soonish.
Just FYI, it seems like tip/perf/core is currently broken for uprobes (and by implication also queue/perf/core). Also torvalds/linux/master master is broken. See what I'm getting when running BPF selftests dealing with uprobes. Sometimes I only get that WARNING and nothing else.
I'm bisecting at the moment with bpf/master being a "good" checkpoint, will let you know once I bisect.
[ 34.343557] ------------[ cut here ]------------
[ 34.343906] WARNING: CPU: 3 PID: 2364 at
kernel/trace/trace_uprobe.c:1109 __probe_event_disable+0x26/0x80
[ 34.344468] Modules linked in:
[ 34.344488] BUG: unable to handle page fault for address: ffffc90001c5bea0
[ 34.345071] #PF: supervisor read access in kernel mode
[ 34.345370] #PF: error_code(0x0000) - not-present page
[ 34.345700] PGD 100000067 P4D 100000067 PUD ffff88810d86cd40
[ 34.346061] Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
[ 34.346377] CPU: 3 UID: 0 PID: 2364 Comm: test_progs Tainted: G
OE 6.11.0-rc1-00006-g6763ebdb4983 #115
[ 34.347052] Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
[ 34.347392] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04
[ 34.348085] RIP: 0010:__wake_up_common+0x3e/0xc0
[ 34.348359] Code: 89 d4 55 53 48 89 fb 48 83 ec 08 8b 05 c3 05 a8
02 89 74 24 04 85 c0 75 6d 48 8b 43 48 48 83 c3 48 4f
[ 34.349440] RSP: 0018:ffffc900001d0d90 EFLAGS: 00010087
[ 34.349796] RAX: ffffc90001c5bea0 RBX: ffff88810138e0e8 RCX: 0000000000000001
[ 34.350282] RDX: 0000000080010005 RSI: ffffffff8295f82c RDI: ffffc90001c5be88
[ 34.350768] RBP: ffff88810138e0a0 R08: 0000000000000000 R09: 000000000000438f
[ 34.351245] R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000001
[ 34.351703] R13: 0000000000000046 R14: 0000000000000000 R15: 0000000000000000
[ 34.352112] FS: 00007fd71c7b3d00(0000) GS:ffff88881ca00000(0000)
knlGS:0000000000000000
[ 34.352574] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 34.352911] CR2: ffffc90001c5bea0 CR3: 000000010b456005 CR4: 0000000000370ef0
[ 34.353320] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 34.353734] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 34.354144] Call Trace:
[ 34.354290] <IRQ>
[ 34.354413] ? __die+0x1f/0x60
[ 34.354601] ? page_fault_oops+0x14c/0x450
[ 34.354848] ? search_bpf_extables+0xa8/0x150
[ 34.355105] ? fixup_exception+0x22/0x2d0
[ 34.355342] ? exc_page_fault+0x207/0x210
[ 34.355579] ? asm_exc_page_fault+0x22/0x30
[ 34.355829] ? __wake_up_common+0x3e/0xc0
[ 34.356065] __wake_up+0x32/0x60
[ 34.356261] ep_poll_callback+0x13e/0x270
[ 34.356502] __wake_up_common+0x7d/0xc0
[ 34.356731] __wake_up+0x32/0x60
[ 34.356961] irq_work_single+0x67/0x90
[ 34.357184] irq_work_run_list+0x26/0x40
[ 34.357442] update_process_times+0x83/0xa0
[ 34.357717] tick_nohz_handler+0x97/0x140
[ 34.357977] ? __pfx_tick_nohz_handler+0x10/0x10
[ 34.358283] __hrtimer_run_queues+0x19a/0x3b0
[ 34.358580] hrtimer_interrupt+0xfe/0x240
[ 34.358864] __sysvec_apic_timer_interrupt+0x87/0x210
[ 34.359189] sysvec_apic_timer_interrupt+0x98/0xc0
[ 34.359487] </IRQ>
[ 34.359614] <TASK>
[ 34.359745] asm_sysvec_apic_timer_interrupt+0x16/0x20
[ 34.360042] RIP: 0010:print_modules+0x27/0xd0
[ 34.360301] Code: 90 90 90 0f 1f 44 00 00 53 48 c7 c7 63 22 98 82
48 83 ec 18 e8 da 8d fc ff bf 01 00 00 00 e8 80 d1 f1
[ 34.361372] RSP: 0018:ffffc90001e5bc58 EFLAGS: 00000297
[ 34.361682] RAX: ffffffffa03f0948 RBX: 0000000000000000 RCX: 0000000000000000
[ 34.362091] RDX: 0000000000000002 RSI: 0000000000000027 RDI: 0000000000000001
[ 34.362502] RBP: ffffc90001e5bd28 R08: 00000000fffeffff R09: 0000000000000001
[ 34.362917] R10: 0000000000000000 R11: ffffffff83299920 R12: ffffffff81233536
[ 34.363325] R13: 0000000000000009 R14: 0000000000000455 R15: ffffffff8298f924
[ 34.363740] ? __probe_event_disable+0x26/0x80
[ 34.364009] ? print_modules+0x20/0xd0
[ 34.364230] ? __probe_event_disable+0x26/0x80
[ 34.364488] __warn+0x6f/0x180
[ 34.364683] ? __probe_event_disable+0x26/0x80
[ 34.364945] report_bug+0x18d/0x1c0
[ 34.365156] handle_bug+0x3a/0x70
[ 34.365354] exc_invalid_op+0x13/0x60
[ 34.365571] asm_exc_invalid_op+0x16/0x20
[ 34.365838] RIP: 0010:__probe_event_disable+0x26/0x80
[ 34.366175] Code: 90 90 90 90 55 48 89 fd 53 48 8b 47 10 8b 90 38
01 00 00 85 d2 75 13 48 8b 88 40 01 00 00 48 8d 90 40
[ 34.367340] RSP: 0018:ffffc90001e5bdd0 EFLAGS: 00010287
[ 34.367651] RAX: ffff88810d86cc00 RBX: ffff88810d86cce0 RCX: ffff8881075c8168
[ 34.368067] RDX: ffff88810d86cd40 RSI: 0000000000000003 RDI: ffff88810a5e1db0
[ 34.368482] RBP: ffff88810a5e1db0 R08: 00000000ffffffff R09: 0000000000000000
[ 34.368896] R10: 0000000000000001 R11: 0000000000000000 R12: ffff8881075c8250
[ 34.369310] R13: ffff8881075c82f0 R14: ffffc90001e5bb80 R15: ffff8881075c8000
[ 34.369775] trace_uprobe_register+0x1a8/0x300
[ 34.370053] perf_trace_event_unreg.isra.0+0x22/0x80
[ 34.370342] perf_uprobe_destroy+0x3a/0x70
[ 34.370582] _free_event+0x114/0x580
[ 34.370806] perf_event_release_kernel+0x282/0x2c0
[ 34.371123] perf_release+0x11/0x20
[ 34.371354] __fput+0x102/0x2e0
[ 34.371561] task_work_run+0x55/0xa0
[ 34.371798] syscall_exit_to_user_mode+0x1dd/0x1f0
[ 34.372104] do_syscall_64+0x70/0x140
[ 34.372337] entry_SYSCALL_64_after_hwframe+0x76/0x7e
[ 34.372659] RIP: 0033:0x7fd71c986a94
[ 34.372901] Code: 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84
00 00 00 00 00 90 f3 0f 1e fa 80 3d d5 18 0e 00 00 73
[ 34.373987] RSP: 002b:00007ffc09d83118 EFLAGS: 00000202 ORIG_RAX:
0000000000000003
[ 34.374426] RAX: 0000000000000000 RBX: 00007ffc09d835e8 RCX: 00007fd71c986a94
[ 34.374874] RDX: 000000000000000b RSI: 0000000000002401 RDI: 000000000000000c
[ 34.375325] RBP: 00007ffc09d83150 R08: 00000000049d39c7 R09: 0000000000000007
[ 34.375782] R10: 00000000049d71b0 R11: 0000000000000202 R12: 0000000000000000
[ 34.376245] R13: 00007ffc09d83608 R14: 00007fd71cae3000 R15: 000000000103cd90
[ 34.376661] </TASK>
[ 34.376794] Modules linked in: bpf_testmod(OE) aesni_intel(E)
crypto_simd(E) cryptd(E) kvm_intel(E) i2c_piix4(E) i2c_s)
[ 34.377900] CR2: ffffc90001c5bea0
[ 34.378097] ---[ end trace 0000000000000000 ]---
[ 34.378366] RIP: 0010:__wake_up_common+0x3e/0xc0
[ 34.378637] Code: 89 d4 55 53 48 89 fb 48 83 ec 08 8b 05 c3 05 a8
02 89 74 24 04 85 c0 75 6d 48 8b 43 48 48 83 c3 48 4f
[ 34.379703] RSP: 0018:ffffc900001d0d90 EFLAGS: 00010087
[ 34.380004] RAX: ffffc90001c5bea0 RBX: ffff88810138e0e8 RCX: 0000000000000001
[ 34.380414] RDX: 0000000080010005 RSI: ffffffff8295f82c RDI: ffffc90001c5be88
[ 34.380827] RBP: ffff88810138e0a0 R08: 0000000000000000 R09: 000000000000438f
[ 34.381284] R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000001
[ 34.381736] R13: 0000000000000046 R14: 0000000000000000 R15: 0000000000000000
[ 34.382198] FS: 00007fd71c7b3d00(0000) GS:ffff88881ca00000(0000)
knlGS:0000000000000000
[ 34.382712] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 34.383082] CR2: ffffc90001c5bea0 CR3: 000000010b456005 CR4: 0000000000370ef0
[ 34.383540] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 34.383991] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 34.384407] Kernel panic - not syncing: Fatal exception in interrupt
[ 35.464708] Shutting down cpus with NMI
[ 35.465244] Kernel Offset: disabled
[ 35.465474] ---[ end Kernel panic - not syncing: Fatal exception ininterrupt ]---
Quoted 13 lines · Andrii Nakryiko
+ bpf
Quoted 8 lines · Peter Zijlstra
Quoted 1 line · Oleg Nesterov
(Andrii, I'll try to look at your new series on Weekend).
OK, I dropped all your previous patches and stuffed these in.
They should all be visible in queue/perf/core, and provided the robot doesn't scream, I'll push them into tip/perf/core soonish.
Quoted 8 lines
Just FYI, it seems like tip/perf/core is currently broken for uprobes (and by implication also queue/perf/core). Also torvalds/linux/master master is broken. See what I'm getting when running BPF selftests dealing with uprobes. Sometimes I only get that WARNING and nothing else.
I'm bisecting at the moment with bpf/master being a "good" checkpoint, will let you know once I bisect.
Ok, this bisected to:
675ad74989c2 ("perf/core: Add aux_pause, aux_resume, aux_start_paused")
Reverting all (applied to tip/perf/core) four patches from that series:
6763ebdb4983 (tip/perf/core) perf/x86/intel: Do not enable large PEBS for events with aux actions or aux sampling6a45d8847597 perf/x86/intel/pt: Add support for pause / resume 675ad74989c2 perf/core: Add aux_pause, aux_resume, aux_start_paused d92792a4b26e perf/x86/intel/pt: Fix sampling synchronization
... makes everything work again. I'll leave it up to you and Adrian to figure this out.
Quoted 128 lines
[ 34.343557] ------------[ cut here ]------------ [ 34.343906] WARNING: CPU: 3 PID: 2364 at kernel/trace/trace_uprobe.c:1109 __probe_event_disable+0x26/0x80 [ 34.344468] Modules linked in: [ 34.344488] BUG: unable to handle page fault for address: ffffc90001c5bea0 [ 34.345071] #PF: supervisor read access in kernel mode [ 34.345370] #PF: error_code(0x0000) - not-present page [ 34.345700] PGD 100000067 P4D 100000067 PUD ffff88810d86cd40 [ 34.346061] Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI [ 34.346377] CPU: 3 UID: 0 PID: 2364 Comm: test_progs Tainted: G OE 6.11.0-rc1-00006-g6763ebdb4983 #115 [ 34.347052] Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE [ 34.347392] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04 [ 34.348085] RIP: 0010:__wake_up_common+0x3e/0xc0 [ 34.348359] Code: 89 d4 55 53 48 89 fb 48 83 ec 08 8b 05 c3 05 a8 02 89 74 24 04 85 c0 75 6d 48 8b 43 48 48 83 c3 48 4f [ 34.349440] RSP: 0018:ffffc900001d0d90 EFLAGS: 00010087 [ 34.349796] RAX: ffffc90001c5bea0 RBX: ffff88810138e0e8 RCX: 0000000000000001 [ 34.350282] RDX: 0000000080010005 RSI: ffffffff8295f82c RDI: ffffc90001c5be88 [ 34.350768] RBP: ffff88810138e0a0 R08: 0000000000000000 R09: 000000000000438f [ 34.351245] R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000001 [ 34.351703] R13: 0000000000000046 R14: 0000000000000000 R15: 0000000000000000 [ 34.352112] FS: 00007fd71c7b3d00(0000) GS:ffff88881ca00000(0000) knlGS:0000000000000000 [ 34.352574] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 34.352911] CR2: ffffc90001c5bea0 CR3: 000000010b456005 CR4: 0000000000370ef0 [ 34.353320] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 34.353734] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 34.354144] Call Trace: [ 34.354290] <IRQ> [ 34.354413] ? __die+0x1f/0x60 [ 34.354601] ? page_fault_oops+0x14c/0x450 [ 34.354848] ? search_bpf_extables+0xa8/0x150 [ 34.355105] ? fixup_exception+0x22/0x2d0 [ 34.355342] ? exc_page_fault+0x207/0x210 [ 34.355579] ? asm_exc_page_fault+0x22/0x30 [ 34.355829] ? __wake_up_common+0x3e/0xc0 [ 34.356065] __wake_up+0x32/0x60 [ 34.356261] ep_poll_callback+0x13e/0x270 [ 34.356502] __wake_up_common+0x7d/0xc0 [ 34.356731] __wake_up+0x32/0x60 [ 34.356961] irq_work_single+0x67/0x90 [ 34.357184] irq_work_run_list+0x26/0x40 [ 34.357442] update_process_times+0x83/0xa0 [ 34.357717] tick_nohz_handler+0x97/0x140 [ 34.357977] ? __pfx_tick_nohz_handler+0x10/0x10 [ 34.358283] __hrtimer_run_queues+0x19a/0x3b0 [ 34.358580] hrtimer_interrupt+0xfe/0x240 [ 34.358864] __sysvec_apic_timer_interrupt+0x87/0x210 [ 34.359189] sysvec_apic_timer_interrupt+0x98/0xc0 [ 34.359487] </IRQ> [ 34.359614] <TASK> [ 34.359745] asm_sysvec_apic_timer_interrupt+0x16/0x20 [ 34.360042] RIP: 0010:print_modules+0x27/0xd0 [ 34.360301] Code: 90 90 90 0f 1f 44 00 00 53 48 c7 c7 63 22 98 82 48 83 ec 18 e8 da 8d fc ff bf 01 00 00 00 e8 80 d1 f1 [ 34.361372] RSP: 0018:ffffc90001e5bc58 EFLAGS: 00000297 [ 34.361682] RAX: ffffffffa03f0948 RBX: 0000000000000000 RCX: 0000000000000000 [ 34.362091] RDX: 0000000000000002 RSI: 0000000000000027 RDI: 0000000000000001 [ 34.362502] RBP: ffffc90001e5bd28 R08: 00000000fffeffff R09: 0000000000000001 [ 34.362917] R10: 0000000000000000 R11: ffffffff83299920 R12: ffffffff81233536 [ 34.363325] R13: 0000000000000009 R14: 0000000000000455 R15: ffffffff8298f924 [ 34.363740] ? __probe_event_disable+0x26/0x80 [ 34.364009] ? print_modules+0x20/0xd0 [ 34.364230] ? __probe_event_disable+0x26/0x80 [ 34.364488] __warn+0x6f/0x180 [ 34.364683] ? __probe_event_disable+0x26/0x80 [ 34.364945] report_bug+0x18d/0x1c0 [ 34.365156] handle_bug+0x3a/0x70 [ 34.365354] exc_invalid_op+0x13/0x60 [ 34.365571] asm_exc_invalid_op+0x16/0x20 [ 34.365838] RIP: 0010:__probe_event_disable+0x26/0x80 [ 34.366175] Code: 90 90 90 90 55 48 89 fd 53 48 8b 47 10 8b 90 38 01 00 00 85 d2 75 13 48 8b 88 40 01 00 00 48 8d 90 40 [ 34.367340] RSP: 0018:ffffc90001e5bdd0 EFLAGS: 00010287 [ 34.367651] RAX: ffff88810d86cc00 RBX: ffff88810d86cce0 RCX: ffff8881075c8168 [ 34.368067] RDX: ffff88810d86cd40 RSI: 0000000000000003 RDI: ffff88810a5e1db0 [ 34.368482] RBP: ffff88810a5e1db0 R08: 00000000ffffffff R09: 0000000000000000 [ 34.368896] R10: 0000000000000001 R11: 0000000000000000 R12: ffff8881075c8250 [ 34.369310] R13: ffff8881075c82f0 R14: ffffc90001e5bb80 R15: ffff8881075c8000 [ 34.369775] trace_uprobe_register+0x1a8/0x300 [ 34.370053] perf_trace_event_unreg.isra.0+0x22/0x80 [ 34.370342] perf_uprobe_destroy+0x3a/0x70 [ 34.370582] _free_event+0x114/0x580 [ 34.370806] perf_event_release_kernel+0x282/0x2c0 [ 34.371123] perf_release+0x11/0x20 [ 34.371354] __fput+0x102/0x2e0 [ 34.371561] task_work_run+0x55/0xa0 [ 34.371798] syscall_exit_to_user_mode+0x1dd/0x1f0 [ 34.372104] do_syscall_64+0x70/0x140 [ 34.372337] entry_SYSCALL_64_after_hwframe+0x76/0x7e [ 34.372659] RIP: 0033:0x7fd71c986a94 [ 34.372901] Code: 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 80 3d d5 18 0e 00 00 73 [ 34.373987] RSP: 002b:00007ffc09d83118 EFLAGS: 00000202 ORIG_RAX: 0000000000000003 [ 34.374426] RAX: 0000000000000000 RBX: 00007ffc09d835e8 RCX: 00007fd71c986a94 [ 34.374874] RDX: 000000000000000b RSI: 0000000000002401 RDI: 000000000000000c [ 34.375325] RBP: 00007ffc09d83150 R08: 00000000049d39c7 R09: 0000000000000007 [ 34.375782] R10: 00000000049d71b0 R11: 0000000000000202 R12: 0000000000000000 [ 34.376245] R13: 00007ffc09d83608 R14: 00007fd71cae3000 R15: 000000000103cd90 [ 34.376661] </TASK> [ 34.376794] Modules linked in: bpf_testmod(OE) aesni_intel(E) crypto_simd(E) cryptd(E) kvm_intel(E) i2c_piix4(E) i2c_s) [ 34.377900] CR2: ffffc90001c5bea0 [ 34.378097] ---[ end trace 0000000000000000 ]--- [ 34.378366] RIP: 0010:__wake_up_common+0x3e/0xc0 [ 34.378637] Code: 89 d4 55 53 48 89 fb 48 83 ec 08 8b 05 c3 05 a8 02 89 74 24 04 85 c0 75 6d 48 8b 43 48 48 83 c3 48 4f [ 34.379703] RSP: 0018:ffffc900001d0d90 EFLAGS: 00010087 [ 34.380004] RAX: ffffc90001c5bea0 RBX: ffff88810138e0e8 RCX: 0000000000000001 [ 34.380414] RDX: 0000000080010005 RSI: ffffffff8295f82c RDI: ffffc90001c5be88 [ 34.380827] RBP: ffff88810138e0a0 R08: 0000000000000000 R09: 000000000000438f [ 34.381284] R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000001 [ 34.381736] R13: 0000000000000046 R14: 0000000000000000 R15: 0000000000000000 [ 34.382198] FS: 00007fd71c7b3d00(0000) GS:ffff88881ca00000(0000) knlGS:0000000000000000 [ 34.382712] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 34.383082] CR2: ffffc90001c5bea0 CR3: 000000010b456005 CR4: 0000000000370ef0 [ 34.383540] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 34.383991] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 34.384407] Kernel panic - not syncing: Fatal exception in interrupt [ 35.464708] Shutting down cpus with NMI [ 35.465244] Kernel Offset: disabled [ 35.465474] ---[ end Kernel panic - not syncing: Fatal exception ininterrupt ]---
Peter Zijlstra
maintainerQuoted 30 lines · Andrii Nakryiko
Quoted 13 lines · Andrii Nakryiko
+ bpf
Quoted 8 lines · Peter Zijlstra
Quoted 1 line · Oleg Nesterov
(Andrii, I'll try to look at your new series on Weekend).
OK, I dropped all your previous patches and stuffed these in.
They should all be visible in queue/perf/core, and provided the robot doesn't scream, I'll push them into tip/perf/core soonish.
Quoted 8 lines
Just FYI, it seems like tip/perf/core is currently broken for uprobes (and by implication also queue/perf/core). Also torvalds/linux/master master is broken. See what I'm getting when running BPF selftests dealing with uprobes. Sometimes I only get that WARNING and nothing else.
I'm bisecting at the moment with bpf/master being a "good" checkpoint, will let you know once I bisect.
Ok, this bisected to:
675ad74989c2("perf/core: Addaux_pause,aux_resume,aux_start_paused")Reverting all (applied to tip/perf/core) four patches from that series:
Quoted 8 lines
6763ebdb4983(tip/perf/core) perf/x86/intel: Do not enable large PEBS for events with aux actions or aux sampling6a45d8847597perf/x86/intel/pt: Add support for pause / resume675ad74989c2perf/core: Addaux_pause,aux_resume,aux_start_pausedd92792a4b26eperf/x86/intel/pt: Fix sampling synchronization... makes everything work again. I'll leave it up to you and Adrian to figure this out.
Thanks for catching this. I'll go have a look.
Peter Zijlstra
maintainerQuoted 3 lines · Andrii Nakryiko
Ok, this bisected to:
675ad74989c2("perf/core: Addaux_pause,aux_resume,aux_start_paused")
Adrian, there are at least two obvious bugs there:
- aux_action was key's off of PERF_PMU_CAP_AUX_OUTPUT, which is not right, that's the capability where events can output to AUX -- aka. PEBS-to-PT. It should be PERF_PMU_CAP_ITRACE, which is the PT/CoreSight thing.
- it sets aux_paused unconditionally, which is scribbling in the giant union which is overwriting state set by perf_init_event().
But I think there's more problems, we need to do the aux_action validation after perf_get_aux_event(), we can't know if having those bits set makes sense before that. This means the perf_event_alloc() site is wrong in the first place.
I'm going to drop these patches for now. Please rework.
Adrian Hunter
reviewerQuoted 16 lines · Peter Zijlstra
Quoted 3 lines · Andrii Nakryiko
Ok, this bisected to:
675ad74989c2("perf/core: Addaux_pause,aux_resume,aux_start_paused")Adrian, there are at least two obvious bugs there:
-
aux_actionwas key's off ofPERF_PMU_CAP_AUX_OUTPUT, which is not right, that's the capability where events can output to AUX -- aka. PEBS-to-PT. It should bePERF_PMU_CAP_ITRACE, which is the PT/CoreSight thing.- it sets
aux_pausedunconditionally, which is scribbling in the giant union which is overwriting state set byperf_init_event().
Quoted 6 lines
But I think there's more problems, we need to do the
aux_actionvalidation afterperf_get_aux_event(), we can't know if having those bits set makes sense before that. This means theperf_event_alloc()site is wrong in the first place.I'm going to drop these patches for now. Please rework.
Yes I will do that.
FWIW, I'd expect the reported issue would go away with just:
kernel/events/core.c +2 -1
@@ -12151,7 +12151,8 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu, err = -EOPNOTSUPP ; goto err_pmu; } - event->hw.aux_paused = event->attr.aux_start_paused; +if (event->attr.aux_start_paused) + event->hw.aux_paused =1 ;if (cgroup_fd !=-1 ) { err =perf_cgroup_connect (cgroup_fd, event, attr, group_leader);
Adrian Hunter
reviewerQuoted 5 lines · Adrian Hunter
Quoted 4 lines · Peter Zijlstra
Quoted 2 lines · Andrii Nakryiko
Ok, this bisected to:
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
675ad74989c2("perf/core: Addaux_pause,aux_resume,aux_start_paused")Adrian, there are at least two obvious bugs there:
-
aux_actionwas key's off ofPERF_PMU_CAP_AUX_OUTPUT, which is not right, that's the capability where events can output to AUX -- aka. PEBS-to-PT. It should bePERF_PMU_CAP_ITRACE, which is the PT/CoreSight thing.
Not sure about that.
In perf_event_alloc(), there is:
if (event->attr.aux_output &&
(!(pmu->capabilities & PERF_PMU_CAP_AUX_OUTPUT) ||
event->attr.aux_pause || event->attr.aux_resume)) {
err = -EOPNOTSUPP;
goto err_pmu;
}which is to prevent aux_output with aux_pause or aux_resume. That is because aux_output (i.e. PEBS-via-PT) has no interrupt and so does not overflow. (Instead the PEBS record is written by hardware to the Intel PT trace) No overflow => no (software) aux_pause/aux_resume, so aux_output with aux_pause/aux_resume does not make sense.
The PMU capability for aux_pause/aux_resume or aux_start_paused is PERF_PMU_CAP_AUX_PAUSE. aux_pause/aux_resume are valid for non-AUX events (member of the group), whereas aux_start_paused is valid for the AUX event itself (group leader). For aux_pause/aux_resume the group leader's PMU capability is checked. For aux_start_paused the event's PMU capability is checked.
Quoted 3 lines
Quoted 3 lines
- it sets
aux_pausedunconditionally, which is scribbling in the giant union which is overwriting state set byperf_init_event().
That definitely needs fixing, but the fix is just the diff from my previous reply:
kernel/events/core.c +2 -1
@@ -12151,7 +12151,8 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu, err = -EOPNOTSUPP ; goto err_pmu; } - event->hw.aux_paused = event->attr.aux_start_paused; +if (event->attr.aux_start_paused) + event->hw.aux_paused =1 ;if (cgroup_fd !=-1 ) { err =perf_cgroup_connect (cgroup_fd, event, attr, group_leader);
I tested that with:
# perf probe -x /root/main -a main
Added new event:
probe_main:main (on main in /root/main)# perf record -e probe_main:main -- ./main
and it made the problem go away.
Quoted 5 lines
Quoted 5 lines
But I think there's more problems, we need to do the
aux_actionvalidation afterperf_get_aux_event(), we can't know if having those bits set makes sense before that. This means theperf_event_alloc()site is wrong in the first place.
As above, aux_start_paused is used on the AUX event itself, so the PMU capability is checked in perf_event_alloc:
if (event->attr.aux_start_paused &&
!(pmu->capabilities & PERF_PMU_CAP_AUX_PAUSE)) {
err = -EOPNOTSUPP;
goto err_pmu;
}Whereas aux_pause/aux_resume are checked in perf_get_aux_event():
if ((event->attr.aux_pause || event->attr.aux_resume) && !(group_leader->pmu->capabilities & PERF_PMU_CAP_AUX_PAUSE)) return 0;
That all seems OK, so please let me know if there is something else to change.