dwarves.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH dwarves] btf_encoder: fix and complete filtering out zero-sized per-CPU variables
@ 2021-05-24 23:42 Andrii Nakryiko
  2021-05-25 19:58 ` Arnaldo Carvalho de Melo
  2021-05-27 14:55 ` Arnaldo Carvalho de Melo
  0 siblings, 2 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2021-05-24 23:42 UTC (permalink / raw)
  To: dwarves, acme, bpf, jolsa; +Cc: andrii, kernel-team

btf_encoder is ignoring zero-sized per-CPU ELF symbols, but the same has to be
done for DWARF variables when matching them with ELF symbols. This is due to
zero-sized DWARF variables matching unrelated (non-zero-sized) variable that
happens to be allocated at the exact same address, leading to a lot of
confusion in BTF.

See [0] for when this causes big problems.

  [0] https://lore.kernel.org/bpf/CAEf4BzZ0-sihSL-UAm21JcaCCY92CqfNxycHRZYXcoj8OYb=wA@mail.gmail.com/

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 btf_encoder.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index c711f124b31e..672b9943a4e2 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -538,6 +538,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
 	cu__for_each_variable(cu, core_id, pos) {
 		uint32_t size, type, linkage;
 		const char *name, *dwarf_name;
+		const struct tag *tag;
 		uint64_t addr;
 		int id;
 
@@ -550,6 +551,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
 
 		/* addr has to be recorded before we follow spec */
 		addr = var->ip.addr;
+		dwarf_name = variable__name(var, cu);
 
 		/* DWARF takes into account .data..percpu section offset
 		 * within its segment, which for vmlinux is 0, but for kernel
@@ -582,11 +584,9 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
 		 *  modules per-CPU data section has non-zero offset so all
 		 *  per-CPU symbols have non-zero values.
 		 */
-		if (var->ip.addr == 0) {
-			dwarf_name = variable__name(var, cu);
+		if (var->ip.addr == 0)
 			if (!dwarf_name || strcmp(dwarf_name, name))
 				continue;
-		}
 
 		if (var->spec)
 			var = var->spec;
@@ -600,6 +600,13 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
 			break;
 		}
 
+		tag = cu__type(cu, var->ip.tag.type);
+		if (tag__size(tag, cu) == 0) {
+			if (btf_elf__verbose)
+				fprintf(stderr, "Ignoring zero-sized per-CPU variable '%s'...\n", dwarf_name ?: "<missing name>");
+			continue;
+		}
+
 		type = var->ip.tag.type + type_id_off;
 		linkage = var->external ? BTF_VAR_GLOBAL_ALLOCATED : BTF_VAR_STATIC;
 
-- 
2.30.2


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

* Re: [PATCH dwarves] btf_encoder: fix and complete filtering out zero-sized per-CPU variables
  2021-05-24 23:42 [PATCH dwarves] btf_encoder: fix and complete filtering out zero-sized per-CPU variables Andrii Nakryiko
@ 2021-05-25 19:58 ` Arnaldo Carvalho de Melo
  2021-05-27 14:55 ` Arnaldo Carvalho de Melo
  1 sibling, 0 replies; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-05-25 19:58 UTC (permalink / raw)
  To: Andrii Nakryiko; +Cc: dwarves, bpf, jolsa, kernel-team

Em Mon, May 24, 2021 at 04:42:22PM -0700, Andrii Nakryiko escreveu:
> btf_encoder is ignoring zero-sized per-CPU ELF symbols, but the same has to be
> done for DWARF variables when matching them with ELF symbols. This is due to
> zero-sized DWARF variables matching unrelated (non-zero-sized) variable that
> happens to be allocated at the exact same address, leading to a lot of
> confusion in BTF.

I've been following this, just didn't got to process it, will do it
soon.

- Arnaldo
 
