[PATCH bpf v2 0/3] Fixes for kfunc-mod regressions and warnings

[PATCH bpf v2 0/3] Fixes for kfunc-mod regressions and warnings

[Kumar Kartikeya Dwivedi]

This set includes fixes for two regressions and one build warning introduced by
the kfunc for modules series.

Changelog:
----------

v1 -> v2:
v1: https://lore.kernel.org/bpf/20211115191840.496263-1-memxor@gmail.com

 * Instead of demoting resolve_btfids warning to debug, only skip in case of
   set->cnt == 0.

Kumar Kartikeya Dwivedi (3):
  bpf: Make CONFIG_DEBUG_INFO_BTF depend upon CONFIG_BPF_SYSCALL
  bpf: Fix bpf_check_mod_kfunc_call for built-in modules
  tools/resolve_btfids: Skip unresolved symbol warning for empty BTF
    sets

 include/linux/btf.h             | 14 ++++++++++----
 kernel/bpf/btf.c                | 11 ++---------
 lib/Kconfig.debug               |  1 +
 tools/bpf/resolve_btfids/main.c |  8 +++++---
 4 files changed, 18 insertions(+), 16 deletions(-)

-- 
2.34.0

[PATCH bpf v2 1/3] bpf: Make CONFIG_DEBUG_INFO_BTF depend upon CONFIG_BPF_SYSCALL

[Kumar Kartikeya Dwivedi]

Vinicius Costa Gomes reported [0] that build fails when
CONFIG_DEBUG_INFO_BTF is enabled and CONFIG_BPF_SYSCALL is disabled.
This leads to btf.c not being compiled, and then no symbol being present
in vmlinux for the declarations in btf.h. Since BTF is not useful
without enabling BPF subsystem, disallow this combination.

However, theoretically disabling both now could still fail, as the
symbol for kfunc_btf_id_list variables is not available. This isn't a
problem as the compiler usually optimizes the whole register/unregister
call, but at lower optimization levels it can fail the build in linking
stage.

Fix that by adding dummy variables so that modules taking address of
them still work, but the whole thing is a noop.

  [0]: https://lore.kernel.org/bpf/20211110205418.332403-1-vinicius.gomes@intel.com

Fixes: 14f267d95fe4 ("bpf: btf: Introduce helpers for dynamic BTF set registration")
Reported-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/btf.h | 14 ++++++++++----
 kernel/bpf/btf.c    |  9 ++-------
 lib/Kconfig.debug   |  1 +
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/include/linux/btf.h b/include/linux/btf.h
index 203eef993d76..0e1b6281fd8f 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -245,7 +245,10 @@ struct kfunc_btf_id_set {
 	struct module *owner;
 };
 
