| Message ID | 20240701223935.3783951-6-andrii@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | uprobes: add batched register/unregister APIs and per-CPU RW semaphore | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Mon, Jul 01, 2024 at 03:39:28PM -0700, Andrii Nakryiko wrote: > Simplify uprobe registration/unregistration interfaces by making offset > and ref_ctr_offset part of uprobe_consumer "interface". In practice, all > existing users already store these fields somewhere in uprobe_consumer's > containing structure, so this doesn't pose any problem. We just move > some fields around. > > On the other hand, this simplifies uprobe_register() and > uprobe_unregister() API by having only struct uprobe_consumer as one > thing representing attachment/detachment entity. This makes batched > versions of uprobe_register() and uprobe_unregister() simpler. > > This also makes uprobe_register_refctr() unnecessary, so remove it and > simplify consumers. > > No functional changes intended. > > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > --- > include/linux/uprobes.h | 18 +++---- > kernel/events/uprobes.c | 19 ++----- > kernel/trace/bpf_trace.c | 21 +++----- > kernel/trace/trace_uprobe.c | 53 ++++++++----------- > .../selftests/bpf/bpf_testmod/bpf_testmod.c | 22 ++++---- > 5 files changed, 55 insertions(+), 78 deletions(-) > > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h > index b503fafb7fb3..a75ba37ce3c8 100644 > --- a/include/linux/uprobes.h > +++ b/include/linux/uprobes.h > @@ -42,6 +42,11 @@ struct uprobe_consumer { > enum uprobe_filter_ctx ctx, > struct mm_struct *mm); > > + /* associated file offset of this probe */ > + loff_t offset; > + /* associated refctr file offset of this probe, or zero */ > + loff_t ref_ctr_offset; > + /* for internal uprobe infra use, consumers shouldn't touch fields below */ > struct uprobe_consumer *next; > }; > > @@ -110,10 +115,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, 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, 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 void uprobe_unregister(struct inode *inode, struct uprobe_consumer *uc); It seems very weird and unnatural to split inode and offset like this. The whole offset thing only makes sense within the context of an inode. So yeah, lets not do this.
On Wed, 3 Jul 2024 10:13:15 +0200 Peter Zijlstra <peterz@infradead.org> wrote: > On Mon, Jul 01, 2024 at 03:39:28PM -0700, Andrii Nakryiko wrote: > > Simplify uprobe registration/unregistration interfaces by making offset > > and ref_ctr_offset part of uprobe_consumer "interface". In practice, all > > existing users already store these fields somewhere in uprobe_consumer's > > containing structure, so this doesn't pose any problem. We just move > > some fields around. > > > > On the other hand, this simplifies uprobe_register() and > > uprobe_unregister() API by having only struct uprobe_consumer as one > > thing representing attachment/detachment entity. This makes batched > > versions of uprobe_register() and uprobe_unregister() simpler. > > > > This also makes uprobe_register_refctr() unnecessary, so remove it and > > simplify consumers. > > > > No functional changes intended. > > > > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > --- > > include/linux/uprobes.h | 18 +++---- > > kernel/events/uprobes.c | 19 ++----- > > kernel/trace/bpf_trace.c | 21 +++----- > > kernel/trace/trace_uprobe.c | 53 ++++++++----------- > > .../selftests/bpf/bpf_testmod/bpf_testmod.c | 22 ++++---- > > 5 files changed, 55 insertions(+), 78 deletions(-) > > > > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h > > index b503fafb7fb3..a75ba37ce3c8 100644 > > --- a/include/linux/uprobes.h > > +++ b/include/linux/uprobes.h > > @@ -42,6 +42,11 @@ struct uprobe_consumer { > > enum uprobe_filter_ctx ctx, > > struct mm_struct *mm); > > > > + /* associated file offset of this probe */ > > + loff_t offset; > > + /* associated refctr file offset of this probe, or zero */ > > + loff_t ref_ctr_offset; > > + /* for internal uprobe infra use, consumers shouldn't touch fields below */ > > struct uprobe_consumer *next; > > }; > > > > @@ -110,10 +115,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, 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, 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 void uprobe_unregister(struct inode *inode, struct uprobe_consumer *uc); > > It seems very weird and unnatural to split inode and offset like this. > The whole offset thing only makes sense within the context of an inode. Hm, so would you mean we should have inode inside the uprobe_consumer? If so, I think it is reasonable. Thank you, > > So yeah, lets not do this.
On Wed, Jul 3, 2024 at 3:13 AM Masami Hiramatsu <masami.hiramatsu@gmail.com> wrote: > > On Wed, 3 Jul 2024 10:13:15 +0200 > Peter Zijlstra <peterz@infradead.org> wrote: > > > On Mon, Jul 01, 2024 at 03:39:28PM -0700, Andrii Nakryiko wrote: > > > Simplify uprobe registration/unregistration interfaces by making offset > > > and ref_ctr_offset part of uprobe_consumer "interface". In practice, all > > > existing users already store these fields somewhere in uprobe_consumer's > > > containing structure, so this doesn't pose any problem. We just move > > > some fields around. > > > > > > On the other hand, this simplifies uprobe_register() and > > > uprobe_unregister() API by having only struct uprobe_consumer as one > > > thing representing attachment/detachment entity. This makes batched > > > versions of uprobe_register() and uprobe_unregister() simpler. > > > > > > This also makes uprobe_register_refctr() unnecessary, so remove it and > > > simplify consumers. > > > > > > No functional changes intended. > > > > > > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > > --- > > > include/linux/uprobes.h | 18 +++---- > > > kernel/events/uprobes.c | 19 ++----- > > > kernel/trace/bpf_trace.c | 21 +++----- > > > kernel/trace/trace_uprobe.c | 53 ++++++++----------- > > > .../selftests/bpf/bpf_testmod/bpf_testmod.c | 22 ++++---- > > > 5 files changed, 55 insertions(+), 78 deletions(-) > > > > > > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h > > > index b503fafb7fb3..a75ba37ce3c8 100644 > > > --- a/include/linux/uprobes.h > > > +++ b/include/linux/uprobes.h > > > @@ -42,6 +42,11 @@ struct uprobe_consumer { > > > enum uprobe_filter_ctx ctx, > > > struct mm_struct *mm); > > > > > > + /* associated file offset of this probe */ > > > + loff_t offset; > > > + /* associated refctr file offset of this probe, or zero */ > > > + loff_t ref_ctr_offset; > > > + /* for internal uprobe infra use, consumers shouldn't touch fields below */ > > > struct uprobe_consumer *next; > > > }; > > > > > > @@ -110,10 +115,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, 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, 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 void uprobe_unregister(struct inode *inode, struct uprobe_consumer *uc); > > > > It seems very weird and unnatural to split inode and offset like this. > > The whole offset thing only makes sense within the context of an inode. > > Hm, so would you mean we should have inode inside the uprobe_consumer? > If so, I think it is reasonable. > I don't think so, for at least two reasons. 1) We will be wasting 8 bytes per consumer saving exactly the same inode pointer for no good reason, while uprobe itself already stores this inode. One can argue that having offset and ref_ctr_offset inside uprobe_consumer is wasteful, in principle, and I agree. But all existing users already store them in the same struct that contains uprobe_consumer, so we are not regressing anything, while making the interface simpler. We can always optimize that further, if necessary, but right now that would give us nothing. But moving inode into uprobe_consumer will regress memory usage. 2) In the context of batched APIs, offset and ref_ctr_offset varies with each uprobe_consumer, while inode is explicitly the same for all consumers in that batch. This API makes it very clear. Technically, we can remove inode completely from *uprobe_unregister*, because we now can access it from uprobe_consumer->uprobe->inode, but it still feels right for symmetry and explicitly making a point that callers should ensure that inode is kept alive. > Thank you, > > > > > So yeah, lets not do this. > > > -- > Masami Hiramatsu
On 07/01, Andrii Nakryiko wrote: > > --- a/include/linux/uprobes.h > +++ b/include/linux/uprobes.h > @@ -42,6 +42,11 @@ struct uprobe_consumer { > enum uprobe_filter_ctx ctx, > struct mm_struct *mm); > > + /* associated file offset of this probe */ > + loff_t offset; > + /* associated refctr file offset of this probe, or zero */ > + loff_t ref_ctr_offset; > + /* for internal uprobe infra use, consumers shouldn't touch fields below */ > struct uprobe_consumer *next; Well, I don't really like this patch either... If nothing else because all the consumers in uprobe->consumers list must have the same offset/ref_ctr_offset. -------------------------------------------------------------------------- But I agree, the ugly uprobe_register_refctr() must die, we need a single function int uprobe_register(inode, offset, ref_ctr_offset, consumer); This change is trivial. -------------------------------------------------------------------------- And speaking of cleanups, I think another change makes sense: - int uprobe_register(...); + struct uprobe* uprobe_register(...); so that uprobe_register() returns uprobe or ERR_PTR. - void uprobe_unregister(inode, offset, consumer); + void uprobe_unregister(uprobe, consumer); this way unregister() doesn't need the extra find_uprobe() + put_uprobe(). The same for uprobe_apply(). The necessary changes in kernel/trace/trace_uprobe.c are trivial, we just need to change struct trace_uprobe - struct inode *inode; + struct uprobe *uprobe; and fix the compilation errors. As for kernel/trace/bpf_trace.c, I guess struct bpf_uprobe needs the new ->uprobe member, we can't kill bpf_uprobe->offset because of bpf_uprobe_multi_link_fill_link_info(), but I think this is not that bad. What do you think? Oleg.
On Sun, Jul 7, 2024 at 5:50 AM Oleg Nesterov <oleg@redhat.com> wrote: > > On 07/01, Andrii Nakryiko wrote: > > > > --- a/include/linux/uprobes.h > > +++ b/include/linux/uprobes.h > > @@ -42,6 +42,11 @@ struct uprobe_consumer { > > enum uprobe_filter_ctx ctx, > > struct mm_struct *mm); > > > > + /* associated file offset of this probe */ > > + loff_t offset; > > + /* associated refctr file offset of this probe, or zero */ > > + loff_t ref_ctr_offset; > > + /* for internal uprobe infra use, consumers shouldn't touch fields below */ > > struct uprobe_consumer *next; > > > Well, I don't really like this patch either... > > If nothing else because all the consumers in uprobe->consumers list > must have the same offset/ref_ctr_offset. You are thinking from a per-uprobe's perspective. But during attachment you are attaching multiple consumers at different locations within a given inode (and that matches for consumers are already doing, they remember those offsets in their own structs), so each consumer has a different offset. Again, I'm just saying that I'm codifying what uprobe users already do and simplifying the interface (otherwise we'd need another set of callbacks or some new struct just to pass those offsets/ref_ctr_offset). But we can put all that on hold if Peter's approach works well enough. My goal is to have faster uprobes, not to land *my* patches. > > -------------------------------------------------------------------------- > But I agree, the ugly uprobe_register_refctr() must die, we need a single > function > > int uprobe_register(inode, offset, ref_ctr_offset, consumer); > > This change is trivial. > > -------------------------------------------------------------------------- > And speaking of cleanups, I think another change makes sense: > > - int uprobe_register(...); > + struct uprobe* uprobe_register(...); > > so that uprobe_register() returns uprobe or ERR_PTR. > > - void uprobe_unregister(inode, offset, consumer); > + void uprobe_unregister(uprobe, consumer); > > this way unregister() doesn't need the extra find_uprobe() + put_uprobe(). > The same for uprobe_apply(). I'm achieving this by keeping uprobe pointer inside uprobe_consumer (and not requiring callers to keep explicit track of that) > > The necessary changes in kernel/trace/trace_uprobe.c are trivial, we just > need to change struct trace_uprobe > > - struct inode *inode; > + struct uprobe *uprobe; > > and fix the compilation errors. > > > As for kernel/trace/bpf_trace.c, I guess struct bpf_uprobe needs the new > ->uprobe member, we can't kill bpf_uprobe->offset because of > bpf_uprobe_multi_link_fill_link_info(), but I think this is not that bad. > > What do you think? I'd add an uprobe field to uprobe_consumer, tbh, and keep callers simpler (less aware of uprobe existence in principle). Even if we don't do batch register/unregister APIs. > > Oleg. >
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h index b503fafb7fb3..a75ba37ce3c8 100644 --- a/include/linux/uprobes.h +++ b/include/linux/uprobes.h @@ -42,6 +42,11 @@ struct uprobe_consumer { enum uprobe_filter_ctx ctx, struct mm_struct *mm); + /* associated file offset of this probe */ + loff_t offset; + /* associated refctr file offset of this probe, or zero */ + loff_t ref_ctr_offset; + /* for internal uprobe infra use, consumers shouldn't touch fields below */ struct uprobe_consumer *next; }; @@ -110,10 +115,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, 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, 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 void uprobe_unregister(struct inode *inode, 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,11 +156,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, struct uprobe_consumer *uc) { return -ENOSYS; } @@ -166,7 +166,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, 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 560cf1ca512a..8759c6d0683e 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -1224,14 +1224,13 @@ __uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc) /* * 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. - * @uc: identify which probe if multiple probes are colocated. + * @uc: identify which probe consumer to unregister. */ -void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc) +void uprobe_unregister(struct inode *inode, struct uprobe_consumer *uc) { struct uprobe *uprobe; - uprobe = find_uprobe(inode, offset); + uprobe = find_uprobe(inode, uc->offset); if (WARN_ON(!uprobe)) return; @@ -1304,20 +1303,12 @@ static int __uprobe_register(struct inode *inode, loff_t offset, return ret; } -int uprobe_register(struct inode *inode, loff_t offset, - struct uprobe_consumer *uc) +int uprobe_register(struct inode *inode, struct uprobe_consumer *uc) { - return __uprobe_register(inode, offset, 0, uc); + return __uprobe_register(inode, uc->offset, uc->ref_ctr_offset, uc); } EXPORT_SYMBOL_GPL(uprobe_register); -int uprobe_register_refctr(struct inode *inode, loff_t offset, - loff_t ref_ctr_offset, struct uprobe_consumer *uc) -{ - return __uprobe_register(inode, offset, ref_ctr_offset, uc); -} -EXPORT_SYMBOL_GPL(uprobe_register_refctr); - /* * uprobe_apply - unregister an already registered probe. * @inode: the file in which the probe has to be removed. diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index d1daeab1bbc1..ba62baec3152 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -3154,8 +3154,6 @@ struct bpf_uprobe_multi_link; struct bpf_uprobe { struct bpf_uprobe_multi_link *link; - loff_t offset; - unsigned long ref_ctr_offset; u64 cookie; struct uprobe_consumer consumer; }; @@ -3181,8 +3179,7 @@ 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].consumer); } } @@ -3262,10 +3259,10 @@ static int bpf_uprobe_multi_link_fill_link_info(const struct bpf_link *link, for (i = 0; i < ucount; i++) { if (uoffsets && - put_user(umulti_link->uprobes[i].offset, uoffsets + i)) + put_user(umulti_link->uprobes[i].consumer.offset, uoffsets + i)) return -EFAULT; if (uref_ctr_offsets && - put_user(umulti_link->uprobes[i].ref_ctr_offset, uref_ctr_offsets + i)) + put_user(umulti_link->uprobes[i].consumer.ref_ctr_offset, uref_ctr_offsets + i)) return -EFAULT; if (ucookies && put_user(umulti_link->uprobes[i].cookie, ucookies + i)) @@ -3439,15 +3436,16 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr goto error_free; for (i = 0; i < cnt; i++) { - if (__get_user(uprobes[i].offset, uoffsets + i)) { + if (__get_user(uprobes[i].consumer.offset, uoffsets + i)) { err = -EFAULT; goto error_free; } - if (uprobes[i].offset < 0) { + if (uprobes[i].consumer.offset < 0) { err = -EINVAL; goto error_free; } - if (uref_ctr_offsets && __get_user(uprobes[i].ref_ctr_offset, uref_ctr_offsets + i)) { + if (uref_ctr_offsets && + __get_user(uprobes[i].consumer.ref_ctr_offset, uref_ctr_offsets + i)) { err = -EFAULT; goto error_free; } @@ -3477,10 +3475,7 @@ 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].consumer); if (err) { bpf_uprobe_unregister(&path, uprobes, i); goto error_free; diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c index c98e3b3386ba..d786f99114be 100644 --- a/kernel/trace/trace_uprobe.c +++ b/kernel/trace/trace_uprobe.c @@ -60,8 +60,6 @@ struct trace_uprobe { struct path path; struct inode *inode; char *filename; - unsigned long offset; - unsigned long ref_ctr_offset; unsigned long nhit; struct trace_probe tp; }; @@ -205,7 +203,7 @@ static unsigned long translate_user_vaddr(unsigned long file_offset) udd = (void *) current->utask->vaddr; - base_addr = udd->bp_addr - udd->tu->offset; + base_addr = udd->bp_addr - udd->tu->consumer.offset; return base_addr + file_offset; } @@ -286,13 +284,13 @@ static bool trace_uprobe_match_command_head(struct trace_uprobe *tu, if (strncmp(tu->filename, argv[0], len) || argv[0][len] != ':') return false; - if (tu->ref_ctr_offset == 0) - snprintf(buf, sizeof(buf), "0x%0*lx", - (int)(sizeof(void *) * 2), tu->offset); + if (tu->consumer.ref_ctr_offset == 0) + snprintf(buf, sizeof(buf), "0x%0*llx", + (int)(sizeof(void *) * 2), tu->consumer.offset); else - snprintf(buf, sizeof(buf), "0x%0*lx(0x%lx)", - (int)(sizeof(void *) * 2), tu->offset, - tu->ref_ctr_offset); + snprintf(buf, sizeof(buf), "0x%0*llx(0x%llx)", + (int)(sizeof(void *) * 2), tu->consumer.offset, + tu->consumer.ref_ctr_offset); if (strcmp(buf, &argv[0][len + 1])) return false; @@ -410,7 +408,7 @@ static bool trace_uprobe_has_same_uprobe(struct trace_uprobe *orig, list_for_each_entry(orig, &tpe->probes, tp.list) { if (comp_inode != d_real_inode(orig->path.dentry) || - comp->offset != orig->offset) + comp->consumer.offset != orig->consumer.offset) continue; /* @@ -472,8 +470,8 @@ static int validate_ref_ctr_offset(struct trace_uprobe *new) for_each_trace_uprobe(tmp, pos) { if (new_inode == d_real_inode(tmp->path.dentry) && - new->offset == tmp->offset && - new->ref_ctr_offset != tmp->ref_ctr_offset) { + new->consumer.offset == tmp->consumer.offset && + new->consumer.ref_ctr_offset != tmp->consumer.ref_ctr_offset) { pr_warn("Reference counter offset mismatch."); return -EINVAL; } @@ -675,8 +673,8 @@ static int __trace_uprobe_create(int argc, const char **argv) WARN_ON_ONCE(ret != -ENOMEM); goto fail_address_parse; } - tu->offset = offset; - tu->ref_ctr_offset = ref_ctr_offset; + tu->consumer.offset = offset; + tu->consumer.ref_ctr_offset = ref_ctr_offset; tu->path = path; tu->filename = filename; @@ -746,12 +744,12 @@ static int trace_uprobe_show(struct seq_file *m, struct dyn_event *ev) char c = is_ret_probe(tu) ? 'r' : 'p'; int i; - seq_printf(m, "%c:%s/%s %s:0x%0*lx", c, trace_probe_group_name(&tu->tp), + seq_printf(m, "%c:%s/%s %s:0x%0*llx", c, trace_probe_group_name(&tu->tp), trace_probe_name(&tu->tp), tu->filename, - (int)(sizeof(void *) * 2), tu->offset); + (int)(sizeof(void *) * 2), tu->consumer.offset); - if (tu->ref_ctr_offset) - seq_printf(m, "(0x%lx)", tu->ref_ctr_offset); + if (tu->consumer.ref_ctr_offset) + seq_printf(m, "(0x%llx)", tu->consumer.ref_ctr_offset); for (i = 0; i < tu->tp.nr_args; i++) seq_printf(m, " %s=%s", tu->tp.args[i].name, tu->tp.args[i].comm); @@ -1089,12 +1087,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->consumer); if (ret) tu->inode = NULL; @@ -1112,7 +1105,7 @@ 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->consumer); tu->inode = NULL; } } @@ -1310,7 +1303,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->inode, tu->consumer.offset, &tu->consumer, false); if (ret) break; } @@ -1334,7 +1327,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->inode, tu->consumer.offset, &tu->consumer, true); if (err) { uprobe_perf_close(call, event); break; @@ -1464,7 +1457,7 @@ int bpf_get_uprobe_info(const struct perf_event *event, u32 *fd_type, *fd_type = is_ret_probe(tu) ? BPF_FD_TYPE_URETPROBE : BPF_FD_TYPE_UPROBE; *filename = tu->filename; - *probe_offset = tu->offset; + *probe_offset = tu->consumer.offset; *probe_addr = 0; return 0; } @@ -1627,9 +1620,9 @@ create_local_trace_uprobe(char *name, unsigned long offs, return ERR_CAST(tu); } - tu->offset = offs; + tu->consumer.offset = offs; tu->path = path; - tu->ref_ctr_offset = ref_ctr_offset; + tu->consumer.ref_ctr_offset = ref_ctr_offset; tu->filename = kstrdup(name, GFP_KERNEL); if (!tu->filename) { ret = -ENOMEM; diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c index b0132a342bb5..ca7122cdbcd3 100644 --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c @@ -391,25 +391,24 @@ static int testmod_register_uprobe(loff_t offset) { int err = -EBUSY; - if (uprobe.offset) + if (uprobe.consumer.offset) return -EBUSY; mutex_lock(&testmod_uprobe_mutex); - if (uprobe.offset) + if (uprobe.consumer.offset) 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.consumer.offset = offset; + err = uprobe_register(d_real_inode(uprobe.path.dentry), &uprobe.consumer); + if (err) { path_put(&uprobe.path); - else - uprobe.offset = offset; - + uprobe.consumer.offset = 0; + } out: mutex_unlock(&testmod_uprobe_mutex); return err; @@ -419,10 +418,9 @@ 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.consumer.offset) { + uprobe_unregister(d_real_inode(uprobe.path.dentry), &uprobe.consumer); + uprobe.consumer.offset = 0; } mutex_unlock(&testmod_uprobe_mutex);