> See [0] for when this causes big problems.
> 
>   [0] https://lore.kernel.org/bpf/CAEf4BzZ0-sihSL-UAm21JcaCCY92CqfNxycHRZYXcoj8OYb=wA@mail.gmail.com/
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  btf_encoder.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/btf_encoder.c b/btf_encoder.c
> index c711f124b31e..672b9943a4e2 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -538,6 +538,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
>  	cu__for_each_variable(cu, core_id, pos) {
>  		uint32_t size, type, linkage;
>  		const char *name, *dwarf_name;
> +		const struct tag *tag;
>  		uint64_t addr;
>  		int id;
>  
> @@ -550,6 +551,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
>  
>  		/* addr has to be recorded before we follow spec */
>  		addr = var->ip.addr;
> +		dwarf_name = variable__name(var, cu);
>  
>  		/* DWARF takes into account .data..percpu section offset
>  		 * within its segment, which for vmlinux is 0, but for kernel
> @@ -582,11 +584,9 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
>  		 *  modules per-CPU data section has non-zero offset so all
>  		 *  per-CPU symbols have non-zero values.
>  		 */
> -		if (var->ip.addr == 0) {
> -			dwarf_name = variable__name(var, cu);
> +		if (var->ip.addr == 0)
>  			if (!dwarf_name || strcmp(dwarf_name, name))
>  				continue;
> -		}
>  
>  		if (var->spec)
>  			var = var->spec;
> @@ -600,6 +600,13 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
>  			break;
>  		}
>  
> +		tag = cu__type(cu, var->ip.tag.type);
> +		if (tag__size(tag, cu) == 0) {
> +			if (btf_elf__verbose)
> +				fprintf(stderr, "Ignoring zero-sized per-CPU variable '%s'...\n", dwarf_name ?: "<missing name>");
> +			continue;
> +		}
> +
>  		type = var->ip.tag.type + type_id_off;
>  		linkage = var->external ? BTF_VAR_GLOBAL_ALLOCATED : BTF_VAR_STATIC;
>  
> -- 
> 2.30.2
> 

-- 

- Arnaldo

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

* Re: [PATCH dwarves] btf_encoder: fix and complete filtering out zero-sized per-CPU variables
  2021-05-24 23:42 [PATCH dwarves] btf_encoder: fix and complete filtering out zero-sized per-CPU variables Andrii Nakryiko
  2021-05-25 19:58 ` Arnaldo Carvalho de Melo
@ 2021-05-27 14:55 ` Arnaldo Carvalho de Melo
  2021-05-27 14:58   ` Arnaldo Carvalho de Melo
  2021-05-27 16:43   ` Andrii Nakryiko
  1 sibling, 2 replies; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-05-27 14:55 UTC (permalink / raw)
  To: Andrii Nakryiko; +Cc: dwarves, bpf, jolsa, kernel-team

Em Mon, May 24, 2021 at 04:42:22PM -0700, Andrii Nakryiko escreveu:
> btf_encoder is ignoring zero-sized per-CPU ELF symbols, but the same has to be
> done for DWARF variables when matching them with ELF symbols. This is due to
> zero-sized DWARF variables matching unrelated (non-zero-sized) variable that
> happens to be allocated at the exact same address, leading to a lot of
> confusion in BTF.
 
> See [0] for when this causes big problems.
 
>   [0] https://lore.kernel.org/bpf/CAEf4BzZ0-sihSL-UAm21JcaCCY92CqfNxycHRZYXcoj8OYb=wA@mail.gmail.com/

> +++ b/btf_encoder.c
> @@ -550,6 +551,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
>  
>  		/* addr has to be recorded before we follow spec */
>  		addr = var->ip.addr;
> +		dwarf_name = variable__name(var, cu);
>  
>  		/* DWARF takes into account .data..percpu section offset
>  		 * within its segment, which for vmlinux is 0, but for kernel
> @@ -582,11 +584,9 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
>  		 *  modules per-CPU data section has non-zero offset so all
>  		 *  per-CPU symbols have non-zero values.
>  		 */
> -		if (var->ip.addr == 0) {
> -			dwarf_name = variable__name(var, cu);
> +		if (var->ip.addr == 0)
>  			if (!dwarf_name || strcmp(dwarf_name, name))
>  				continue;
> -		}
>  
>  		if (var->spec)
>  			var = var->spec;
> @@ -600,6 +600,13 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,

I just changed the above hunk to be:

@@ -583,7 +585,6 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
                 *  per-CPU symbols have non-zero values.
                 */
                if (var->ip.addr == 0) {
-                       dwarf_name = variable__name(var, cu);
                        if (!dwarf_name || strcmp(dwarf_name, name))
                                continue;
                }


Which is shorter and keeps the {} around a multi line if block, ok?

Thanks, applied!

- Arnaldo

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

* Re: [PATCH dwarves] btf_encoder: fix and complete filtering out zero-sized per-CPU variables
  2021-05-27 14:55 ` Arnaldo Carvalho de Melo
@ 2021-05-27 14:58   ` Arnaldo Carvalho de Melo
  2021-05-27 15:27     ` Michal Suchánek
  2021-05-27 16:43   ` Andrii Nakryiko
  1 sibling, 1 reply; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-05-27 14:58 UTC (permalink / raw)
  To: Michal Suchánek; +Cc: Andrii Nakryiko, dwarves, bpf, jolsa, kernel-team

