diff mbox series

[RFC,bpf-next,4/4] selftests/bpf: Fix kprobe get_func_ip tests for CONFIG_X86_KERNEL_IBT

Message ID 20220705190308.1063813-5-jolsa@kernel.org (mailing list archive)
State RFC
Delegated to: BPF
Headers show
Series bpf: Fixes for CONFIG_X86_KERNEL_IBT | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 4 maintainers not CCed: alan.maguire@oracle.com shuah@kernel.org linux-kselftest@vger.kernel.org kpsingh@kernel.org
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: externs should be avoided in .c files
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Kernel LATEST on z15 with gcc
bpf/vmtest-bpf-next-VM_Test-1 success Logs for Kernel LATEST on ubuntu-latest with gcc
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Kernel LATEST on ubuntu-latest with llvm-15

Commit Message

Jiri Olsa July 5, 2022, 7:03 p.m. UTC
The kprobe can be placed anywhere and user must be aware
of the underlying instructions. Therefore fixing just
the bpf program to 'fix' the address to match the actual
function address when CONFIG_X86_KERNEL_IBT is enabled.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/testing/selftests/bpf/progs/get_func_ip_test.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Andrii Nakryiko July 6, 2022, 5:29 a.m. UTC | #1
On Tue, Jul 5, 2022 at 12:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> The kprobe can be placed anywhere and user must be aware
> of the underlying instructions. Therefore fixing just
> the bpf program to 'fix' the address to match the actual
> function address when CONFIG_X86_KERNEL_IBT is enabled.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/testing/selftests/bpf/progs/get_func_ip_test.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> 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..220d56b7c1dc 100644
> --- a/tools/testing/selftests/bpf/progs/get_func_ip_test.c
> +++ b/tools/testing/selftests/bpf/progs/get_func_ip_test.c
> @@ -2,6 +2,7 @@
>  #include <linux/bpf.h>
>  #include <bpf/bpf_helpers.h>
>  #include <bpf/bpf_tracing.h>
> +#include <stdbool.h>
>
>  char _license[] SEC("license") = "GPL";
>
> @@ -13,6 +14,8 @@ extern const void bpf_modify_return_test __ksym;
>  extern const void bpf_fentry_test6 __ksym;
>  extern const void bpf_fentry_test7 __ksym;
>
> +extern bool CONFIG_X86_KERNEL_IBT __kconfig __weak;
> +
>  __u64 test1_result = 0;
>  SEC("fentry/bpf_fentry_test1")
>  int BPF_PROG(test1, int a)
> @@ -37,7 +40,7 @@ __u64 test3_result = 0;
>  SEC("kprobe/bpf_fentry_test3")
>  int test3(struct pt_regs *ctx)
>  {
> -       __u64 addr = bpf_get_func_ip(ctx);
> +       __u64 addr = bpf_get_func_ip(ctx) - (CONFIG_X86_KERNEL_IBT ? 4 : 0);

so for kprobe bpf_get_func_ip() gets an address with 5 byte
compensation for `call __fentry__`, but not for endr? Why can't we
compensate for endbr inside the kernel code as well? I'd imagine we
either do no compensation (and thus we get &bpf_fentry_test3+5 or
&bpf_fentry_test3+9, depending on CONFIG_X86_KERNEL_IBT) or full
compensation (and thus always get &bpf_fentry_test3), but this
in-between solution seems to be the worst of both worlds?...

>
>         test3_result = (const void *) addr == &bpf_fentry_test3;
>         return 0;
> @@ -47,7 +50,7 @@ __u64 test4_result = 0;
>  SEC("kretprobe/bpf_fentry_test4")
>  int BPF_KRETPROBE(test4)
>  {
> -       __u64 addr = bpf_get_func_ip(ctx);
> +       __u64 addr = bpf_get_func_ip(ctx) - (CONFIG_X86_KERNEL_IBT ? 4 : 0);
>
>         test4_result = (const void *) addr == &bpf_fentry_test4;
>         return 0;
> --
> 2.35.3
>
Jiri Olsa July 7, 2022, 10:16 p.m. UTC | #2
On Tue, Jul 05, 2022 at 10:29:17PM -0700, Andrii Nakryiko wrote:
> On Tue, Jul 5, 2022 at 12:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > The kprobe can be placed anywhere and user must be aware
> > of the underlying instructions. Therefore fixing just
> > the bpf program to 'fix' the address to match the actual
> > function address when CONFIG_X86_KERNEL_IBT is enabled.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/testing/selftests/bpf/progs/get_func_ip_test.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > 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..220d56b7c1dc 100644
> > --- a/tools/testing/selftests/bpf/progs/get_func_ip_test.c
> > +++ b/tools/testing/selftests/bpf/progs/get_func_ip_test.c
> > @@ -2,6 +2,7 @@
> >  #include <linux/bpf.h>
> >  #include <bpf/bpf_helpers.h>
> >  #include <bpf/bpf_tracing.h>
> > +#include <stdbool.h>
> >
> >  char _license[] SEC("license") = "GPL";
> >
> > @@ -13,6 +14,8 @@ extern const void bpf_modify_return_test __ksym;
> >  extern const void bpf_fentry_test6 __ksym;
> >  extern const void bpf_fentry_test7 __ksym;
> >
> > +extern bool CONFIG_X86_KERNEL_IBT __kconfig __weak;
> > +
> >  __u64 test1_result = 0;
> >  SEC("fentry/bpf_fentry_test1")
> >  int BPF_PROG(test1, int a)
> > @@ -37,7 +40,7 @@ __u64 test3_result = 0;
> >  SEC("kprobe/bpf_fentry_test3")
> >  int test3(struct pt_regs *ctx)
> >  {
> > -       __u64 addr = bpf_get_func_ip(ctx);
> > +       __u64 addr = bpf_get_func_ip(ctx) - (CONFIG_X86_KERNEL_IBT ? 4 : 0);
> 
> so for kprobe bpf_get_func_ip() gets an address with 5 byte
> compensation for `call __fentry__`, but not for endr? Why can't we
> compensate for endbr inside the kernel code as well? I'd imagine we
> either do no compensation (and thus we get &bpf_fentry_test3+5 or
> &bpf_fentry_test3+9, depending on CONFIG_X86_KERNEL_IBT) or full
> compensation (and thus always get &bpf_fentry_test3), but this
> in-between solution seems to be the worst of both worlds?...

hm rigth, I guess we should be able to do that in bpf_get_func_ip,
I'll check

thanks,
jirka

> 
> >
> >         test3_result = (const void *) addr == &bpf_fentry_test3;
> >         return 0;
> > @@ -47,7 +50,7 @@ __u64 test4_result = 0;
> >  SEC("kretprobe/bpf_fentry_test4")
> >  int BPF_KRETPROBE(test4)
> >  {
> > -       __u64 addr = bpf_get_func_ip(ctx);
> > +       __u64 addr = bpf_get_func_ip(ctx) - (CONFIG_X86_KERNEL_IBT ? 4 : 0);
> >
> >         test4_result = (const void *) addr == &bpf_fentry_test4;
> >         return 0;
> > --
> > 2.35.3
> >
Jiri Olsa July 17, 2022, 9:43 p.m. UTC | #3
On Fri, Jul 08, 2022 at 12:16:35AM +0200, Jiri Olsa wrote:
> On Tue, Jul 05, 2022 at 10:29:17PM -0700, Andrii Nakryiko wrote:
> > On Tue, Jul 5, 2022 at 12:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > The kprobe can be placed anywhere and user must be aware
> > > of the underlying instructions. Therefore fixing just
> > > the bpf program to 'fix' the address to match the actual
> > > function address when CONFIG_X86_KERNEL_IBT is enabled.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  tools/testing/selftests/bpf/progs/get_func_ip_test.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > 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..220d56b7c1dc 100644
> > > --- a/tools/testing/selftests/bpf/progs/get_func_ip_test.c
> > > +++ b/tools/testing/selftests/bpf/progs/get_func_ip_test.c
> > > @@ -2,6 +2,7 @@
> > >  #include <linux/bpf.h>
> > >  #include <bpf/bpf_helpers.h>
> > >  #include <bpf/bpf_tracing.h>
> > > +#include <stdbool.h>
> > >
> > >  char _license[] SEC("license") = "GPL";
> > >
> > > @@ -13,6 +14,8 @@ extern const void bpf_modify_return_test __ksym;
> > >  extern const void bpf_fentry_test6 __ksym;
> > >  extern const void bpf_fentry_test7 __ksym;
> > >
> > > +extern bool CONFIG_X86_KERNEL_IBT __kconfig __weak;
> > > +
> > >  __u64 test1_result = 0;
> > >  SEC("fentry/bpf_fentry_test1")
> > >  int BPF_PROG(test1, int a)
> > > @@ -37,7 +40,7 @@ __u64 test3_result = 0;
> > >  SEC("kprobe/bpf_fentry_test3")
> > >  int test3(struct pt_regs *ctx)
> > >  {
> > > -       __u64 addr = bpf_get_func_ip(ctx);
> > > +       __u64 addr = bpf_get_func_ip(ctx) - (CONFIG_X86_KERNEL_IBT ? 4 : 0);
> > 
> > so for kprobe bpf_get_func_ip() gets an address with 5 byte
> > compensation for `call __fentry__`, but not for endr? Why can't we
> > compensate for endbr inside the kernel code as well? I'd imagine we
> > either do no compensation (and thus we get &bpf_fentry_test3+5 or
> > &bpf_fentry_test3+9, depending on CONFIG_X86_KERNEL_IBT) or full
> > compensation (and thus always get &bpf_fentry_test3), but this
> > in-between solution seems to be the worst of both worlds?...
> 
> hm rigth, I guess we should be able to do that in bpf_get_func_ip,
> I'll check

sorry for late follow up..

so the problem is that you can place kprobe anywhere in the function
(on instruction boundary) but the IBT adjustment of kprobe address is
made only if it's at the function entry and there's endbr instruction

and that kprobe address is what we return in helper:

  BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs)
  {
        struct kprobe *kp = kprobe_running();

        return kp ? (uintptr_t)kp->addr : 0;
  }

so the adjustment would work only for address at function entry, but
would be wrong for address within the function

perhaps we could add flag to kprobe to indicate the addr adjustment
was done and use it in helper

but that's why I thought I'd keep bpf_get_func_ip_kprobe as it and
leave it up to user

kprobe_multi and trampolines are different, because they can be
only at the function entry, so we can adjust the ip properly

jirka
Martynas Pumputis July 18, 2022, 11:09 a.m. UTC | #4
On 7/18/22 00:43, Jiri Olsa wrote:
> On Fri, Jul 08, 2022 at 12:16:35AM +0200, Jiri Olsa wrote:
>> On Tue, Jul 05, 2022 at 10:29:17PM -0700, Andrii Nakryiko wrote:
>>> On Tue, Jul 5, 2022 at 12:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
>>>>
>>>> The kprobe can be placed anywhere and user must be aware
>>>> of the underlying instructions. Therefore fixing just
>>>> the bpf program to 'fix' the address to match the actual
>>>> function address when CONFIG_X86_KERNEL_IBT is enabled.
>>>>
>>>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>>>> ---
>>>>   tools/testing/selftests/bpf/progs/get_func_ip_test.c | 7 +++++--
>>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> 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..220d56b7c1dc 100644
>>>> --- a/tools/testing/selftests/bpf/progs/get_func_ip_test.c
>>>> +++ b/tools/testing/selftests/bpf/progs/get_func_ip_test.c
>>>> @@ -2,6 +2,7 @@
>>>>   #include <linux/bpf.h>
>>>>   #include <bpf/bpf_helpers.h>
>>>>   #include <bpf/bpf_tracing.h>
>>>> +#include <stdbool.h>
>>>>
>>>>   char _license[] SEC("license") = "GPL";
>>>>
>>>> @@ -13,6 +14,8 @@ extern const void bpf_modify_return_test __ksym;
>>>>   extern const void bpf_fentry_test6 __ksym;
>>>>   extern const void bpf_fentry_test7 __ksym;
>>>>
>>>> +extern bool CONFIG_X86_KERNEL_IBT __kconfig __weak;
>>>> +
>>>>   __u64 test1_result = 0;
>>>>   SEC("fentry/bpf_fentry_test1")
>>>>   int BPF_PROG(test1, int a)
>>>> @@ -37,7 +40,7 @@ __u64 test3_result = 0;
>>>>   SEC("kprobe/bpf_fentry_test3")
>>>>   int test3(struct pt_regs *ctx)
>>>>   {
>>>> -       __u64 addr = bpf_get_func_ip(ctx);
>>>> +       __u64 addr = bpf_get_func_ip(ctx) - (CONFIG_X86_KERNEL_IBT ? 4 : 0);
>>>
>>> so for kprobe bpf_get_func_ip() gets an address with 5 byte
>>> compensation for `call __fentry__`, but not for endr? Why can't we
>>> compensate for endbr inside the kernel code as well? I'd imagine we
>>> either do no compensation (and thus we get &bpf_fentry_test3+5 or
>>> &bpf_fentry_test3+9, depending on CONFIG_X86_KERNEL_IBT) or full
>>> compensation (and thus always get &bpf_fentry_test3), but this
>>> in-between solution seems to be the worst of both worlds?...
>>
>> hm rigth, I guess we should be able to do that in bpf_get_func_ip,
>> I'll check
> 
> sorry for late follow up..
> 
> so the problem is that you can place kprobe anywhere in the function
> (on instruction boundary) but the IBT adjustment of kprobe address is
> made only if it's at the function entry and there's endbr instruction

To add more fun to the issue, not all non-inlined functions get endbr64. 
For example "skb_release_head_state()" does, while "skb_free_head()" 
doesn't.

> 
> and that kprobe address is what we return in helper:
> 
>    BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs)
>    {
>          struct kprobe *kp = kprobe_running();
> 
>          return kp ? (uintptr_t)kp->addr : 0;
>    }
> 
> so the adjustment would work only for address at function entry, but
> would be wrong for address within the function
> 
> perhaps we could add flag to kprobe to indicate the addr adjustment
> was done and use it in helper
> 
> but that's why I thought I'd keep bpf_get_func_ip_kprobe as it and
> leave it up to user
> 
> kprobe_multi and trampolines are different, because they can be
> only at the function entry, so we can adjust the ip properly
> 
> jirka
Jiri Olsa July 18, 2022, 12:48 p.m. UTC | #5
On Mon, Jul 18, 2022 at 02:09:54PM +0300, Martynas Pumputis wrote:
> 
> 
> On 7/18/22 00:43, Jiri Olsa wrote:
> > On Fri, Jul 08, 2022 at 12:16:35AM +0200, Jiri Olsa wrote:
> > > On Tue, Jul 05, 2022 at 10:29:17PM -0700, Andrii Nakryiko wrote:
> > > > On Tue, Jul 5, 2022 at 12:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > > > > 
> > > > > The kprobe can be placed anywhere and user must be aware
> > > > > of the underlying instructions. Therefore fixing just
> > > > > the bpf program to 'fix' the address to match the actual
> > > > > function address when CONFIG_X86_KERNEL_IBT is enabled.
> > > > > 
> > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > > ---
> > > > >   tools/testing/selftests/bpf/progs/get_func_ip_test.c | 7 +++++--
> > > > >   1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > 
> > > > > 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..220d56b7c1dc 100644
> > > > > --- a/tools/testing/selftests/bpf/progs/get_func_ip_test.c
> > > > > +++ b/tools/testing/selftests/bpf/progs/get_func_ip_test.c
> > > > > @@ -2,6 +2,7 @@
> > > > >   #include <linux/bpf.h>
> > > > >   #include <bpf/bpf_helpers.h>
> > > > >   #include <bpf/bpf_tracing.h>
> > > > > +#include <stdbool.h>
> > > > > 
> > > > >   char _license[] SEC("license") = "GPL";
> > > > > 
> > > > > @@ -13,6 +14,8 @@ extern const void bpf_modify_return_test __ksym;
> > > > >   extern const void bpf_fentry_test6 __ksym;
> > > > >   extern const void bpf_fentry_test7 __ksym;
> > > > > 
> > > > > +extern bool CONFIG_X86_KERNEL_IBT __kconfig __weak;
> > > > > +
> > > > >   __u64 test1_result = 0;
> > > > >   SEC("fentry/bpf_fentry_test1")
> > > > >   int BPF_PROG(test1, int a)
> > > > > @@ -37,7 +40,7 @@ __u64 test3_result = 0;
> > > > >   SEC("kprobe/bpf_fentry_test3")
> > > > >   int test3(struct pt_regs *ctx)
> > > > >   {
> > > > > -       __u64 addr = bpf_get_func_ip(ctx);
> > > > > +       __u64 addr = bpf_get_func_ip(ctx) - (CONFIG_X86_KERNEL_IBT ? 4 : 0);
> > > > 
> > > > so for kprobe bpf_get_func_ip() gets an address with 5 byte
> > > > compensation for `call __fentry__`, but not for endr? Why can't we
> > > > compensate for endbr inside the kernel code as well? I'd imagine we
> > > > either do no compensation (and thus we get &bpf_fentry_test3+5 or
> > > > &bpf_fentry_test3+9, depending on CONFIG_X86_KERNEL_IBT) or full
> > > > compensation (and thus always get &bpf_fentry_test3), but this
> > > > in-between solution seems to be the worst of both worlds?...
> > > 
> > > hm rigth, I guess we should be able to do that in bpf_get_func_ip,
> > > I'll check
> > 
> > sorry for late follow up..
> > 
> > so the problem is that you can place kprobe anywhere in the function
> > (on instruction boundary) but the IBT adjustment of kprobe address is
> > made only if it's at the function entry and there's endbr instruction
> 
> To add more fun to the issue, not all non-inlined functions get endbr64. For
> example "skb_release_head_state()" does, while "skb_free_head()" doesn't.

ah great.. thanks for info, will check

jirka

> 
> > 
> > and that kprobe address is what we return in helper:
> > 
> >    BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs)
> >    {
> >          struct kprobe *kp = kprobe_running();
> > 
> >          return kp ? (uintptr_t)kp->addr : 0;
> >    }
> > 
> > so the adjustment would work only for address at function entry, but
> > would be wrong for address within the function
> > 
> > perhaps we could add flag to kprobe to indicate the addr adjustment
> > was done and use it in helper
> > 
> > but that's why I thought I'd keep bpf_get_func_ip_kprobe as it and
> > leave it up to user
> > 
> > kprobe_multi and trampolines are different, because they can be
> > only at the function entry, so we can adjust the ip properly
> > 
> > jirka
Jiri Olsa July 19, 2022, 8:24 a.m. UTC | #6
On Mon, Jul 18, 2022 at 02:48:46PM +0200, Jiri Olsa wrote:
> On Mon, Jul 18, 2022 at 02:09:54PM +0300, Martynas Pumputis wrote:
> > 
> > 
> > On 7/18/22 00:43, Jiri Olsa wrote:
> > > On Fri, Jul 08, 2022 at 12:16:35AM +0200, Jiri Olsa wrote:
> > > > On Tue, Jul 05, 2022 at 10:29:17PM -0700, Andrii Nakryiko wrote:
> > > > > On Tue, Jul 5, 2022 at 12:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > > > > > 
> > > > > > The kprobe can be placed anywhere and user must be aware
> > > > > > of the underlying instructions. Therefore fixing just
> > > > > > the bpf program to 'fix' the address to match the actual
> > > > > > function address when CONFIG_X86_KERNEL_IBT is enabled.
> > > > > > 
> > > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > > > ---
> > > > > >   tools/testing/selftests/bpf/progs/get_func_ip_test.c | 7 +++++--
> > > > > >   1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > 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..220d56b7c1dc 100644
> > > > > > --- a/tools/testing/selftests/bpf/progs/get_func_ip_test.c
> > > > > > +++ b/tools/testing/selftests/bpf/progs/get_func_ip_test.c
> > > > > > @@ -2,6 +2,7 @@
> > > > > >   #include <linux/bpf.h>
> > > > > >   #include <bpf/bpf_helpers.h>
> > > > > >   #include <bpf/bpf_tracing.h>
> > > > > > +#include <stdbool.h>
> > > > > > 
> > > > > >   char _license[] SEC("license") = "GPL";
> > > > > > 
> > > > > > @@ -13,6 +14,8 @@ extern const void bpf_modify_return_test __ksym;
> > > > > >   extern const void bpf_fentry_test6 __ksym;
> > > > > >   extern const void bpf_fentry_test7 __ksym;
> > > > > > 
> > > > > > +extern bool CONFIG_X86_KERNEL_IBT __kconfig __weak;
> > > > > > +
> > > > > >   __u64 test1_result = 0;
> > > > > >   SEC("fentry/bpf_fentry_test1")
> > > > > >   int BPF_PROG(test1, int a)
> > > > > > @@ -37,7 +40,7 @@ __u64 test3_result = 0;
> > > > > >   SEC("kprobe/bpf_fentry_test3")
> > > > > >   int test3(struct pt_regs *ctx)
> > > > > >   {
> > > > > > -       __u64 addr = bpf_get_func_ip(ctx);
> > > > > > +       __u64 addr = bpf_get_func_ip(ctx) - (CONFIG_X86_KERNEL_IBT ? 4 : 0);
> > > > > 
> > > > > so for kprobe bpf_get_func_ip() gets an address with 5 byte
> > > > > compensation for `call __fentry__`, but not for endr? Why can't we
> > > > > compensate for endbr inside the kernel code as well? I'd imagine we
> > > > > either do no compensation (and thus we get &bpf_fentry_test3+5 or
> > > > > &bpf_fentry_test3+9, depending on CONFIG_X86_KERNEL_IBT) or full
> > > > > compensation (and thus always get &bpf_fentry_test3), but this
> > > > > in-between solution seems to be the worst of both worlds?...
> > > > 
> > > > hm rigth, I guess we should be able to do that in bpf_get_func_ip,
> > > > I'll check
> > > 
> > > sorry for late follow up..
> > > 
> > > so the problem is that you can place kprobe anywhere in the function
> > > (on instruction boundary) but the IBT adjustment of kprobe address is
> > > made only if it's at the function entry and there's endbr instruction
> > 
> > To add more fun to the issue, not all non-inlined functions get endbr64. For
> > example "skb_release_head_state()" does, while "skb_free_head()" doesn't.
> 
> ah great.. thanks for info, will check

I checked with Peter and yes the endbr does not need to be there

<peterz> IBT is 'Indirect Branch Tracking' ENDBR needs to be at the target for "JMP *%reg" and "CALL *%reg"
<peterz> direct call/jmp don't need anything specal

so we will need to hold the +4 info somewhere for each address
and use that in get_func_ip helper or perhaps we could read
previous instruction and check if the previous instruction is
endbr with check like:

	if (is_endbr(*(u32 *)(addr - 4)))
		addr -= 4

jirka
diff mbox series

Patch

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..220d56b7c1dc 100644
--- a/tools/testing/selftests/bpf/progs/get_func_ip_test.c
+++ b/tools/testing/selftests/bpf/progs/get_func_ip_test.c
@@ -2,6 +2,7 @@ 
 #include <linux/bpf.h>
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_tracing.h>
+#include <stdbool.h>
 
 char _license[] SEC("license") = "GPL";
 
@@ -13,6 +14,8 @@  extern const void bpf_modify_return_test __ksym;
 extern const void bpf_fentry_test6 __ksym;
 extern const void bpf_fentry_test7 __ksym;
 
+extern bool CONFIG_X86_KERNEL_IBT __kconfig __weak;
+
 __u64 test1_result = 0;
 SEC("fentry/bpf_fentry_test1")
 int BPF_PROG(test1, int a)
@@ -37,7 +40,7 @@  __u64 test3_result = 0;
 SEC("kprobe/bpf_fentry_test3")
 int test3(struct pt_regs *ctx)
 {
-	__u64 addr = bpf_get_func_ip(ctx);
+	__u64 addr = bpf_get_func_ip(ctx) - (CONFIG_X86_KERNEL_IBT ? 4 : 0);
 
 	test3_result = (const void *) addr == &bpf_fentry_test3;
 	return 0;
@@ -47,7 +50,7 @@  __u64 test4_result = 0;
 SEC("kretprobe/bpf_fentry_test4")
 int BPF_KRETPROBE(test4)
 {
-	__u64 addr = bpf_get_func_ip(ctx);
+	__u64 addr = bpf_get_func_ip(ctx) - (CONFIG_X86_KERNEL_IBT ? 4 : 0);
 
 	test4_result = (const void *) addr == &bpf_fentry_test4;
 	return 0;