| Message ID | 20220104080943.113249-3-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | BPF |
| Headers | show |
| Series | kprobe/bpf: Add support to attach multiple kprobes | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next | fail | VM_Test |
| bpf/vmtest-bpf-next-PR | fail | PR summary |
| netdev/tree_selection | success | Not a local patch, async |
On Tue, 4 Jan 2022 09:09:32 +0100 Jiri Olsa <jolsa@redhat.com> wrote: > The bpf_get_func_ip_kprobe helper should return traced function > address, but it's doing so only for kprobes that are placed on > the function entry. > > If kprobe is placed within the function, bpf_get_func_ip_kprobe > returns that address instead of function entry. > > Storing the function entry directly in kprobe object, so it could > be used in bpf_get_func_ip_kprobe helper. Hmm, please do this in bpf side, which should have some data structure around the kprobe itself. Do not add this "specialized" field to the kprobe data structure. Thank you, > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > include/linux/kprobes.h | 3 +++ > kernel/kprobes.c | 12 ++++++++++++ > kernel/trace/bpf_trace.c | 2 +- > tools/testing/selftests/bpf/progs/get_func_ip_test.c | 4 ++-- > 4 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h > index 8c8f7a4d93af..a204df4fef96 100644 > --- a/include/linux/kprobes.h > +++ b/include/linux/kprobes.h > @@ -74,6 +74,9 @@ struct kprobe { > /* Offset into the symbol */ > unsigned int offset; > > + /* traced function address */ > + unsigned long func_addr; > + > /* Called before addr is executed. */ > kprobe_pre_handler_t pre_handler; > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c > index d20ae8232835..c4060a8da050 100644 > --- a/kernel/kprobes.c > +++ b/kernel/kprobes.c > @@ -1310,6 +1310,7 @@ static void init_aggr_kprobe(struct kprobe *ap, struct kprobe *p) > copy_kprobe(p, ap); > flush_insn_slot(ap); > ap->addr = p->addr; > + ap->func_addr = p->func_addr; > ap->flags = p->flags & ~KPROBE_FLAG_OPTIMIZED; > ap->pre_handler = aggr_pre_handler; > /* We don't care the kprobe which has gone. */ > @@ -1588,6 +1589,16 @@ static int check_kprobe_address_safe(struct kprobe *p, > return ret; > } > > +static unsigned long resolve_func_addr(kprobe_opcode_t *addr) > +{ > + char str[KSYM_SYMBOL_LEN]; > + unsigned long offset; > + > + if (kallsyms_lookup((unsigned long) addr, NULL, &offset, NULL, str)) > + return (unsigned long) addr - offset; > + return 0; > +} > + > int register_kprobe(struct kprobe *p) > { > int ret; > @@ -1600,6 +1611,7 @@ int register_kprobe(struct kprobe *p) > if (IS_ERR(addr)) > return PTR_ERR(addr); > p->addr = addr; > + p->func_addr = resolve_func_addr(addr); > > ret = warn_kprobe_rereg(p); > if (ret) > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 21aa30644219..25631253084a 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -1026,7 +1026,7 @@ BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs) > { > struct kprobe *kp = kprobe_running(); > > - return kp ? (uintptr_t)kp->addr : 0; > + return kp ? (uintptr_t)kp->func_addr : 0; > } > > static const struct bpf_func_proto bpf_get_func_ip_proto_kprobe = { > diff --git a/tools/testing/selftests/bpf/progs/get_func_ip_test.c b/tools/testing/selftests/bpf/progs/get_func_ip_test.c > index a587aeca5ae0..e988aefa567e 100644 > --- a/tools/testing/selftests/bpf/progs/get_func_ip_test.c > +++ b/tools/testing/selftests/bpf/progs/get_func_ip_test.c > @@ -69,7 +69,7 @@ int test6(struct pt_regs *ctx) > { > __u64 addr = bpf_get_func_ip(ctx); > > - test6_result = (const void *) addr == &bpf_fentry_test6 + 5; > + test6_result = (const void *) addr == &bpf_fentry_test6; > return 0; > } > > @@ -79,6 +79,6 @@ int test7(struct pt_regs *ctx) > { > __u64 addr = bpf_get_func_ip(ctx); > > - test7_result = (const void *) addr == &bpf_fentry_test7 + 5; > + test7_result = (const void *) addr == &bpf_fentry_test7; > return 0; > } > -- > 2.33.1 >
On Tue, Jan 4, 2022 at 12:10 AM Jiri Olsa <jolsa@redhat.com> wrote: > > The bpf_get_func_ip_kprobe helper should return traced function > address, but it's doing so only for kprobes that are placed on > the function entry. > > If kprobe is placed within the function, bpf_get_func_ip_kprobe > returns that address instead of function entry. > > Storing the function entry directly in kprobe object, so it could > be used in bpf_get_func_ip_kprobe helper. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > include/linux/kprobes.h | 3 +++ > kernel/kprobes.c | 12 ++++++++++++ > kernel/trace/bpf_trace.c | 2 +- > tools/testing/selftests/bpf/progs/get_func_ip_test.c | 4 ++-- > 4 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h > index 8c8f7a4d93af..a204df4fef96 100644 > --- a/include/linux/kprobes.h > +++ b/include/linux/kprobes.h > @@ -74,6 +74,9 @@ struct kprobe { > /* Offset into the symbol */ > unsigned int offset; > > + /* traced function address */ > + unsigned long func_addr; > + keep in mind that we'll also need (maybe in a follow up series) to store bpf_cookie somewhere close to this func_addr as well. Just mentioning to keep in mind as you decide with Masami where to put it. > /* Called before addr is executed. */ > kprobe_pre_handler_t pre_handler; > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c > index d20ae8232835..c4060a8da050 100644 > --- a/kernel/kprobes.c > +++ b/kernel/kprobes.c > @@ -1310,6 +1310,7 @@ static void init_aggr_kprobe(struct kprobe *ap, struct kprobe *p) > copy_kprobe(p, ap); > flush_insn_slot(ap); > ap->addr = p->addr; > + ap->func_addr = p->func_addr; > ap->flags = p->flags & ~KPROBE_FLAG_OPTIMIZED; > ap->pre_handler = aggr_pre_handler; > /* We don't care the kprobe which has gone. */ > @@ -1588,6 +1589,16 @@ static int check_kprobe_address_safe(struct kprobe *p, > return ret; > } > > +static unsigned long resolve_func_addr(kprobe_opcode_t *addr) > +{ > + char str[KSYM_SYMBOL_LEN]; > + unsigned long offset; > + > + if (kallsyms_lookup((unsigned long) addr, NULL, &offset, NULL, str)) > + return (unsigned long) addr - offset; > + return 0; > +} > + > int register_kprobe(struct kprobe *p) > { > int ret; > @@ -1600,6 +1611,7 @@ int register_kprobe(struct kprobe *p) > if (IS_ERR(addr)) > return PTR_ERR(addr); > p->addr = addr; > + p->func_addr = resolve_func_addr(addr); > > ret = warn_kprobe_rereg(p); > if (ret) > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 21aa30644219..25631253084a 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -1026,7 +1026,7 @@ BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs) > { > struct kprobe *kp = kprobe_running(); > > - return kp ? (uintptr_t)kp->addr : 0; > + return kp ? (uintptr_t)kp->func_addr : 0; > } > > static const struct bpf_func_proto bpf_get_func_ip_proto_kprobe = { > diff --git a/tools/testing/selftests/bpf/progs/get_func_ip_test.c b/tools/testing/selftests/bpf/progs/get_func_ip_test.c > index a587aeca5ae0..e988aefa567e 100644 > --- a/tools/testing/selftests/bpf/progs/get_func_ip_test.c > +++ b/tools/testing/selftests/bpf/progs/get_func_ip_test.c > @@ -69,7 +69,7 @@ int test6(struct pt_regs *ctx) > { > __u64 addr = bpf_get_func_ip(ctx); > > - test6_result = (const void *) addr == &bpf_fentry_test6 + 5; > + test6_result = (const void *) addr == &bpf_fentry_test6; > return 0; > } > > @@ -79,6 +79,6 @@ int test7(struct pt_regs *ctx) > { > __u64 addr = bpf_get_func_ip(ctx); > > - test7_result = (const void *) addr == &bpf_fentry_test7 + 5; > + test7_result = (const void *) addr == &bpf_fentry_test7; we can treat this as a bug fix for bpf_get_func_ip() for kprobes, right? I think "Fixes: " tag is in order then. > return 0; > } > -- > 2.33.1 >
On Wed, Jan 05, 2022 at 11:32:52PM +0900, Masami Hiramatsu wrote: > On Tue, 4 Jan 2022 09:09:32 +0100 > Jiri Olsa <jolsa@redhat.com> wrote: > > > The bpf_get_func_ip_kprobe helper should return traced function > > address, but it's doing so only for kprobes that are placed on > > the function entry. > > > > If kprobe is placed within the function, bpf_get_func_ip_kprobe > > returns that address instead of function entry. > > > > Storing the function entry directly in kprobe object, so it could > > be used in bpf_get_func_ip_kprobe helper. > > Hmm, please do this in bpf side, which should have some data structure > around the kprobe itself. Do not add this "specialized" field to > the kprobe data structure. ok, will check thanks, jirka > > Thank you, > > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > include/linux/kprobes.h | 3 +++ > > kernel/kprobes.c | 12 ++++++++++++ > > kernel/trace/bpf_trace.c | 2 +- > > tools/testing/selftests/bpf/progs/get_func_ip_test.c | 4 ++-- > > 4 files changed, 18 insertions(+), 3 deletions(-) > > > > diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h > > index 8c8f7a4d93af..a204df4fef96 100644 > > --- a/include/linux/kprobes.h > > +++ b/include/linux/kprobes.h > > @@ -74,6 +74,9 @@ struct kprobe { > > /* Offset into the symbol */ > > unsigned int offset; > > > > + /* traced function address */ > > + unsigned long func_addr; > > + > > /* Called before addr is executed. */ > > kprobe_pre_handler_t pre_handler; > > > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c > > index d20ae8232835..c4060a8da050 100644 > > --- a/kernel/kprobes.c > > +++ b/kernel/kprobes.c > > @@ -1310,6 +1310,7 @@ static void init_aggr_kprobe(struct kprobe *ap, struct kprobe *p) > > copy_kprobe(p, ap); > > flush_insn_slot(ap); > > ap->addr = p->addr; > > + ap->func_addr = p->func_addr; > > ap->flags = p->flags & ~KPROBE_FLAG_OPTIMIZED; > > ap->pre_handler = aggr_pre_handler; > > /* We don't care the kprobe which has gone. */ > > @@ -1588,6 +1589,16 @@ static int check_kprobe_address_safe(struct kprobe *p, > > return ret; > > } > > > > +static unsigned long resolve_func_addr(kprobe_opcode_t *addr) > > +{ > > + char str[KSYM_SYMBOL_LEN]; > > + unsigned long offset; > > + > > + if (kallsyms_lookup((unsigned long) addr, NULL, &offset, NULL, str)) > > + return (unsigned long) addr - offset; > > + return 0; > > +} > > + > > int register_kprobe(struct kprobe *p) > > { > > int ret; > > @@ -1600,6 +1611,7 @@ int register_kprobe(struct kprobe *p) > > if (IS_ERR(addr)) > > return PTR_ERR(addr); > > p->addr = addr; > > + p->func_addr = resolve_func_addr(addr); > > > > ret = warn_kprobe_rereg(p); > > if (ret) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > > index 21aa30644219..25631253084a 100644 > > --- a/kernel/trace/bpf_trace.c > > +++ b/kernel/trace/bpf_trace.c > > @@ -1026,7 +1026,7 @@ BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs) > > { > > struct kprobe *kp = kprobe_running(); > > > > - return kp ? (uintptr_t)kp->addr : 0; > > + return kp ? (uintptr_t)kp->func_addr : 0; > > } > > > > static const struct bpf_func_proto bpf_get_func_ip_proto_kprobe = { > > diff --git a/tools/testing/selftests/bpf/progs/get_func_ip_test.c b/tools/testing/selftests/bpf/progs/get_func_ip_test.c > > index a587aeca5ae0..e988aefa567e 100644 > > --- a/tools/testing/selftests/bpf/progs/get_func_ip_test.c > > +++ b/tools/testing/selftests/bpf/progs/get_func_ip_test.c > > @@ -69,7 +69,7 @@ int test6(struct pt_regs *ctx) > > { > > __u64 addr = bpf_get_func_ip(ctx); > > > > - test6_result = (const void *) addr == &bpf_fentry_test6 + 5; > > + test6_result = (const void *) addr == &bpf_fentry_test6; > > return 0; > > } > > > > @@ -79,6 +79,6 @@ int test7(struct pt_regs *ctx) > > { > > __u64 addr = bpf_get_func_ip(ctx); > > > > - test7_result = (const void *) addr == &bpf_fentry_test7 + 5; > > + test7_result = (const void *) addr == &bpf_fentry_test7; > > return 0; > > } > > -- > > 2.33.1 > > > > > -- > Masami Hiramatsu <mhiramat@kernel.org> >
On Wed, Jan 05, 2022 at 08:30:48PM -0800, Andrii Nakryiko wrote: > On Tue, Jan 4, 2022 at 12:10 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > The bpf_get_func_ip_kprobe helper should return traced function > > address, but it's doing so only for kprobes that are placed on > > the function entry. > > > > If kprobe is placed within the function, bpf_get_func_ip_kprobe > > returns that address instead of function entry. > > > > Storing the function entry directly in kprobe object, so it could > > be used in bpf_get_func_ip_kprobe helper. > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > include/linux/kprobes.h | 3 +++ > > kernel/kprobes.c | 12 ++++++++++++ > > kernel/trace/bpf_trace.c | 2 +- > > tools/testing/selftests/bpf/progs/get_func_ip_test.c | 4 ++-- > > 4 files changed, 18 insertions(+), 3 deletions(-) > > > > diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h > > index 8c8f7a4d93af..a204df4fef96 100644 > > --- a/include/linux/kprobes.h > > +++ b/include/linux/kprobes.h > > @@ -74,6 +74,9 @@ struct kprobe { > > /* Offset into the symbol */ > > unsigned int offset; > > > > + /* traced function address */ > > + unsigned long func_addr; > > + > > keep in mind that we'll also need (maybe in a follow up series) to > store bpf_cookie somewhere close to this func_addr as well. Just > mentioning to keep in mind as you decide with Masami where to put it. ok SNIP > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > > index 21aa30644219..25631253084a 100644 > > --- a/kernel/trace/bpf_trace.c > > +++ b/kernel/trace/bpf_trace.c > > @@ -1026,7 +1026,7 @@ BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs) > > { > > struct kprobe *kp = kprobe_running(); > > > > - return kp ? (uintptr_t)kp->addr : 0; > > + return kp ? (uintptr_t)kp->func_addr : 0; > > } > > > > static const struct bpf_func_proto bpf_get_func_ip_proto_kprobe = { > > diff --git a/tools/testing/selftests/bpf/progs/get_func_ip_test.c b/tools/testing/selftests/bpf/progs/get_func_ip_test.c > > index a587aeca5ae0..e988aefa567e 100644 > > --- a/tools/testing/selftests/bpf/progs/get_func_ip_test.c > > +++ b/tools/testing/selftests/bpf/progs/get_func_ip_test.c > > @@ -69,7 +69,7 @@ int test6(struct pt_regs *ctx) > > { > > __u64 addr = bpf_get_func_ip(ctx); > > > > - test6_result = (const void *) addr == &bpf_fentry_test6 + 5; > > + test6_result = (const void *) addr == &bpf_fentry_test6; > > return 0; > > } > > > > @@ -79,6 +79,6 @@ int test7(struct pt_regs *ctx) > > { > > __u64 addr = bpf_get_func_ip(ctx); > > > > - test7_result = (const void *) addr == &bpf_fentry_test7 + 5; > > + test7_result = (const void *) addr == &bpf_fentry_test7; > > we can treat this as a bug fix for bpf_get_func_ip() for kprobes, > right? I think "Fixes: " tag is in order then. true, will add that in next version thanks, jirka
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h index 8c8f7a4d93af..a204df4fef96 100644 --- a/include/linux/kprobes.h +++ b/include/linux/kprobes.h @@ -74,6 +74,9 @@ struct kprobe { /* Offset into the symbol */ unsigned int offset; + /* traced function address */ + unsigned long func_addr; + /* Called before addr is executed. */ kprobe_pre_handler_t pre_handler; diff --git a/kernel/kprobes.c b/kernel/kprobes.c index d20ae8232835..c4060a8da050 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -1310,6 +1310,7 @@ static void init_aggr_kprobe(struct kprobe *ap, struct kprobe *p) copy_kprobe(p, ap); flush_insn_slot(ap); ap->addr = p->addr; + ap->func_addr = p->func_addr; ap->flags = p->flags & ~KPROBE_FLAG_OPTIMIZED; ap->pre_handler = aggr_pre_handler; /* We don't care the kprobe which has gone. */ @@ -1588,6 +1589,16 @@ static int check_kprobe_address_safe(struct kprobe *p, return ret; } +static unsigned long resolve_func_addr(kprobe_opcode_t *addr) +{ + char str[KSYM_SYMBOL_LEN]; + unsigned long offset; + + if (kallsyms_lookup((unsigned long) addr, NULL, &offset, NULL, str)) + return (unsigned long) addr - offset; + return 0; +} + int register_kprobe(struct kprobe *p) { int ret; @@ -1600,6 +1611,7 @@ int register_kprobe(struct kprobe *p) if (IS_ERR(addr)) return PTR_ERR(addr); p->addr = addr; + p->func_addr = resolve_func_addr(addr); ret = warn_kprobe_rereg(p); if (ret) diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 21aa30644219..25631253084a 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -1026,7 +1026,7 @@ BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs) { struct kprobe *kp = kprobe_running(); - return kp ? (uintptr_t)kp->addr : 0; + return kp ? (uintptr_t)kp->func_addr : 0; } static const struct bpf_func_proto bpf_get_func_ip_proto_kprobe = { diff --git a/tools/testing/selftests/bpf/progs/get_func_ip_test.c b/tools/testing/selftests/bpf/progs/get_func_ip_test.c index a587aeca5ae0..e988aefa567e 100644 --- a/tools/testing/selftests/bpf/progs/get_func_ip_test.c +++ b/tools/testing/selftests/bpf/progs/get_func_ip_test.c @@ -69,7 +69,7 @@ int test6(struct pt_regs *ctx) { __u64 addr = bpf_get_func_ip(ctx); - test6_result = (const void *) addr == &bpf_fentry_test6 + 5; + test6_result = (const void *) addr == &bpf_fentry_test6; return 0; } @@ -79,6 +79,6 @@ int test7(struct pt_regs *ctx) { __u64 addr = bpf_get_func_ip(ctx); - test7_result = (const void *) addr == &bpf_fentry_test7 + 5; + test7_result = (const void *) addr == &bpf_fentry_test7; return 0; }
The bpf_get_func_ip_kprobe helper should return traced function address, but it's doing so only for kprobes that are placed on the function entry. If kprobe is placed within the function, bpf_get_func_ip_kprobe returns that address instead of function entry. Storing the function entry directly in kprobe object, so it could be used in bpf_get_func_ip_kprobe helper. Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- include/linux/kprobes.h | 3 +++ kernel/kprobes.c | 12 ++++++++++++ kernel/trace/bpf_trace.c | 2 +- tools/testing/selftests/bpf/progs/get_func_ip_test.c | 4 ++-- 4 files changed, 18 insertions(+), 3 deletions(-)