Em Thu, May 27, 2021 at 11:55:10AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, May 24, 2021 at 04:42:22PM -0700, Andrii Nakryiko escreveu:
> > btf_encoder is ignoring zero-sized per-CPU ELF symbols, but the same has to be
> > done for DWARF variables when matching them with ELF symbols. This is due to
> > zero-sized DWARF variables matching unrelated (non-zero-sized) variable that
> > happens to be allocated at the exact same address, leading to a lot of
> > confusion in BTF.
>  
> > See [0] for when this causes big problems.
>  
> >   [0] https://lore.kernel.org/bpf/CAEf4BzZ0-sihSL-UAm21JcaCCY92CqfNxycHRZYXcoj8OYb=wA@mail.gmail.com/

I also added this:

Reported-by: Michal Suchánek <msuchanek@suse.de>

Michal, so you tested this patch and verified it fixed the problem? If
so please let me know so that I also add:

Tested-by: Michal Suchánek <msuchanek@suse.de>

Thanks,

- Arnaldo
 
> > +++ b/btf_encoder.c
> > @@ -550,6 +551,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> >  
> >  		/* addr has to be recorded before we follow spec */
> >  		addr = var->ip.addr;
> > +		dwarf_name = variable__name(var, cu);
> >  
> >  		/* DWARF takes into account .data..percpu section offset
> >  		 * within its segment, which for vmlinux is 0, but for kernel
> > @@ -582,11 +584,9 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> >  		 *  modules per-CPU data section has non-zero offset so all
> >  		 *  per-CPU symbols have non-zero values.
> >  		 */
> > -		if (var->ip.addr == 0) {
> > -			dwarf_name = variable__name(var, cu);
> > +		if (var->ip.addr == 0)
> >  			if (!dwarf_name || strcmp(dwarf_name, name))
> >  				continue;
> > -		}
> >  
> >  		if (var->spec)
> >  			var = var->spec;
> > @@ -600,6 +600,13 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> 
> I just changed the above hunk to be:
> 
> @@ -583,7 +585,6 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
>                  *  per-CPU symbols have non-zero values.
>                  */
>                 if (var->ip.addr == 0) {
> -                       dwarf_name = variable__name(var, cu);
>                         if (!dwarf_name || strcmp(dwarf_name, name))
>                                 continue;
>                 }
> 
> 
> Which is shorter and keeps the {} around a multi line if block, ok?
> 
> Thanks, applied!
> 
> - Arnaldo

-- 

- Arnaldo

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

* Re: [PATCH dwarves] btf_encoder: fix and complete filtering out zero-sized per-CPU variables
  2021-05-27 14:58   ` Arnaldo Carvalho de Melo
@ 2021-05-27 15:27     ` Michal Suchánek
  2021-05-27 15:38       ` Arnaldo Carvalho de Melo
  2021-05-27 16:45       ` Andrii Nakryiko
  0 siblings, 2 replies; 10+ messages in thread
From: Michal Suchánek @ 2021-05-27 15:27 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Andrii Nakryiko, dwarves, bpf, jolsa, kernel-team

Hello,

On Thu, May 27, 2021 at 11:58:24AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, May 27, 2021 at 11:55:10AM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Mon, May 24, 2021 at 04:42:22PM -0700, Andrii Nakryiko escreveu:
> > > btf_encoder is ignoring zero-sized per-CPU ELF symbols, but the same has to be
> > > done for DWARF variables when matching them with ELF symbols. This is due to
> > > zero-sized DWARF variables matching unrelated (non-zero-sized) variable that
> > > happens to be allocated at the exact same address, leading to a lot of
> > > confusion in BTF.
> >  
> > > See [0] for when this causes big problems.
> >  
> > >   [0] https://lore.kernel.org/bpf/CAEf4BzZ0-sihSL-UAm21JcaCCY92CqfNxycHRZYXcoj8OYb=wA@mail.gmail.com/
> 
> I also added this:
> 
> Reported-by: Michal Suchánek <msuchanek@suse.de>
> 
> Michal, so you tested this patch and verified it fixed the problem? If
> so please let me know so that I also add:

This is the first time I see this patch.

Given that linux-next does not build for me at the moment
I don't think I will test it soon.

Thanks

Michal

