Re: [PATCH v2 04/12] uprobes: revamp uprobe refcounting and lifetime management
From: Peter Zijlstra <peterz@infradead.org>
Date: 2024-07-04 08:45:35
Also in:
bpf
Subsystem:
bpf [general] (safe dynamic programs and tools), bpf [security & lsm] (security audit and enforcement using bpf), bpf [tracing], performance events subsystem, the rest, tracing, uprobes · Maintainers:
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi, KP Singh, Matt Bobrowski, Song Liu, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Linus Torvalds, Steven Rostedt, Masami Hiramatsu, Oleg Nesterov
On Thu, Jul 04, 2024 at 10:03:48AM +0200, Peter Zijlstra wrote:
quoted hunk ↗ jump to hunk
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c index c98e3b3386ba..4aafb4485be7 100644 --- a/kernel/trace/trace_uprobe.c +++ b/kernel/trace/trace_uprobe.c@@ -1112,7 +1112,8 @@ static void __probe_event_disable(struct trace_probe *tp) if (!tu->inode) continue; - uprobe_unregister(tu->inode, tu->offset, &tu->consumer); + uprobe_unregister(tu->inode, tu->offset, &tu->consumer, + list_is_last(trace_probe_probe_list(tp), &tu->tp.list) ? 0 : URF_NO_SYNC); tu->inode = NULL; } }
Hmm, that continue clause might ruin things. Still easy enough to add uprobe_unregister_sync() and simpy always pass URF_NO_SYNC. I really don't see why we should make this more complicated than it needs to be.
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 354cab634341..681741a51df3 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h@@ -115,7 +115,9 @@ extern int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm 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_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); +#define URF_NO_SYNC 0x01 +extern void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, unsigned int flags); +extern void uprobe_unregister_sync(void); 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);
@@ -165,7 +167,7 @@ uprobe_apply(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, boo return -ENOSYS; } static inline void -uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc) +uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, unsigned int flags) { } static inline int uprobe_mmap(struct vm_area_struct *vma)
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 0b7574a54093..d09f7b942076 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c@@ -1145,7 +1145,7 @@ __uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc) * @offset: offset from the start of the file. * @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 inode *inode, loff_t offset, struct uprobe_consumer *uc, unsigned int flags) { scoped_guard (srcu, &uprobe_srcu) { struct uprobe *uprobe = find_uprobe(inode, offset);
@@ -1157,10 +1157,17 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consume mutex_unlock(&uprobe->register_mutex); } - synchronize_srcu(&uprobe_srcu); // XXX amortize / batch + if (!(flags & URF_NO_SYNC)) + synchronize_srcu(&uprobe_srcu); } EXPORT_SYMBOL_GPL(uprobe_unregister); +void uprobe_unregister_sync(void) +{ + synchronize_srcu(&uprobe_srcu); +} +EXPORT_SYMBOL_GPL(uprobe_unregister_sync); + /* * __uprobe_register - register a probe * @inode: the file in which the probe has to be placed.
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index d1daeab1bbc1..1f6adabbb1e7 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c@@ -3181,9 +3181,10 @@ static void bpf_uprobe_unregister(struct path *path, struct bpf_uprobe *uprobes, u32 i; for (i = 0; i < cnt; i++) { - uprobe_unregister(d_real_inode(path->dentry), uprobes[i].offset, - &uprobes[i].consumer); + uprobe_unregister(d_real_inode(path->dentry), uprobes[i].offset, URF_NO_SYNC); } + if (cnt > 0) + uprobe_unregister_sync(); } static void bpf_uprobe_multi_link_release(struct bpf_link *link)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index c98e3b3386ba..6b64470a1c5c 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c@@ -1104,6 +1104,7 @@ static int trace_uprobe_enable(struct trace_uprobe *tu, filter_func_t filter) static void __probe_event_disable(struct trace_probe *tp) { struct trace_uprobe *tu; + bool sync = false; tu = container_of(tp, struct trace_uprobe, tp); WARN_ON(!uprobe_filter_is_empty(tu->tp.event->filter));
@@ -1112,9 +1113,12 @@ static void __probe_event_disable(struct trace_probe *tp) if (!tu->inode) continue; - uprobe_unregister(tu->inode, tu->offset, &tu->consumer); + uprobe_unregister(tu->inode, tu->offset, &tu->consumer, URF_NO_SYNC); + sync = true; tu->inode = NULL; } + if (sync) + uprobe_unregister_sync(); } static int probe_event_enable(struct trace_event_call *call,