bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] btf_encoder: Match ftrace addresses within elf functions
       [not found] <20210212135427.1250224-1-jolsa@redhat.com>
@ 2021-02-12 22:04 ` Jiri Olsa
  2021-02-12 22:21   ` Andrii Nakryiko
  0 siblings, 1 reply; 3+ messages in thread
From: Jiri Olsa @ 2021-02-12 22:04 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko
  Cc: dwarves, netdev, bpf, Yonghong Song, Hao Luo, Martin KaFai Lau,
	Song Liu, John Fastabend, KP Singh, Nathan Chancellor,
	Sedat Dilek

Currently when processing DWARF function, we check its entrypoint
against ftrace addresses, assuming that the ftrace address matches
with function's entrypoint.

This is not the case on some architectures as reported by Nathan
when building kernel on arm [1].

Fixing the check to take into account the whole function not
just the entrypoint.

Most of the is_ftrace_func code was contributed by Andrii.

[1] https://lore.kernel.org/bpf/20210209034416.GA1669105@ubuntu-m3-large-x86/
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 btf_encoder.c | 55 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 45 insertions(+), 10 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index b124ec20a689..03242f04c55d 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -36,6 +36,7 @@ struct funcs_layout {
 struct elf_function {
 	const char	*name;
 	unsigned long	 addr;
+	unsigned long	 size;
 	unsigned long	 sh_addr;
 	bool		 generated;
 };
@@ -98,6 +99,7 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym,
 
 	functions[functions_cnt].name = name;
 	functions[functions_cnt].addr = elf_sym__value(sym);
+	functions[functions_cnt].size = elf_sym__size(sym);
 	functions[functions_cnt].sh_addr = sh.sh_addr;
 	functions[functions_cnt].generated = false;
 	functions_cnt++;
@@ -236,6 +238,48 @@ get_kmod_addrs(struct btf_elf *btfe, __u64 **paddrs, __u64 *pcount)
 	return 0;
 }
 