> 
> Tested-by: Michal Suchánek <msuchanek@suse.de>
> 
> Thanks,
> 
> - Arnaldo
>  
> > > +++ b/btf_encoder.c
> > > @@ -550,6 +551,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > >  
> > >  		/* addr has to be recorded before we follow spec */
> > >  		addr = var->ip.addr;
> > > +		dwarf_name = variable__name(var, cu);
> > >  
> > >  		/* DWARF takes into account .data..percpu section offset
> > >  		 * within its segment, which for vmlinux is 0, but for kernel
> > > @@ -582,11 +584,9 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > >  		 *  modules per-CPU data section has non-zero offset so all
> > >  		 *  per-CPU symbols have non-zero values.
> > >  		 */
> > > -		if (var->ip.addr == 0) {
> > > -			dwarf_name = variable__name(var, cu);
> > > +		if (var->ip.addr == 0)
> > >  			if (!dwarf_name || strcmp(dwarf_name, name))
> > >  				continue;
> > > -		}
> > >  
> > >  		if (var->spec)
> > >  			var = var->spec;
> > > @@ -600,6 +600,13 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > 
> > I just changed the above hunk to be:
> > 
> > @@ -583,7 +585,6 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> >                  *  per-CPU symbols have non-zero values.
> >                  */
> >                 if (var->ip.addr == 0) {
> > -                       dwarf_name = variable__name(var, cu);
> >                         if (!dwarf_name || strcmp(dwarf_name, name))
> >                                 continue;
> >                 }
> > 
> > 
> > Which is shorter and keeps the {} around a multi line if block, ok?
> > 
> > Thanks, applied!
> > 
> > - Arnaldo
> 
> -- 
> 
> - Arnaldo

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

* Re: [PATCH dwarves] btf_encoder: fix and complete filtering out zero-sized per-CPU variables
  2021-05-27 15:27     ` Michal Suchánek
@ 2021-05-27 15:38       ` Arnaldo Carvalho de Melo
  2021-05-27 16:47         ` Arnaldo Carvalho de Melo
  2021-05-27 16:45       ` Andrii Nakryiko
  1 sibling, 1 reply; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-05-27 15:38 UTC (permalink / raw)
  To: Michal Suchánek; +Cc: Andrii Nakryiko, dwarves, bpf, jolsa, kernel-team

Em Thu, May 27, 2021 at 05:27:58PM +0200, Michal Suchánek escreveu:
> Hello,
> 
> On Thu, May 27, 2021 at 11:58:24AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Thu, May 27, 2021 at 11:55:10AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Mon, May 24, 2021 at 04:42:22PM -0700, Andrii Nakryiko escreveu:
> > > > btf_encoder is ignoring zero-sized per-CPU ELF symbols, but the same has to be
> > > > done for DWARF variables when matching them with ELF symbols. This is due to
> > > > zero-sized DWARF variables matching unrelated (non-zero-sized) variable that
> > > > happens to be allocated at the exact same address, leading to a lot of
> > > > confusion in BTF.
> > >  
> > > > See [0] for when this causes big problems.
> > >  
> > > >   [0] https://lore.kernel.org/bpf/CAEf4BzZ0-sihSL-UAm21JcaCCY92CqfNxycHRZYXcoj8OYb=wA@mail.gmail.com/
> > 
> > I also added this:
> > 
> > Reported-by: Michal Suchánek <msuchanek@suse.de>
> > 
> > Michal, so you tested this patch and verified it fixed the problem? If
> > so please let me know so that I also add:
> 
> This is the first time I see this patch.
> 
> Given that linux-next does not build for me at the moment
> I don't think I will test it soon.

Ok, I'm test building with torvalds/master, will try with linux-next
afterwards,

Thanks,

- Arnaldo
 
> Thanks
> 
> Michal
> 
> > 
> > Tested-by: Michal Suchánek <msuchanek@suse.de>
> > 
> > Thanks,
> > 
> > - Arnaldo
> >  
> > > > +++ b/btf_encoder.c
> > > > @@ -550,6 +551,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > > >  
> > > >  		/* addr has to be recorded before we follow spec */
> > > >  		addr = var->ip.addr;
> > > > +		dwarf_name = variable__name(var, cu);
> > > >  
> > > >  		/* DWARF takes into account .data..percpu section offset
> > > >  		 * within its segment, which for vmlinux is 0, but for kernel
> > > > @@ -582,11 +584,9 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > > >  		 *  modules per-CPU data section has non-zero offset so all
> > > >  		 *  per-CPU symbols have non-zero values.
> > > >  		 */
> > > > -		if (var->ip.addr == 0) {
> > > > -			dwarf_name = variable__name(var, cu);
> > > > +		if (var->ip.addr == 0)
> > > >  			if (!dwarf_name || strcmp(dwarf_name, name))
> > > >  				continue;
> > > > -		}
> > > >  
> > > >  		if (var->spec)
> > > >  			var = var->spec;
> > > > @@ -600,6 +600,13 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > > 
> > > I just changed the above hunk to be:
> > > 
> > > @@ -583,7 +585,6 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > >                  *  per-CPU symbols have non-zero values.
> > >                  */
> > >                 if (var->ip.addr == 0) {
> > > -                       dwarf_name = variable__name(var, cu);
> > >                         if (!dwarf_name || strcmp(dwarf_name, name))
> > >                                 continue;
> > >                 }
> > > 
> > > 
> > > Which is shorter and keeps the {} around a multi line if block, ok?
> > > 
> > > Thanks, applied!
> > > 
> > > - Arnaldo
> > 
> > -- 
> > 
> > - Arnaldo

