On Mon, 24 Jun 2024 17:21:37 -0700
Andrii Nakryiko <andrii@xxxxxxxxxx> 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.
>
Looks good to me.
Acked-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
Thanks!
> Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> ---
> 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 | 23 ++++----
> 5 files changed, 55 insertions(+), 79 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);
> 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 8ce669bc6474..2544e8b79bad 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1197,14 +1197,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;
>
> @@ -1277,20 +1276,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..9ae2a3c64daa 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -377,7 +377,6 @@ uprobe_ret_handler(struct uprobe_consumer *self, unsigned long func,
>
> struct testmod_uprobe {
> struct path path;
> - loff_t offset;
> struct uprobe_consumer consumer;
> };
>
> @@ -391,25 +390,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 +417,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);
> --
> 2.43.0
>
--
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
