[PATCH bpf-next] tools/resolve_btfids: Warn when having multiple IDs for single type

Subsystems: bpf [general] (safe dynamic programs and tools), the rest

STALE2048d

6 messages, 4 authors, 2021-01-05 · open the first message on its own page

[PATCH bpf-next] tools/resolve_btfids: Warn when having multiple IDs for single type

From: Jiri Olsa <jolsa@kernel.org>
Date: 2021-01-05 15:41:01 

The kernel image can contain multiple types (structs/unions)
with the same name. This causes distinct type hierarchies in
BTF data and makes resolve_btfids fail with error like:

  BTFIDS  vmlinux
FAILED unresolved symbol udp6_sock

as reported by Qais Yousef [1].

This change adds warning when multiple types of the same name
are detected:

  BTFIDS  vmlinux
WARN: multiple IDs found for 'file' (526, 113351)
WARN: multiple IDs found for 'sk_buff' (2744, 113958)

We keep the lower ID for the given type instance and let the
build continue.

[1] https://lore.kernel.org/lkml/20201229151352.6hzmjvu3qh6p2qgg@e107158-lin/
Reported-by: Qais Yousef <redacted>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/bpf/resolve_btfids/main.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
index e3ea569ee125..36a3b1024cdc 100644
--- a/tools/bpf/resolve_btfids/main.c
+++ b/tools/bpf/resolve_btfids/main.c
@@ -139,6 +139,8 @@ int eprintf(int level, int var, const char *fmt, ...)
 #define pr_debug2(fmt, ...) pr_debugN(2, pr_fmt(fmt), ##__VA_ARGS__)
 #define pr_err(fmt, ...) \
 	eprintf(0, verbose, pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_info(fmt, ...) \
+	eprintf(0, verbose, pr_fmt(fmt), ##__VA_ARGS__)
 
 static bool is_btf_id(const char *name)
 {
@@ -526,8 +528,13 @@ static int symbols_resolve(struct object *obj)
 
 		id = btf_id__find(root, str);
 		if (id) {
-			id->id = type_id;
-			(*nr)--;
+			if (id->id) {
+				pr_info("WARN: multiple IDs found for '%s' (%d, %d)\n",
+					str, id->id, type_id);
+			} else {
+				id->id = type_id;
+				(*nr)--;
+			}
 		}
 	}
 
-- 
2.26.2

Re: [PATCH bpf-next] tools/resolve_btfids: Warn when having multiple IDs for single type

From: Alexei Starovoitov <hidden>
Date: 2021-01-05 19:38:04 

On Tue, Jan 5, 2021 at 7:39 AM Jiri Olsa [off-list ref] wrote:
The kernel image can contain multiple types (structs/unions)
with the same name. This causes distinct type hierarchies in
BTF data and makes resolve_btfids fail with error like:

  BTFIDS  vmlinux
FAILED unresolved symbol udp6_sock

as reported by Qais Yousef [1].

This change adds warning when multiple types of the same name
are detected:

  BTFIDS  vmlinux
WARN: multiple IDs found for 'file' (526, 113351)
WARN: multiple IDs found for 'sk_buff' (2744, 113958)

We keep the lower ID for the given type instance and let the
build continue.
I think it would make sense to mention this decision in the warning.
'WARN: multiple IDs' is ambiguous and confusing when action is not specified.

Re: [PATCH bpf-next] tools/resolve_btfids: Warn when having multiple IDs for single type

From: Jiri Olsa <hidden>
Date: 2021-01-05 19:52:07 

On Tue, Jan 05, 2021 at 11:37:09AM -0800, Alexei Starovoitov wrote:
On Tue, Jan 5, 2021 at 7:39 AM Jiri Olsa [off-list ref] wrote:
quoted
The kernel image can contain multiple types (structs/unions)
with the same name. This causes distinct type hierarchies in
BTF data and makes resolve_btfids fail with error like:

  BTFIDS  vmlinux
FAILED unresolved symbol udp6_sock

as reported by Qais Yousef [1].

This change adds warning when multiple types of the same name
are detected:

  BTFIDS  vmlinux
WARN: multiple IDs found for 'file' (526, 113351)
WARN: multiple IDs found for 'sk_buff' (2744, 113958)

We keep the lower ID for the given type instance and let the
build continue.
I think it would make sense to mention this decision in the warning.
'WARN: multiple IDs' is ambiguous and confusing when action is not specified.
ok, how about:

WARN: multiple IDs found for 'file': 526, 113351 - using 526

jirka

Re: [PATCH bpf-next] tools/resolve_btfids: Warn when having multiple IDs for single type

From: Alexei Starovoitov <hidden>
Date: 2021-01-05 20:03:12 

On Tue, Jan 5, 2021 at 11:50 AM Jiri Olsa [off-list ref] wrote:
On Tue, Jan 05, 2021 at 11:37:09AM -0800, Alexei Starovoitov wrote:
quoted
On Tue, Jan 5, 2021 at 7:39 AM Jiri Olsa [off-list ref] wrote:
quoted
The kernel image can contain multiple types (structs/unions)
with the same name. This causes distinct type hierarchies in
BTF data and makes resolve_btfids fail with error like:

  BTFIDS  vmlinux
FAILED unresolved symbol udp6_sock

as reported by Qais Yousef [1].

This change adds warning when multiple types of the same name
are detected:

  BTFIDS  vmlinux
WARN: multiple IDs found for 'file' (526, 113351)
WARN: multiple IDs found for 'sk_buff' (2744, 113958)

We keep the lower ID for the given type instance and let the
build continue.
I think it would make sense to mention this decision in the warning.
'WARN: multiple IDs' is ambiguous and confusing when action is not specified.
ok, how about:

WARN: multiple IDs found for 'file': 526, 113351 - using 526
yep. much better imo.
Please add a comment to .c file with the suggestion that such types should
be renamed to avoid ambiguity.
Adding 'please rename' to WARN is probably overkill.

Re: [PATCH bpf-next] tools/resolve_btfids: Warn when having multiple IDs for single type

From: Andrii Nakryiko <hidden>
Date: 2021-01-05 21:16:46 

On Tue, Jan 5, 2021 at 7:41 AM Jiri Olsa [off-list ref] wrote:
The kernel image can contain multiple types (structs/unions)
with the same name. This causes distinct type hierarchies in
BTF data and makes resolve_btfids fail with error like:

  BTFIDS  vmlinux
FAILED unresolved symbol udp6_sock

as reported by Qais Yousef [1].

This change adds warning when multiple types of the same name
are detected:

  BTFIDS  vmlinux
WARN: multiple IDs found for 'file' (526, 113351)
WARN: multiple IDs found for 'sk_buff' (2744, 113958)

We keep the lower ID for the given type instance and let the
build continue.

[1] https://lore.kernel.org/lkml/20201229151352.6hzmjvu3qh6p2qgg@e107158-lin/
Reported-by: Qais Yousef <redacted>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
see comments below, but otherwise lgtm

Acked-by: Andrii Nakryiko <andrii@kernel.org>
quoted hunk
 tools/bpf/resolve_btfids/main.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
index e3ea569ee125..36a3b1024cdc 100644
--- a/tools/bpf/resolve_btfids/main.c
+++ b/tools/bpf/resolve_btfids/main.c
@@ -139,6 +139,8 @@ int eprintf(int level, int var, const char *fmt, ...)
 #define pr_debug2(fmt, ...) pr_debugN(2, pr_fmt(fmt), ##__VA_ARGS__)
 #define pr_err(fmt, ...) \
        eprintf(0, verbose, pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_info(fmt, ...) \
+       eprintf(0, verbose, pr_fmt(fmt), ##__VA_ARGS__)
how is it different from pr_err? Did you forget to update verboseness
levels or it's intentional?
quoted hunk
 static bool is_btf_id(const char *name)
 {
@@ -526,8 +528,13 @@ static int symbols_resolve(struct object *obj)

                id = btf_id__find(root, str);
                if (id) {
-                       id->id = type_id;
-                       (*nr)--;
+                       if (id->id) {
+                               pr_info("WARN: multiple IDs found for '%s' (%d, %d)\n",
+                                       str, id->id, type_id);
+                       } else {
+                               id->id = type_id;
+                               (*nr)--;
btw, there is a nasty shadowing of nr variable, which is used both for
the for() loop condition (as int) and as `int *` inside the loop body.
It's better to rename inner (or outer) nr, it's extremely confusing as
is.

+                       }
                }
        }

--
2.26.2

Re: [PATCH bpf-next] tools/resolve_btfids: Warn when having multiple IDs for single type

From: Jiri Olsa <hidden>
Date: 2021-01-05 22:49:06 

On Tue, Jan 05, 2021 at 01:15:39PM -0800, Andrii Nakryiko wrote:
On Tue, Jan 5, 2021 at 7:41 AM Jiri Olsa [off-list ref] wrote:
quoted
The kernel image can contain multiple types (structs/unions)
with the same name. This causes distinct type hierarchies in
BTF data and makes resolve_btfids fail with error like:

  BTFIDS  vmlinux
FAILED unresolved symbol udp6_sock

as reported by Qais Yousef [1].

This change adds warning when multiple types of the same name
are detected:

  BTFIDS  vmlinux
WARN: multiple IDs found for 'file' (526, 113351)
WARN: multiple IDs found for 'sk_buff' (2744, 113958)

We keep the lower ID for the given type instance and let the
build continue.

[1] https://lore.kernel.org/lkml/20201229151352.6hzmjvu3qh6p2qgg@e107158-lin/
Reported-by: Qais Yousef <redacted>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
see comments below, but otherwise lgtm

Acked-by: Andrii Nakryiko <andrii@kernel.org>
quoted
 tools/bpf/resolve_btfids/main.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
index e3ea569ee125..36a3b1024cdc 100644
--- a/tools/bpf/resolve_btfids/main.c
+++ b/tools/bpf/resolve_btfids/main.c
@@ -139,6 +139,8 @@ int eprintf(int level, int var, const char *fmt, ...)
 #define pr_debug2(fmt, ...) pr_debugN(2, pr_fmt(fmt), ##__VA_ARGS__)
 #define pr_err(fmt, ...) \
        eprintf(0, verbose, pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_info(fmt, ...) \
+       eprintf(0, verbose, pr_fmt(fmt), ##__VA_ARGS__)
how is it different from pr_err? Did you forget to update verboseness
levels or it's intentional?
intentional, I'm using pr_err to print in error paths,
so I wanted to add new one for other 'info' messages without -v

quoted
 static bool is_btf_id(const char *name)
 {
@@ -526,8 +528,13 @@ static int symbols_resolve(struct object *obj)

                id = btf_id__find(root, str);
                if (id) {
-                       id->id = type_id;
-                       (*nr)--;
+                       if (id->id) {
+                               pr_info("WARN: multiple IDs found for '%s' (%d, %d)\n",
+                                       str, id->id, type_id);
+                       } else {
+                               id->id = type_id;
+                               (*nr)--;
btw, there is a nasty shadowing of nr variable, which is used both for
the for() loop condition (as int) and as `int *` inside the loop body.
It's better to rename inner (or outer) nr, it's extremely confusing as
is.
nice, I'll change that

thanks,
jirka

quoted
+                       }
                }
        }

--
2.26.2




    
  

  
    

      

        Keyboard shortcuts
      

      

        
          
hback out one level

          
jnext message in thread

          
kprevious message in thread

          
ldrill in

          
Escclose help / fold thread tree

          
?toggle this help