-- 

- Arnaldo

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

* Re: [PATCH dwarves] btf_encoder: fix and complete filtering out zero-sized per-CPU variables
  2021-05-27 14:55 ` Arnaldo Carvalho de Melo
  2021-05-27 14:58   ` Arnaldo Carvalho de Melo
@ 2021-05-27 16:43   ` Andrii Nakryiko
  1 sibling, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2021-05-27 16:43 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Andrii Nakryiko, dwarves, bpf, Jiri Olsa, Kernel Team

On Thu, May 27, 2021 at 7:55 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Mon, May 24, 2021 at 04:42:22PM -0700, Andrii Nakryiko escreveu:
> > btf_encoder is ignoring zero-sized per-CPU ELF symbols, but the same has to be
> > done for DWARF variables when matching them with ELF symbols. This is due to
> > zero-sized DWARF variables matching unrelated (non-zero-sized) variable that
> > happens to be allocated at the exact same address, leading to a lot of
> > confusion in BTF.
>
> > See [0] for when this causes big problems.
>
> >   [0] https://lore.kernel.org/bpf/CAEf4BzZ0-sihSL-UAm21JcaCCY92CqfNxycHRZYXcoj8OYb=wA@mail.gmail.com/
>
> > +++ b/btf_encoder.c
> > @@ -550,6 +551,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> >
> >               /* addr has to be recorded before we follow spec */
> >               addr = var->ip.addr;
> > +             dwarf_name = variable__name(var, cu);
> >
> >               /* DWARF takes into account .data..percpu section offset
> >                * within its segment, which for vmlinux is 0, but for kernel
> > @@ -582,11 +584,9 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> >                *  modules per-CPU data section has non-zero offset so all
> >                *  per-CPU symbols have non-zero values.
> >                */
> > -             if (var->ip.addr == 0) {
> > -                     dwarf_name = variable__name(var, cu);
> > +             if (var->ip.addr == 0)
> >                       if (!dwarf_name || strcmp(dwarf_name, name))
> >                               continue;
> > -             }
> >
> >               if (var->spec)
> >                       var = var->spec;
> > @@ -600,6 +600,13 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
>
> I just changed the above hunk to be:
>
> @@ -583,7 +585,6 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
>                  *  per-CPU symbols have non-zero values.
>                  */
>                 if (var->ip.addr == 0) {
> -                       dwarf_name = variable__name(var, cu);
>                         if (!dwarf_name || strcmp(dwarf_name, name))
>                                 continue;
>                 }
>
>
> Which is shorter and keeps the {} around a multi line if block, ok?

yeah, no problem

>
> Thanks, applied!

Thanks!
>
> - Arnaldo

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

* Re: [PATCH dwarves] btf_encoder: fix and complete filtering out zero-sized per-CPU variables
  2021-05-27 15:27     ` Michal Suchánek
  2021-05-27 15:38       ` Arnaldo Carvalho de Melo
@ 2021-05-27 16:45       ` Andrii Nakryiko
  1 sibling, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2021-05-27 16:45 UTC (permalink / raw)
  To: Michal Suchánek
  Cc: Arnaldo Carvalho de Melo, Andrii Nakryiko, dwarves, bpf,
	Jiri Olsa, Kernel Team

On Thu, May 27, 2021 at 8:37 AM Michal Suchánek <msuchanek@suse.de> wrote:
>
> Hello,
>
> On Thu, May 27, 2021 at 11:58:24AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Thu, May 27, 2021 at 11:55:10AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Mon, May 24, 2021 at 04:42:22PM -0700, Andrii Nakryiko escreveu:
> > > > btf_encoder is ignoring zero-sized per-CPU ELF symbols, but the same has to be
> > > > done for DWARF variables when matching them with ELF symbols. This is due to
> > > > zero-sized DWARF variables matching unrelated (non-zero-sized) variable that
> > > > happens to be allocated at the exact same address, leading to a lot of
> > > > confusion in BTF.
> > >
> > > > See [0] for when this causes big problems.
> > >
> > > >   [0] https://lore.kernel.org/bpf/CAEf4BzZ0-sihSL-UAm21JcaCCY92CqfNxycHRZYXcoj8OYb=wA@mail.gmail.com/
> >
> > I also added this:
> >
> > Reported-by: Michal Suchánek <msuchanek@suse.de>
> >
> > Michal, so you tested this patch and verified it fixed the problem? If
> > so please let me know so that I also add:
>
> This is the first time I see this patch.
>

I've posted a link to it in your thread [0].

  [0] https://lore.kernel.org/bpf/CAEf4BzZ9=aLVD7ytgCcSxcbOLqFNK-p1mj14Rv_TGnOyL3aO_g@mail.gmail.com/