+static int is_ftrace_func(struct elf_function *func, __u64 *addrs,
+			  __u64 count, bool kmod)
+{
+	/*
+	 * For vmlinux image both addrs[x] and functions[x]::addr
+	 * values are final address and are comparable.
+	 *
+	 * For kernel module addrs[x] is final address, but
+	 * functions[x]::addr is relative address within section
+	 * and needs to be relocated by adding sh_addr.
+	 */
+	__u64 start = kmod ? func->addr + func->sh_addr : func->addr;
+	__u64 addr, end = func->addr + func->size;
+
+	/*
+	 * The invariant here is addr[r] that is the smallest address
+	 * that is >= than function start addr. Except the corner case
+	 * where there is no such r, but for that we have a final check
+	 * in the return.
+	 */
+	size_t l = 0, r = count - 1, m;
+
+	/* make sure we don't use invalid r */
+	if (count == 0)
+		return false;
+
+	while (l < r) {
+		m = l + (r - l) / 2;
+		addr = addrs[m];
+
+		if (addr >= start) {
+			/* we satisfy invariant, so tighten r */
+			r = m;
+		} else {
+			/* m is not good enough as l, maybe m + 1 will be */
+			l = m + 1;
+		}
+	}
+
+	return start <= addrs[r] && addrs[r] < end;
+}
+
 static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
 {
 	__u64 *addrs, count, i;
@@ -275,18 +319,9 @@ static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
 	 */
 	for (i = 0; i < functions_cnt; i++) {
 		struct elf_function *func = &functions[i];
-		/*
-		 * For vmlinux image both addrs[x] and functions[x]::addr
-		 * values are final address and are comparable.
-		 *
-		 * For kernel module addrs[x] is final address, but
-		 * functions[x]::addr is relative address within section
-		 * and needs to be relocated by adding sh_addr.
-		 */
-		__u64 addr = kmod ? func->addr + func->sh_addr : func->addr;
 
 		/* Make sure function is within ftrace addresses. */
-		if (bsearch(&addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) {
+		if (is_ftrace_func(func, addrs, count, kmod)) {
 			/*
 			 * We iterate over sorted array, so we can easily skip
 			 * not valid item and move following valid field into
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] btf_encoder: Match ftrace addresses within elf functions
  2021-02-12 22:04 ` [PATCH] btf_encoder: Match ftrace addresses within elf functions Jiri Olsa
@ 2021-02-12 22:21   ` Andrii Nakryiko
  2021-02-13 16:41     ` Jiri Olsa
  0 siblings, 1 reply; 3+ messages in thread
From: Andrii Nakryiko @ 2021-02-12 22:21 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Arnaldo Carvalho de Melo, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, dwarves, Networking, bpf, Yonghong Song,
	Hao Luo, Martin KaFai Lau, Song Liu, John Fastabend, KP Singh,
	Nathan Chancellor, Sedat Dilek

On Fri, Feb 12, 2021 at 2:05 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Currently when processing DWARF function, we check its entrypoint
> against ftrace addresses, assuming that the ftrace address matches
> with function's entrypoint.
>
> This is not the case on some architectures as reported by Nathan
> when building kernel on arm [1].
>
> Fixing the check to take into account the whole function not
> just the entrypoint.
>
> Most of the is_ftrace_func code was contributed by Andrii.
>
> [1] https://lore.kernel.org/bpf/20210209034416.GA1669105@ubuntu-m3-large-x86/
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

LGTM. But see another suggestion below. In either case:

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  btf_encoder.c | 55 +++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 45 insertions(+), 10 deletions(-)
>
> diff --git a/btf_encoder.c b/btf_encoder.c
> index b124ec20a689..03242f04c55d 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -36,6 +36,7 @@ struct funcs_layout {
>  struct elf_function {
>         const char      *name;
>         unsigned long    addr;
> +       unsigned long    size;
>         unsigned long    sh_addr;
>         bool             generated;
>  };
> @@ -98,6 +99,7 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym,
>
>         functions[functions_cnt].name = name;
>         functions[functions_cnt].addr = elf_sym__value(sym);
> +       functions[functions_cnt].size = elf_sym__size(sym);
>         functions[functions_cnt].sh_addr = sh.sh_addr;
>         functions[functions_cnt].generated = false;
>         functions_cnt++;
> @@ -236,6 +238,48 @@ get_kmod_addrs(struct btf_elf *btfe, __u64 **paddrs, __u64 *pcount)
>         return 0;
>  }
>
> +static int is_ftrace_func(struct elf_function *func, __u64 *addrs,
> +                         __u64 count, bool kmod)
> +{
> +       /*
> +        * For vmlinux image both addrs[x] and functions[x]::addr
> +        * values are final address and are comparable.
> +        *
> +        * For kernel module addrs[x] is final address, but
> +        * functions[x]::addr is relative address within section
> +        * and needs to be relocated by adding sh_addr.
> +        */
> +       __u64 start = kmod ? func->addr + func->sh_addr : func->addr;
> +       __u64 addr, end = func->addr + func->size;
> +
> +       /*
> +        * The invariant here is addr[r] that is the smallest address
> +        * that is >= than function start addr. Except the corner case
> +        * where there is no such r, but for that we have a final check
> +        * in the return.
> +        */
> +       size_t l = 0, r = count - 1, m;
> +
> +       /* make sure we don't use invalid r */
> +       if (count == 0)
> +               return false;
> +
> +       while (l < r) {
> +               m = l + (r - l) / 2;
> +               addr = addrs[m];
> +
> +               if (addr >= start) {
> +                       /* we satisfy invariant, so tighten r */
> +                       r = m;
> +               } else {
> +                       /* m is not good enough as l, maybe m + 1 will be */
> +                       l = m + 1;
> +               }
> +       }
> +
> +       return start <= addrs[r] && addrs[r] < end;
> +}
> +
>  static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
>  {
>         __u64 *addrs, count, i;
> @@ -275,18 +319,9 @@ static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
>          */
>         for (i = 0; i < functions_cnt; i++) {
>                 struct elf_function *func = &functions[i];
> -               /*
> -                * For vmlinux image both addrs[x] and functions[x]::addr
> -                * values are final address and are comparable.
> -                *
> -                * For kernel module addrs[x] is final address, but
> -                * functions[x]::addr is relative address within section
> -                * and needs to be relocated by adding sh_addr.
> -                */
> -               __u64 addr = kmod ? func->addr + func->sh_addr : func->addr;

if we just...

if (kmod)
    func->addr += func->sh_addr;

... here, that would make is_ftrace_func() free of kmod knowledge. If
there are other places that rely on kmod vs non-kmod address of a
function, that would be simplified as well, right?

>
>                 /* Make sure function is within ftrace addresses. */
> -               if (bsearch(&addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) {
> +               if (is_ftrace_func(func, addrs, count, kmod)) {
>                         /*
>                          * We iterate over sorted array, so we can easily skip
>                          * not valid item and move following valid field into
> --
> 2.29.2
>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] btf_encoder: Match ftrace addresses within elf functions
  2021-02-12 22:21   ` Andrii Nakryiko
@ 2021-02-13 16:41     ` Jiri Olsa
  0 siblings, 0 replies; 3+ messages in thread
From: Jiri Olsa @ 2021-02-13 16:41 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, dwarves, Networking, bpf,
	Yonghong Song, Hao Luo, Martin KaFai Lau, Song Liu,
	John Fastabend, KP Singh, Nathan Chancellor, Sedat Dilek

On Fri, Feb 12, 2021 at 02:21:04PM -0800, Andrii Nakryiko wrote:
> On Fri, Feb 12, 2021 at 2:05 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Currently when processing DWARF function, we check its entrypoint
> > against ftrace addresses, assuming that the ftrace address matches
> > with function's entrypoint.
> >
> > This is not the case on some architectures as reported by Nathan
> > when building kernel on arm [1].
> >
> > Fixing the check to take into account the whole function not
> > just the entrypoint.
> >
> > Most of the is_ftrace_func code was contributed by Andrii.
> >
> > [1] https://lore.kernel.org/bpf/20210209034416.GA1669105@ubuntu-m3-large-x86/
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> 
> LGTM. But see another suggestion below. In either case:
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> >  btf_encoder.c | 55 +++++++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 45 insertions(+), 10 deletions(-)
> >
> > diff --git a/btf_encoder.c b/btf_encoder.c
> > index b124ec20a689..03242f04c55d 100644
> > --- a/btf_encoder.c
> > +++ b/btf_encoder.c
> > @@ -36,6 +36,7 @@ struct funcs_layout {
> >  struct elf_function {
> >         const char      *name;
> >         unsigned long    addr;
> > +       unsigned long    size;
> >         unsigned long    sh_addr;
> >         bool             generated;
> >  };
> > @@ -98,6 +99,7 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym,
> >
> >         functions[functions_cnt].name = name;
> >         functions[functions_cnt].addr = elf_sym__value(sym);
> > +       functions[functions_cnt].size = elf_sym__size(sym);
> >         functions[functions_cnt].sh_addr = sh.sh_addr;
> >         functions[functions_cnt].generated = false;
> >         functions_cnt++;
> > @@ -236,6 +238,48 @@ get_kmod_addrs(struct btf_elf *btfe, __u64 **paddrs, __u64 *pcount)
> >         return 0;
> >  }
> >
> > +static int is_ftrace_func(struct elf_function *func, __u64 *addrs,
> > +                         __u64 count, bool kmod)
> > +{
> > +       /*
> > +        * For vmlinux image both addrs[x] and functions[x]::addr
> > +        * values are final address and are comparable.
> > +        *
> > +        * For kernel module addrs[x] is final address, but
> > +        * functions[x]::addr is relative address within section
> > +        * and needs to be relocated by adding sh_addr.
> > +        */
> > +       __u64 start = kmod ? func->addr + func->sh_addr : func->addr;
> > +       __u64 addr, end = func->addr + func->size;
> > +
> > +       /*
> > +        * The invariant here is addr[r] that is the smallest address
> > +        * that is >= than function start addr. Except the corner case
> > +        * where there is no such r, but for that we have a final check
> > +        * in the return.
> > +        */
> > +       size_t l = 0, r = count - 1, m;
> > +
> > +       /* make sure we don't use invalid r */
> > +       if (count == 0)
> > +               return false;
> > +
> > +       while (l < r) {
> > +               m = l + (r - l) / 2;
> > +               addr = addrs[m];
> > +
> > +               if (addr >= start) {
> > +                       /* we satisfy invariant, so tighten r */
> > +                       r = m;
> > +               } else {
> > +                       /* m is not good enough as l, maybe m + 1 will be */
> > +                       l = m + 1;
> > +               }
> > +       }
> > +
> > +       return start <= addrs[r] && addrs[r] < end;
> > +}
> > +
> >  static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> >  {
> >         __u64 *addrs, count, i;
> > @@ -275,18 +319,9 @@ static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> >          */
> >         for (i = 0; i < functions_cnt; i++) {
> >                 struct elf_function *func = &functions[i];
> > -               /*
> > -                * For vmlinux image both addrs[x] and functions[x]::addr
> > -                * values are final address and are comparable.
> > -                *
> > -                * For kernel module addrs[x] is final address, but
> > -                * functions[x]::addr is relative address within section
> > -                * and needs to be relocated by adding sh_addr.
> > -                */
> > -               __u64 addr = kmod ? func->addr + func->sh_addr : func->addr;
> 
> if we just...
> 
> if (kmod)
>     func->addr += func->sh_addr;
> 
> ... here, that would make is_ftrace_func() free of kmod knowledge. If
> there are other places that rely on kmod vs non-kmod address of a
> function, that would be simplified as well, right?

yes, this is the only place for now, I'll make the change

thanks,
jirka

> 
> >
> >                 /* Make sure function is within ftrace addresses. */
> > -               if (bsearch(&addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) {
> > +               if (is_ftrace_func(func, addrs, count, kmod)) {
> >                         /*
> >                          * We iterate over sorted array, so we can easily skip
> >                          * not valid item and move following valid field into
> > --
> > 2.29.2
> >
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-02-13 16:43 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20210212135427.1250224-1-jolsa@redhat.com>
2021-02-12 22:04 ` [PATCH] btf_encoder: Match ftrace addresses within elf functions Jiri Olsa
2021-02-12 22:21   ` Andrii Nakryiko
2021-02-13 16:41     ` Jiri Olsa

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).