-struct kfunc_btf_id_list;
+struct kfunc_btf_id_list {
+	struct list_head list;
+	struct mutex mutex;
+};
 
 #ifdef CONFIG_DEBUG_INFO_BTF_MODULES
 void register_kfunc_btf_id_set(struct kfunc_btf_id_list *l,
@@ -254,6 +257,9 @@ void unregister_kfunc_btf_id_set(struct kfunc_btf_id_list *l,
 				 struct kfunc_btf_id_set *s);
 bool bpf_check_mod_kfunc_call(struct kfunc_btf_id_list *klist, u32 kfunc_id,
 			      struct module *owner);
+
+extern struct kfunc_btf_id_list bpf_tcp_ca_kfunc_list;
+extern struct kfunc_btf_id_list prog_test_kfunc_list;
 #else
 static inline void register_kfunc_btf_id_set(struct kfunc_btf_id_list *l,
 					     struct kfunc_btf_id_set *s)
@@ -268,13 +274,13 @@ static inline bool bpf_check_mod_kfunc_call(struct kfunc_btf_id_list *klist,
 {
 	return false;
 }
+
+static struct kfunc_btf_id_list bpf_tcp_ca_kfunc_list __maybe_unused;
+static struct kfunc_btf_id_list prog_test_kfunc_list __maybe_unused;
 #endif
 
 #define DEFINE_KFUNC_BTF_ID_SET(set, name)                                     \
 	struct kfunc_btf_id_set name = { LIST_HEAD_INIT(name.list), (set),     \
 					 THIS_MODULE }
 
-extern struct kfunc_btf_id_list bpf_tcp_ca_kfunc_list;
-extern struct kfunc_btf_id_list prog_test_kfunc_list;
-
 #endif
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index dbc3ad07e21b..ea3df9867cec 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6346,11 +6346,6 @@ BTF_ID_LIST_GLOBAL_SINGLE(btf_task_struct_ids, struct, task_struct)
 
 /* BTF ID set registration API for modules */
 
-struct kfunc_btf_id_list {
-	struct list_head list;
-	struct mutex mutex;
-};
-
 #ifdef CONFIG_DEBUG_INFO_BTF_MODULES
 
 void register_kfunc_btf_id_set(struct kfunc_btf_id_list *l,
@@ -6389,8 +6384,6 @@ bool bpf_check_mod_kfunc_call(struct kfunc_btf_id_list *klist, u32 kfunc_id,
 	return false;
 }
 
-#endif
-
 #define DEFINE_KFUNC_BTF_ID_LIST(name)                                         \
 	struct kfunc_btf_id_list name = { LIST_HEAD_INIT(name.list),           \
 					  __MUTEX_INITIALIZER(name.mutex) };   \
@@ -6398,3 +6391,5 @@ bool bpf_check_mod_kfunc_call(struct kfunc_btf_id_list *klist, u32 kfunc_id,
 
 DEFINE_KFUNC_BTF_ID_LIST(bpf_tcp_ca_kfunc_list);
 DEFINE_KFUNC_BTF_ID_LIST(prog_test_kfunc_list);
+
+#endif
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 9ef7ce18b4f5..596bb5e4790c 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -316,6 +316,7 @@ config DEBUG_INFO_BTF
 	bool "Generate BTF typeinfo"
 	depends on !DEBUG_INFO_SPLIT && !DEBUG_INFO_REDUCED
 	depends on !GCC_PLUGIN_RANDSTRUCT || COMPILE_TEST
+	depends on BPF_SYSCALL
 	help
 	  Generate deduplicated BTF type information from DWARF debug info.
 	  Turning this on expects presence of pahole tool, which will convert
-- 
2.34.0

Re: [PATCH bpf v2 1/3] bpf: Make CONFIG_DEBUG_INFO_BTF depend upon CONFIG_BPF_SYSCALL

[Song Liu]

On Mon, Nov 22, 2021 at 6:47 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> Vinicius Costa Gomes reported [0] that build fails when
> CONFIG_DEBUG_INFO_BTF is enabled and CONFIG_BPF_SYSCALL is disabled.
> This leads to btf.c not being compiled, and then no symbol being present
> in vmlinux for the declarations in btf.h. Since BTF is not useful
> without enabling BPF subsystem, disallow this combination.
>
> However, theoretically disabling both now could still fail, as the
> symbol for kfunc_btf_id_list variables is not available. This isn't a
> problem as the compiler usually optimizes the whole register/unregister
> call, but at lower optimization levels it can fail the build in linking
> stage.
>
> Fix that by adding dummy variables so that modules taking address of
> them still work, but the whole thing is a noop.
>
>   [0]: https://lore.kernel.org/bpf/20211110205418.332403-1-vinicius.gomes@intel.com
>
> Fixes: 14f267d95fe4 ("bpf: btf: Introduce helpers for dynamic BTF set registration")
> Reported-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

Acked-by: Song Liu <songliubraving@fb.com>

Re: [PATCH bpf v2 1/3] bpf: Make CONFIG_DEBUG_INFO_BTF depend upon CONFIG_BPF_SYSCALL

[Geert Uytterhoeven]

 	Hi Kumar,

On Mon, 22 Nov 2021, Kumar Kartikeya Dwivedi wrote:
> Vinicius Costa Gomes reported [0] that build fails when
> CONFIG_DEBUG_INFO_BTF is enabled and CONFIG_BPF_SYSCALL is disabled.
> This leads to btf.c not being compiled, and then no symbol being present
> in vmlinux for the declarations in btf.h. Since BTF is not useful
> without enabling BPF subsystem, disallow this combination.
>
> However, theoretically disabling both now could still fail, as the
> symbol for kfunc_btf_id_list variables is not available. This isn't a
> problem as the compiler usually optimizes the whole register/unregister
> call, but at lower optimization levels it can fail the build in linking
> stage.
>
> Fix that by adding dummy variables so that modules taking address of
> them still work, but the whole thing is a noop.
>
>  [0]: https://lore.kernel.org/bpf/20211110205418.332403-1-vinicius.gomes@intel.com
>
> Fixes: 14f267d95fe4 ("bpf: btf: Introduce helpers for dynamic BTF set registration")
> Reported-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

Thanks for your patch, which is now commit d9847eb8be3d895b ("bpf:
Make CONFIG_DEBUG_INFO_BTF depend upon CONFIG_BPF_SYSCALL") in v5.16.

> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -316,6 +316,7 @@ config DEBUG_INFO_BTF
> 	bool "Generate BTF typeinfo"
> 	depends on !DEBUG_INFO_SPLIT && !DEBUG_INFO_REDUCED
> 	depends on !GCC_PLUGIN_RANDSTRUCT || COMPILE_TEST
> +	depends on BPF_SYSCALL
> 	help
> 	  Generate deduplicated BTF type information from DWARF debug info.
> 	  Turning this on expects presence of pahole tool, which will convert

I wanted to run pahole on a kernel object file, but was greeted by an
error message:

     libbpf: failed to find '.BTF' ELF section in <foo>.o

Then I discovered I could not enable CONFIG_DEBUG_INFO_BTF without also
enabling BPF_SYSCALL, which looks totally unrelated to me.  So yes,
there seems to be a use case for BTF without enabling the BPF subsystem.

Thanks!

Gr{oetje,eeting}s,

 						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 							    -- Linus Torvalds

[PATCH bpf v2 2/3] bpf: Fix bpf_check_mod_kfunc_call for built-in modules

[Kumar Kartikeya Dwivedi]

When module registering its set is built-in, THIS_MODULE will be NULL,
hence we cannot return early in case owner is NULL.

Fixes: 14f267d95fe4 ("bpf: btf: Introduce helpers for dynamic BTF set registration")
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/btf.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index ea3df9867cec..9bdb03767db5 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6371,8 +6371,6 @@ bool bpf_check_mod_kfunc_call(struct kfunc_btf_id_list *klist, u32 kfunc_id,
 {
 	struct kfunc_btf_id_set *s;
 
-	if (!owner)
-		return false;
 	mutex_lock(&klist->mutex);
 	list_for_each_entry(s, &klist->list, list) {
 		if (s->owner == owner && btf_id_set_contains(s->set, kfunc_id)) {
-- 
2.34.0

Re: [PATCH bpf v2 2/3] bpf: Fix bpf_check_mod_kfunc_call for built-in modules

[Song Liu]

On Mon, Nov 22, 2021 at 6:47 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> When module registering its set is built-in, THIS_MODULE will be NULL,
> hence we cannot return early in case owner is NULL.
>
> Fixes: 14f267d95fe4 ("bpf: btf: Introduce helpers for dynamic BTF set registration")
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

Acked-by: Song Liu <songliubraving@fb.com>

[PATCH bpf v2 3/3] tools/resolve_btfids: Skip unresolved symbol warning for empty BTF sets

[Kumar Kartikeya Dwivedi]

resolve_btfids prints a warning when it finds an unresolved symbol,
(id == 0) in id_patch. This can be the case for BTF sets that are empty
(due to disabled config options), hence printing warnings for certain
builds, most recently seen in [0].

The reason behind this is because id->cnt aliases id->id in btf_id
struct, leading to empty set showing up as ID 0 when we get to id_patch,
which triggers the warning. Since sets are an exception here, accomodate
by reusing hole in btf_id for bool is_set member, setting it to true for
BTF set when setting id->cnt, and use that to skip extraneous warning.

  [0]: https://lore.kernel.org/all/1b99ae14-abb4-d18f-cc6a-d7e523b25542@gmail.com

Before:

; ./tools/bpf/resolve_btfids/resolve_btfids -v -b vmlinux net/ipv4/tcp_cubic.ko
adding symbol tcp_cubic_kfunc_ids
WARN: resolve_btfids: unresolved symbol tcp_cubic_kfunc_ids
patching addr     0: ID       0 [tcp_cubic_kfunc_ids]
sorting  addr     4: cnt      0 [tcp_cubic_kfunc_ids]
update ok for net/ipv4/tcp_cubic.ko

After:

; ./tools/bpf/resolve_btfids/resolve_btfids -v -b vmlinux net/ipv4/tcp_cubic.ko
adding symbol tcp_cubic_kfunc_ids
patching addr     0: ID       0 [tcp_cubic_kfunc_ids]
sorting  addr     4: cnt      0 [tcp_cubic_kfunc_ids]
update ok for net/ipv4/tcp_cubic.ko

Cc: Jiri Olsa <jolsa@kernel.org>
Fixes: 0e32dfc80bae ("bpf: Enable TCP congestion control kfunc from modules")
Reported-by: Pavel Skripkin <paskripkin@gmail.com>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 tools/bpf/resolve_btfids/main.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
index a59cb0ee609c..73409e27be01 100644
--- a/tools/bpf/resolve_btfids/main.c
+++ b/tools/bpf/resolve_btfids/main.c
@@ -83,6 +83,7 @@ struct btf_id {
 		int	 cnt;
 	};
 	int		 addr_cnt;
+	bool		 is_set;
 	Elf64_Addr	 addr[ADDR_CNT];
 };
 
@@ -451,8 +452,10 @@ static int symbols_collect(struct object *obj)
 			 * in symbol's size, together with 'cnt' field hence
 			 * that - 1.
 			 */
-			if (id)
+			if (id) {
 				id->cnt = sym.st_size / sizeof(int) - 1;
+				id->is_set = true;
+			}
 		} else {
 			pr_err("FAILED unsupported prefix %s\n", prefix);
 			return -1;
@@ -568,9 +571,8 @@ static int id_patch(struct object *obj, struct btf_id *id)
 	int *ptr = data->d_buf;
 	int i;
 
-	if (!id->id) {
+	if (!id->id && !id->is_set)
 		pr_err("WARN: resolve_btfids: unresolved symbol %s\n", id->name);
-	}
 
 	for (i = 0; i < id->addr_cnt; i++) {
 		unsigned long addr = id->addr[i];
-- 
2.34.0

Re: [PATCH bpf v2 3/3] tools/resolve_btfids: Skip unresolved symbol warning for empty BTF sets

[Song Liu]

On Mon, Nov 22, 2021 at 6:47 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> resolve_btfids prints a warning when it finds an unresolved symbol,
> (id == 0) in id_patch. This can be the case for BTF sets that are empty
> (due to disabled config options), hence printing warnings for certain
> builds, most recently seen in [0].
>
> The reason behind this is because id->cnt aliases id->id in btf_id
> struct, leading to empty set showing up as ID 0 when we get to id_patch,
> which triggers the warning. Since sets are an exception here, accomodate
> by reusing hole in btf_id for bool is_set member, setting it to true for
> BTF set when setting id->cnt, and use that to skip extraneous warning.
>
>   [0]: https://lore.kernel.org/all/1b99ae14-abb4-d18f-cc6a-d7e523b25542@gmail.com
>
> Before:
>
> ; ./tools/bpf/resolve_btfids/resolve_btfids -v -b vmlinux net/ipv4/tcp_cubic.ko
> adding symbol tcp_cubic_kfunc_ids
> WARN: resolve_btfids: unresolved symbol tcp_cubic_kfunc_ids
> patching addr     0: ID       0 [tcp_cubic_kfunc_ids]
> sorting  addr     4: cnt      0 [tcp_cubic_kfunc_ids]
> update ok for net/ipv4/tcp_cubic.ko
>
> After:
>
> ; ./tools/bpf/resolve_btfids/resolve_btfids -v -b vmlinux net/ipv4/tcp_cubic.ko
> adding symbol tcp_cubic_kfunc_ids
> patching addr     0: ID       0 [tcp_cubic_kfunc_ids]
> sorting  addr     4: cnt      0 [tcp_cubic_kfunc_ids]
> update ok for net/ipv4/tcp_cubic.ko
>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Fixes: 0e32dfc80bae ("bpf: Enable TCP congestion control kfunc from modules")
> Reported-by: Pavel Skripkin <paskripkin@gmail.com>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

Acked-by: Song Liu <songliubraving@fb.com>

Re: [PATCH bpf v2 3/3] tools/resolve_btfids: Skip unresolved symbol warning for empty BTF sets

[Andrii Nakryiko]

On Mon, Nov 22, 2021 at 6:47 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> resolve_btfids prints a warning when it finds an unresolved symbol,
> (id == 0) in id_patch. This can be the case for BTF sets that are empty
> (due to disabled config options), hence printing warnings for certain
> builds, most recently seen in [0].
>
> The reason behind this is because id->cnt aliases id->id in btf_id

do we need to alias this, btw? We are trying to save 4 bytes on 800+
struct (addr[ADDR_CNT] is big) and instead are getting more confusion
in the code.

> struct, leading to empty set showing up as ID 0 when we get to id_patch,
> which triggers the warning. Since sets are an exception here, accomodate
> by reusing hole in btf_id for bool is_set member, setting it to true for
> BTF set when setting id->cnt, and use that to skip extraneous warning.
>
>   [0]: https://lore.kernel.org/all/1b99ae14-abb4-d18f-cc6a-d7e523b25542@gmail.com
>
> Before:
>
> ; ./tools/bpf/resolve_btfids/resolve_btfids -v -b vmlinux net/ipv4/tcp_cubic.ko
> adding symbol tcp_cubic_kfunc_ids
> WARN: resolve_btfids: unresolved symbol tcp_cubic_kfunc_ids
> patching addr     0: ID       0 [tcp_cubic_kfunc_ids]
> sorting  addr     4: cnt      0 [tcp_cubic_kfunc_ids]
> update ok for net/ipv4/tcp_cubic.ko
>
> After:
>
> ; ./tools/bpf/resolve_btfids/resolve_btfids -v -b vmlinux net/ipv4/tcp_cubic.ko
> adding symbol tcp_cubic_kfunc_ids
> patching addr     0: ID       0 [tcp_cubic_kfunc_ids]
> sorting  addr     4: cnt      0 [tcp_cubic_kfunc_ids]
> update ok for net/ipv4/tcp_cubic.ko
>
> Cc: Jiri Olsa <jolsa@kernel.org>

Jiri, can you please take a look as well?

> Fixes: 0e32dfc80bae ("bpf: Enable TCP congestion control kfunc from modules")
> Reported-by: Pavel Skripkin <paskripkin@gmail.com>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  tools/bpf/resolve_btfids/main.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> index a59cb0ee609c..73409e27be01 100644
> --- a/tools/bpf/resolve_btfids/main.c
> +++ b/tools/bpf/resolve_btfids/main.c
> @@ -83,6 +83,7 @@ struct btf_id {
>                 int      cnt;
>         };
>         int              addr_cnt;
> +       bool             is_set;
>         Elf64_Addr       addr[ADDR_CNT];
>  };
>
> @@ -451,8 +452,10 @@ static int symbols_collect(struct object *obj)
>                          * in symbol's size, together with 'cnt' field hence
>                          * that - 1.
>                          */
> -                       if (id)
> +                       if (id) {
>                                 id->cnt = sym.st_size / sizeof(int) - 1;
> +                               id->is_set = true;
> +                       }
>                 } else {
>                         pr_err("FAILED unsupported prefix %s\n", prefix);
>                         return -1;
> @@ -568,9 +571,8 @@ static int id_patch(struct object *obj, struct btf_id *id)
>         int *ptr = data->d_buf;
>         int i;
>
> -       if (!id->id) {
> +       if (!id->id && !id->is_set)
>                 pr_err("WARN: resolve_btfids: unresolved symbol %s\n", id->name);
> -       }
>
>         for (i = 0; i < id->addr_cnt; i++) {
>                 unsigned long addr = id->addr[i];
> --
> 2.34.0
>

Re: [PATCH bpf v2 3/3] tools/resolve_btfids: Skip unresolved symbol warning for empty BTF sets

[Kumar Kartikeya Dwivedi]

On Tue, Nov 30, 2021 at 05:16:46AM IST, Andrii Nakryiko wrote:
> On Mon, Nov 22, 2021 at 6:47 AM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > resolve_btfids prints a warning when it finds an unresolved symbol,
> > (id == 0) in id_patch. This can be the case for BTF sets that are empty
> > (due to disabled config options), hence printing warnings for certain
> > builds, most recently seen in [0].
> >
> > The reason behind this is because id->cnt aliases id->id in btf_id
>
> do we need to alias this, btw? We are trying to save 4 bytes on 800+
> struct (addr[ADDR_CNT] is big) and instead are getting more confusion
> in the code.
>

I can do that, but going to wait for Jiri's response before respinning.

> > struct, leading to empty set showing up as ID 0 when we get to id_patch,
> > which triggers the warning. Since sets are an exception here, accomodate
> > by reusing hole in btf_id for bool is_set member, setting it to true for
> > BTF set when setting id->cnt, and use that to skip extraneous warning.
> >
> >   [0]: https://lore.kernel.org/all/1b99ae14-abb4-d18f-cc6a-d7e523b25542@gmail.com
> >
> > Before:
> >
> > ; ./tools/bpf/resolve_btfids/resolve_btfids -v -b vmlinux net/ipv4/tcp_cubic.ko
> > adding symbol tcp_cubic_kfunc_ids
> > WARN: resolve_btfids: unresolved symbol tcp_cubic_kfunc_ids
> > patching addr     0: ID       0 [tcp_cubic_kfunc_ids]
> > sorting  addr     4: cnt      0 [tcp_cubic_kfunc_ids]
> > update ok for net/ipv4/tcp_cubic.ko
> >
> > After:
> >
> > ; ./tools/bpf/resolve_btfids/resolve_btfids -v -b vmlinux net/ipv4/tcp_cubic.ko
> > adding symbol tcp_cubic_kfunc_ids
> > patching addr     0: ID       0 [tcp_cubic_kfunc_ids]
> > sorting  addr     4: cnt      0 [tcp_cubic_kfunc_ids]
> > update ok for net/ipv4/tcp_cubic.ko
> >
> > Cc: Jiri Olsa <jolsa@kernel.org>
>
> Jiri, can you please take a look as well?
>
> > Fixes: 0e32dfc80bae ("bpf: Enable TCP congestion control kfunc from modules")
> > Reported-by: Pavel Skripkin <paskripkin@gmail.com>
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> >  tools/bpf/resolve_btfids/main.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> > index a59cb0ee609c..73409e27be01 100644
> > --- a/tools/bpf/resolve_btfids/main.c
> > +++ b/tools/bpf/resolve_btfids/main.c
> > @@ -83,6 +83,7 @@ struct btf_id {
> >                 int      cnt;
> >         };
> >         int              addr_cnt;
> > +       bool             is_set;
> >         Elf64_Addr       addr[ADDR_CNT];
> >  };
> >
> > @@ -451,8 +452,10 @@ static int symbols_collect(struct object *obj)
> >                          * in symbol's size, together with 'cnt' field hence
> >                          * that - 1.
> >                          */
> > -                       if (id)
> > +                       if (id) {
> >                                 id->cnt = sym.st_size / sizeof(int) - 1;
> > +                               id->is_set = true;
> > +                       }
> >                 } else {
> >                         pr_err("FAILED unsupported prefix %s\n", prefix);
> >                         return -1;
> > @@ -568,9 +571,8 @@ static int id_patch(struct object *obj, struct btf_id *id)
> >         int *ptr = data->d_buf;
> >         int i;
> >
> > -       if (!id->id) {
> > +       if (!id->id && !id->is_set)
> >                 pr_err("WARN: resolve_btfids: unresolved symbol %s\n", id->name);
> > -       }
> >
> >         for (i = 0; i < id->addr_cnt; i++) {
> >                 unsigned long addr = id->addr[i];
> > --
> > 2.34.0
> >

--
Kartikeya

Re: [PATCH bpf v2 3/3] tools/resolve_btfids: Skip unresolved symbol warning for empty BTF sets

[Andrii Nakryiko]

On Tue, Nov 30, 2021 at 11:56 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Tue, Nov 30, 2021 at 05:16:46AM IST, Andrii Nakryiko wrote:
> > On Mon, Nov 22, 2021 at 6:47 AM Kumar Kartikeya Dwivedi
> > <memxor@gmail.com> wrote:
> > >
> > > resolve_btfids prints a warning when it finds an unresolved symbol,
> > > (id == 0) in id_patch. This can be the case for BTF sets that are empty
> > > (due to disabled config options), hence printing warnings for certain
> > > builds, most recently seen in [0].
> > >
> > > The reason behind this is because id->cnt aliases id->id in btf_id
> >
> > do we need to alias this, btw? We are trying to save 4 bytes on 800+
> > struct (addr[ADDR_CNT] is big) and instead are getting more confusion
> > in the code.
> >
>
> I can do that, but going to wait for Jiri's response before respinning.
>

Alright, I've applied the patch set to bpf tree. Please follow up with
the discussed improvements separately. Thanks.

> > > struct, leading to empty set showing up as ID 0 when we get to id_patch,
> > > which triggers the warning. Since sets are an exception here, accomodate
> > > by reusing hole in btf_id for bool is_set member, setting it to true for
> > > BTF set when setting id->cnt, and use that to skip extraneous warning.
> > >
> > >   [0]: https://lore.kernel.org/all/1b99ae14-abb4-d18f-cc6a-d7e523b25542@gmail.com
> > >
> > > Before:
> > >
> > > ; ./tools/bpf/resolve_btfids/resolve_btfids -v -b vmlinux net/ipv4/tcp_cubic.ko
> > > adding symbol tcp_cubic_kfunc_ids
> > > WARN: resolve_btfids: unresolved symbol tcp_cubic_kfunc_ids
> > > patching addr     0: ID       0 [tcp_cubic_kfunc_ids]
> > > sorting  addr     4: cnt      0 [tcp_cubic_kfunc_ids]
> > > update ok for net/ipv4/tcp_cubic.ko
> > >
> > > After:
> > >
> > > ; ./tools/bpf/resolve_btfids/resolve_btfids -v -b vmlinux net/ipv4/tcp_cubic.ko
> > > adding symbol tcp_cubic_kfunc_ids
> > > patching addr     0: ID       0 [tcp_cubic_kfunc_ids]
> > > sorting  addr     4: cnt      0 [tcp_cubic_kfunc_ids]
> > > update ok for net/ipv4/tcp_cubic.ko
> > >
> > > Cc: Jiri Olsa <jolsa@kernel.org>
> >
> > Jiri, can you please take a look as well?
> >
> > > Fixes: 0e32dfc80bae ("bpf: Enable TCP congestion control kfunc from modules")
> > > Reported-by: Pavel Skripkin <paskripkin@gmail.com>
> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > > ---
> > >  tools/bpf/resolve_btfids/main.c | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> > > index a59cb0ee609c..73409e27be01 100644
> > > --- a/tools/bpf/resolve_btfids/main.c
> > > +++ b/tools/bpf/resolve_btfids/main.c
> > > @@ -83,6 +83,7 @@ struct btf_id {
> > >                 int      cnt;
> > >         };
> > >         int              addr_cnt;
> > > +       bool             is_set;
> > >         Elf64_Addr       addr[ADDR_CNT];
> > >  };
> > >
> > > @@ -451,8 +452,10 @@ static int symbols_collect(struct object *obj)
> > >                          * in symbol's size, together with 'cnt' field hence
> > >                          * that - 1.
> > >                          */
> > > -                       if (id)
> > > +                       if (id) {
> > >                                 id->cnt = sym.st_size / sizeof(int) - 1;
> > > +                               id->is_set = true;
> > > +                       }
> > >                 } else {
> > >                         pr_err("FAILED unsupported prefix %s\n", prefix);
> > >                         return -1;
> > > @@ -568,9 +571,8 @@ static int id_patch(struct object *obj, struct btf_id *id)
> > >         int *ptr = data->d_buf;
> > >         int i;
> > >
> > > -       if (!id->id) {
> > > +       if (!id->id && !id->is_set)
> > >                 pr_err("WARN: resolve_btfids: unresolved symbol %s\n", id->name);
> > > -       }
> > >
> > >         for (i = 0; i < id->addr_cnt; i++) {
> > >                 unsigned long addr = id->addr[i];
> > > --
> > > 2.34.0
> > >
>
> --
> Kartikeya

Re: [PATCH bpf v2 0/3] Fixes for kfunc-mod regressions and warnings

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Mon, 22 Nov 2021 20:17:39 +0530 you wrote:
> This set includes fixes for two regressions and one build warning introduced by
> the kfunc for modules series.
> 
> Changelog:
> ----------
> 
> v1 -> v2:
> v1: https://lore.kernel.org/bpf/20211115191840.496263-1-memxor@gmail.com
> 
> [...]

Here is the summary with links:
  - [bpf,v2,1/3] bpf: Make CONFIG_DEBUG_INFO_BTF depend upon CONFIG_BPF_SYSCALL
    https://git.kernel.org/bpf/bpf/c/d9847eb8be3d
  - [bpf,v2,2/3] bpf: Fix bpf_check_mod_kfunc_call for built-in modules
    https://git.kernel.org/bpf/bpf/c/b12f03104324
  - [bpf,v2,3/3] tools/resolve_btfids: Skip unresolved symbol warning for empty BTF sets
    https://git.kernel.org/bpf/bpf/c/3345193f6f3c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html