> Given that linux-next does not build for me at the moment
> I don't think I will test it soon.

This patch applied to pahole master will fix linux-next build.

>
> Thanks
>
> Michal
>
> >
> > Tested-by: Michal Suchánek <msuchanek@suse.de>
> >
> > Thanks,
> >
> > - Arnaldo
> >
> > > > +++ b/btf_encoder.c
> > > > @@ -550,6 +551,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > > >
> > > >           /* addr has to be recorded before we follow spec */
> > > >           addr = var->ip.addr;
> > > > +         dwarf_name = variable__name(var, cu);
> > > >
> > > >           /* DWARF takes into account .data..percpu section offset
> > > >            * within its segment, which for vmlinux is 0, but for kernel
> > > > @@ -582,11 +584,9 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > > >            *  modules per-CPU data section has non-zero offset so all
> > > >            *  per-CPU symbols have non-zero values.
> > > >            */
> > > > -         if (var->ip.addr == 0) {
> > > > -                 dwarf_name = variable__name(var, cu);
> > > > +         if (var->ip.addr == 0)
> > > >                   if (!dwarf_name || strcmp(dwarf_name, name))
> > > >                           continue;
> > > > -         }
> > > >
> > > >           if (var->spec)
> > > >                   var = var->spec;
> > > > @@ -600,6 +600,13 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > >
> > > I just changed the above hunk to be:
> > >
> > > @@ -583,7 +585,6 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > >                  *  per-CPU symbols have non-zero values.
> > >                  */
> > >                 if (var->ip.addr == 0) {
> > > -                       dwarf_name = variable__name(var, cu);
> > >                         if (!dwarf_name || strcmp(dwarf_name, name))
> > >                                 continue;
> > >                 }
> > >
> > >
> > > Which is shorter and keeps the {} around a multi line if block, ok?
> > >
> > > Thanks, applied!
> > >
> > > - Arnaldo
> >
> > --
> >
> > - Arnaldo

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

* Re: [PATCH dwarves] btf_encoder: fix and complete filtering out zero-sized per-CPU variables
  2021-05-27 15:38       ` Arnaldo Carvalho de Melo
@ 2021-05-27 16:47         ` Arnaldo Carvalho de Melo
  2021-05-28  6:24           ` Michal Suchánek
  0 siblings, 1 reply; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-05-27 16:47 UTC (permalink / raw)
  To: Michal Suchánek; +Cc: Andrii Nakryiko, dwarves, bpf, jolsa, kernel-team

Em Thu, May 27, 2021 at 12:38:38PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, May 27, 2021 at 05:27:58PM +0200, Michal Suchánek escreveu:
> > Hello,
> > 
> > On Thu, May 27, 2021 at 11:58:24AM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Thu, May 27, 2021 at 11:55:10AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > Em Mon, May 24, 2021 at 04:42:22PM -0700, Andrii Nakryiko escreveu:
> > > > > btf_encoder is ignoring zero-sized per-CPU ELF symbols, but the same has to be
> > > > > done for DWARF variables when matching them with ELF symbols. This is due to
> > > > > zero-sized DWARF variables matching unrelated (non-zero-sized) variable that
> > > > > happens to be allocated at the exact same address, leading to a lot of
> > > > > confusion in BTF.
> > > >  
> > > > > See [0] for when this causes big problems.
> > > >  
> > > > >   [0] https://lore.kernel.org/bpf/CAEf4BzZ0-sihSL-UAm21JcaCCY92CqfNxycHRZYXcoj8OYb=wA@mail.gmail.com/
> > > 
> > > I also added this:
> > > 
> > > Reported-by: Michal Suchánek <msuchanek@suse.de>
> > > 
> > > Michal, so you tested this patch and verified it fixed the problem? If
> > > so please let me know so that I also add:
> > 
> > This is the first time I see this patch.
> > 
> > Given that linux-next does not build for me at the moment
> > I don't think I will test it soon.
> 
> Ok, I'm test building with torvalds/master, will try with linux-next
> afterwards,

I build and booted torvalds/master, all seems to work, now moving to
linux-next.
 
> Thanks,
> 
> - Arnaldo
>  
> > Thanks
> > 
> > Michal
> > 
> > > 
> > > Tested-by: Michal Suchánek <msuchanek@suse.de>
> > > 
> > > Thanks,
> > > 
> > > - Arnaldo
> > >  
> > > > > +++ b/btf_encoder.c
> > > > > @@ -550,6 +551,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > > > >  
> > > > >  		/* addr has to be recorded before we follow spec */
> > > > >  		addr = var->ip.addr;
> > > > > +		dwarf_name = variable__name(var, cu);
> > > > >  
> > > > >  		/* DWARF takes into account .data..percpu section offset
> > > > >  		 * within its segment, which for vmlinux is 0, but for kernel
> > > > > @@ -582,11 +584,9 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > > > >  		 *  modules per-CPU data section has non-zero offset so all
> > > > >  		 *  per-CPU symbols have non-zero values.
> > > > >  		 */
> > > > > -		if (var->ip.addr == 0) {
> > > > > -			dwarf_name = variable__name(var, cu);
> > > > > +		if (var->ip.addr == 0)
> > > > >  			if (!dwarf_name || strcmp(dwarf_name, name))
> > > > >  				continue;
> > > > > -		}
> > > > >  
> > > > >  		if (var->spec)
> > > > >  			var = var->spec;
> > > > > @@ -600,6 +600,13 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > > > 
> > > > I just changed the above hunk to be:
> > > > 
> > > > @@ -583,7 +585,6 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > > >                  *  per-CPU symbols have non-zero values.
> > > >                  */
> > > >                 if (var->ip.addr == 0) {
> > > > -                       dwarf_name = variable__name(var, cu);
> > > >                         if (!dwarf_name || strcmp(dwarf_name, name))
> > > >                                 continue;
> > > >                 }
> > > > 
> > > > 
> > > > Which is shorter and keeps the {} around a multi line if block, ok?
> > > > 
> > > > Thanks, applied!
> > > > 
> > > > - Arnaldo
> > > 
> > > -- 
> > > 
> > > - Arnaldo
> 
> -- 
> 
> - Arnaldo

-- 

- Arnaldo

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

* Re: [PATCH dwarves] btf_encoder: fix and complete filtering out zero-sized per-CPU variables
  2021-05-27 16:47         ` Arnaldo Carvalho de Melo
@ 2021-05-28  6:24           ` Michal Suchánek
  0 siblings, 0 replies; 10+ messages in thread
From: Michal Suchánek @ 2021-05-28  6:24 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Andrii Nakryiko, dwarves, bpf, jolsa, kernel-team

On Thu, May 27, 2021 at 01:47:41PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, May 27, 2021 at 12:38:38PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Thu, May 27, 2021 at 05:27:58PM +0200, Michal Suchánek escreveu:
> > > Hello,
> > > 
> > > On Thu, May 27, 2021 at 11:58:24AM -0300, Arnaldo Carvalho de Melo wrote:
> > > > Em Thu, May 27, 2021 at 11:55:10AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > > Em Mon, May 24, 2021 at 04:42:22PM -0700, Andrii Nakryiko escreveu:
> > > > > > btf_encoder is ignoring zero-sized per-CPU ELF symbols, but the same has to be
> > > > > > done for DWARF variables when matching them with ELF symbols. This is due to
> > > > > > zero-sized DWARF variables matching unrelated (non-zero-sized) variable that
> > > > > > happens to be allocated at the exact same address, leading to a lot of
> > > > > > confusion in BTF.
> > > > >  
> > > > > > See [0] for when this causes big problems.
> > > > >  
> > > > > >   [0] https://lore.kernel.org/bpf/CAEf4BzZ0-sihSL-UAm21JcaCCY92CqfNxycHRZYXcoj8OYb=wA@mail.gmail.com/
> > > > 
> > > > I also added this:
> > > > 
> > > > Reported-by: Michal Suchánek <msuchanek@suse.de>
> > > > 
> > > > Michal, so you tested this patch and verified it fixed the problem? If
> > > > so please let me know so that I also add:
> > > 
> > > This is the first time I see this patch.
> > > 
> > > Given that linux-next does not build for me at the moment
> > > I don't think I will test it soon.
> > 
> > Ok, I'm test building with torvalds/master, will try with linux-next
> > afterwards,
> 
> I build and booted torvalds/master, all seems to work, now moving to
> linux-next.

[    9s] /usr/bin/qemu-kvm -nodefaults -no-reboot -nographic -vga none -cpu host -object rng-random,filename=/dev/random,id=rng0 -device virtio-rng-pci,rng=rng0 -runas qemu -net none -kernel /var/cache/obs/worker/root_2/.mount/boot/kernel -initrd /var/cache/obs/worker/root_2/.mount/boot/initrd -append root=/dev/disk/by-id/virtio-0 rootfstype=ext4 rootflags=noatime ext4.allow_unsupported=1 mitigations=off panic=1 quiet no-kvmclock elevator=noop nmi_watchdog=0 rw rd.driver.pre=binfmt_misc console=ttyS0 init=/.build/build -m 8192 -drive file=/var/cache/obs/worker/root_2/root,format=raw,if=none,id=disk,cache=unsafe -device virtio-blk-pci,drive=disk,serial=0 -drive file=/var/cache/obs/worker/root_2/swap,format=raw,if=none,id=swap,cache=unsafe -device virtio-blk-pci,drive=swap,serial=1 -serial stdio -chardev socket,id=monitor,server,nowait,path=/var/cache/obs/worker/root_2/root.qemu/monitor -mon chardev=monitor,mode=readline -smp 8
[    9s] c[?7l[2J[0mSeaBIOS (version rel-1.14.0-0-g155821a-rebuilt.opensuse.org)
[   15s] Booting from ROM..c[?7l[2J### VM INTERACTION END ###
[   15s] 2nd stage started in virtual machine
[   15s] machine type: x86_64
[   15s] Linux version: 5.13.0-rc3-next-2[    5.210478] sysrq: Changing Loglevel
[   15s] 0210526-2.g3bd43[    5.211566] sysrq: Loglevel set to 4
[   15s] 9f-vanilla #1 SMP Thu May 27 18:27:17 UTC 2021 (3bd439f)
[   15s] Increasing log level from now on...
[   15s] Enable sysrq operations
[   15s] Setting up swapspace version 1, size = 2 GiB (2097147904 bytes)
[   15s] no label, UUID=6e22796c-6df5-4859-a291-24acde2e92e5
[   15s] swapon: /dev/vdb: found signature [pagesize=4096, signature=swap]
[   15s] swapon: /dev/vdb: pagesize=4096, swapsize=2097152000, devsize=2097152000
[   15s] swapon /dev/vdb
[   15s] WARNING: udev not running, creating extra device nodes
[   15s] logging output to //.build.log...

next-20210527 does not build becuase of missing symbol but next-20210526 built with the patched pahole boots.

Tested-by: Michal Suchánek <msuchanek@suse.de>

Thanks

Michal
>  
> > Thanks,
> > 
> > - Arnaldo
> >  
> > > Thanks
> > > 
> > > Michal
> > > 
> > > > 
> > > > Tested-by: Michal Suchánek <msuchanek@suse.de>
> > > > 
> > > > Thanks,
> > > > 
> > > > - Arnaldo
> > > >  
> > > > > > +++ b/btf_encoder.c
> > > > > > @@ -550,6 +551,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > > > > >  
> > > > > >  		/* addr has to be recorded before we follow spec */
> > > > > >  		addr = var->ip.addr;
> > > > > > +		dwarf_name = variable__name(var, cu);
> > > > > >  
> > > > > >  		/* DWARF takes into account .data..percpu section offset
> > > > > >  		 * within its segment, which for vmlinux is 0, but for kernel
> > > > > > @@ -582,11 +584,9 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > > > > >  		 *  modules per-CPU data section has non-zero offset so all
> > > > > >  		 *  per-CPU symbols have non-zero values.
> > > > > >  		 */
> > > > > > -		if (var->ip.addr == 0) {
> > > > > > -			dwarf_name = variable__name(var, cu);
> > > > > > +		if (var->ip.addr == 0)
> > > > > >  			if (!dwarf_name || strcmp(dwarf_name, name))
> > > > > >  				continue;
> > > > > > -		}
> > > > > >  
> > > > > >  		if (var->spec)
> > > > > >  			var = var->spec;
> > > > > > @@ -600,6 +600,13 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > > > > 
> > > > > I just changed the above hunk to be:
> > > > > 
> > > > > @@ -583,7 +585,6 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > > > >                  *  per-CPU symbols have non-zero values.
> > > > >                  */
> > > > >                 if (var->ip.addr == 0) {
> > > > > -                       dwarf_name = variable__name(var, cu);
> > > > >                         if (!dwarf_name || strcmp(dwarf_name, name))
> > > > >                                 continue;
> > > > >                 }
> > > > > 
> > > > > 
> > > > > Which is shorter and keeps the {} around a multi line if block, ok?
> > > > > 
> > > > > Thanks, applied!
> > > > > 
> > > > > - Arnaldo
> > > > 
> > > > -- 
> > > > 
> > > > - Arnaldo
> > 
> > -- 
> > 
> > - Arnaldo
> 
> -- 
> 
> - Arnaldo

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

end of thread, other threads:[~2021-05-28  6:24 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-24 23:42 [PATCH dwarves] btf_encoder: fix and complete filtering out zero-sized per-CPU variables Andrii Nakryiko
2021-05-25 19:58 ` Arnaldo Carvalho de Melo
2021-05-27 14:55 ` Arnaldo Carvalho de Melo
2021-05-27 14:58   ` Arnaldo Carvalho de Melo
2021-05-27 15:27     ` Michal Suchánek
2021-05-27 15:38       ` Arnaldo Carvalho de Melo
2021-05-27 16:47         ` Arnaldo Carvalho de Melo
2021-05-28  6:24           ` Michal Suchánek
2021-05-27 16:45       ` Andrii Nakryiko
2021-05-27 16:43   ` Andrii Nakryiko

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).