btf: warn but return no error for NULL btf from __register_btf_kfunc_id_set()

PATCH Superseded SeongJae Park 2 participants 6 messages Jun 28, 2023 – Jul 3, 2023

Subsystems (2)

Stable branch BPF

lore

Share & tools

lore

SeongJae Park

author

2023-06-28 16:46 UTC #0 lore raw

PATCH v2 1/1 btf: warn but return no error for NULL btf from __register_btf_kfunc_id_set()

__register_btf_kfunc_id_set() assumes .BTF to be part of the module's .ko file if CONFIG_DEBUG_INFO_BTF is enabled. If that's not the case, the function prints an error message and return an error. As a result, such modules cannot be loaded.

However, the section could be stripped out during a build process. It would be better to let the modules loaded, because their basic functionalities have no problem[1], though the BTF functionalities will not be supported. Make the function to lower the level of the message from error to warn, and return no error.

[1] https://lore.kernel.org/bpf/[email protected]/

Reported-by Alexander Egorenkov <[email protected]>

Link https://lore.kernel.org/bpf/[email protected]/ open here

Fixes c446fdacb10d ("bpf: fix register_btf_kfunc_id_set for !CONFIG_DEBUG_INFO_BTF")

Cc <[email protected]> # 5.18.x

Signed-off-by SeongJae Park <[email protected]>

Acked-by Jiri Olsa <[email protected]>

---
Changes from v1
(https://lore.kernel.org/all/[email protected]/)
- Fix Fixes: tag (Jiri Olsa)
- Add 'Acked-by: ' from Jiri Olsa

 kernel/bpf/btf.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

kernel/bpf/btf.c +4 -8

index 6b682b8e4b50..d683f034996f 100644
@@ -7848,14 +7848,10 @@ static int __register_btf_kfunc_id_set(enum btf_kfunc_hook hook,
 
 	btf = btf_get_module_btf(kset->owner);
 	if (!btf) {
-		if (!kset->owner && IS_ENABLED(CONFIG_DEBUG_INFO_BTF)) {
-			pr_err("missing vmlinux BTF, cannot register kfuncs\n");
-			return -ENOENT;
-		}
-		if (kset->owner && IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES)) {
-			pr_err("missing module BTF, cannot register kfuncs\n");
-			return -ENOENT;
-		}
+		if (!kset->owner && IS_ENABLED(CONFIG_DEBUG_INFO_BTF))
+			pr_warn("missing vmlinux BTF, cannot register kfuncs\n");
+		if (kset->owner && IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES))
+			pr_warn("missing module BTF, cannot register kfuncs\n");
 		return 0;
 	}
 	if (IS_ERR(btf))

Signature · 1 line

2.25.1

Daniel Borkmann

maintainer

2023-06-30 14:53 UTC #1 lore raw

Quoted 13 lines · SeongJae Park

__register_btf_kfunc_id_set() assumes .BTF to be part of the module's .ko file if CONFIG_DEBUG_INFO_BTF is enabled. If that's not the case, the function prints an error message and return an error. As a result, such modules cannot be loaded.
However, the section could be stripped out during a build process. It would be better to let the modules loaded, because their basic functionalities have no problem[1], though the BTF functionalities will not be supported. Make the function to lower the level of the message from error to warn, and return no error.
[1] https://lore.kernel.org/bpf/[email protected]/

Quoted 8 lines

Reported-by Alexander Egorenkov <[email protected]>
Link https://lore.kernel.org/bpf/[email protected]/ open here
Suggested-by Kumar Kartikeya Dwivedi <[email protected]>
Link https://lore.kernel.org/bpf/[email protected]/ open here
Fixes c446fdacb10d ("bpf: fix register_btf_kfunc_id_set for !CONFIG_DEBUG_INFO_BTF")
Cc <[email protected]> # 5.18.x
Signed-off-by SeongJae Park <[email protected]>
Acked-by Jiri Olsa <[email protected]>

I presume this one is targeted at bpf (rather than bpf-next) tree, right?

kernel/bpf/btf.c static int __register_btf_kfunc_id_set(enum btf_kfunc_hook hook, · line 7854

Quoted 12 lines

kernel/bpf/btf.c +0 -4

index 6b682b8e4b50..d683f034996f 100644
@@ -7848,14 +7848,10 @@ static int __register_btf_kfunc_id_set(enum btf_kfunc_hook hook,
 
  	btf = btf_get_module_btf(kset->owner);
  	if (!btf) {
-		if (!kset->owner && IS_ENABLED(CONFIG_DEBUG_INFO_BTF)) {
-			pr_err("missing vmlinux BTF, cannot register kfuncs\n");
-			return -ENOENT;
-		}

Why the above one needs to be changed? Do you also run into this case? vmlinux BTF should be built-in in this case. I understand it's rather the one below for BTF + modules instead, no?

Quoted 12 lines

kernel/bpf/btf.c +4 -4

-		if (kset->owner && IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES)) {
-			pr_err("missing module BTF, cannot register kfuncs\n");
-			return -ENOENT;
-		}
+		if (!kset->owner && IS_ENABLED(CONFIG_DEBUG_INFO_BTF))
+			pr_warn("missing vmlinux BTF, cannot register kfuncs\n");
+		if (kset->owner && IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES))
+			pr_warn("missing module BTF, cannot register kfuncs\n");
  		return 0;
  	}
  	if (IS_ERR(btf))

SeongJae Park

author

2023-06-30 19:49 UTC #2 lore raw

Hi Daniel,

Quoted 23 lines · On Fri, 30 Jun 2023 16:53:38 +0200 Daniel Borkmann <[email protected]> wrote:

Quoted 13 lines · SeongJae Park
__register_btf_kfunc_id_set() assumes .BTF to be part of the module's .ko file if CONFIG_DEBUG_INFO_BTF is enabled. If that's not the case, the function prints an error message and return an error. As a result, such modules cannot be loaded.
However, the section could be stripped out during a build process. It would be better to let the modules loaded, because their basic functionalities have no problem[1], though the BTF functionalities will not be supported. Make the function to lower the level of the message from error to warn, and return no error.
[1] https://lore.kernel.org/bpf/[email protected]/
Quoted 8 lines
Reported-by Alexander Egorenkov <[email protected]>
Link https://lore.kernel.org/bpf/[email protected]/ open here
Suggested-by Kumar Kartikeya Dwivedi <[email protected]>
Link https://lore.kernel.org/bpf/[email protected]/ open here
Fixes c446fdacb10d ("bpf: fix register_btf_kfunc_id_set for !CONFIG_DEBUG_INFO_BTF")
Cc <[email protected]> # 5.18.x
Signed-off-by SeongJae Park <[email protected]>
Acked-by Jiri Olsa <[email protected]>

Quoted 1 line

I presume this one is targeted at bpf (rather than bpf-next) tree, right?

You're correct. It's not urgent for us, but I would prefer it to be merged into all affected kernels as early as possible.

Quoted 7 lines

kernel/bpf/btf.c +0 -0

index 6b682b8e4b50..d683f034996f 100644
@@ -7848,14 +7848,10 @@ static int __register_btf_kfunc_id_set(enum btf_kfunc_hook hook,

Quoted 10 lines

kernel/bpf/btf.c static int __register_btf_kfunc_id_set(enum btf_kfunc_hook hook, · line 7854
Quoted 6 lines
kernel/bpf/btf.c +0 -4
  	btf = btf_get_module_btf(kset->owner);
  	if (!btf) {
-		if (!kset->owner && IS_ENABLED(CONFIG_DEBUG_INFO_BTF)) {
-			pr_err("missing vmlinux BTF, cannot register kfuncs\n");
-			return -ENOENT;
-		}
Why the above one needs to be changed? Do you also run into this case? vmlinux BTF should be built-in in this case. I understand it's rather the one below for BTF + modules instead, no?

Again, you're correct. This change is not really needed. I was interpreting Kumar's suggestion merely into code without thinking about his real meaning, sorry. I will restore this in the next spin.

Thanks,
SJ

Quoted 15 lines

Quoted 13 lines

kernel/bpf/btf.c +4 -4

-		if (kset->owner && IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES)) {
-			pr_err("missing module BTF, cannot register kfuncs\n");
-			return -ENOENT;
-		}
+		if (!kset->owner && IS_ENABLED(CONFIG_DEBUG_INFO_BTF))
+			pr_warn("missing vmlinux BTF, cannot register kfuncs\n");
+		if (kset->owner && IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES))
+			pr_warn("missing module BTF, cannot register kfuncs\n");
  		return 0;
  	}
  	if (IS_ERR(btf))

Daniel Borkmann

maintainer

2023-06-30 20:52 UTC #3 lore raw

Quoted 24 lines · SeongJae Park

Quoted 23 lines · On Fri, 30 Jun 2023 16:53:38 +0200 Daniel Borkmann <[email protected]> wrote:
Quoted 21 lines · SeongJae Park
__register_btf_kfunc_id_set() assumes .BTF to be part of the module's .ko file if CONFIG_DEBUG_INFO_BTF is enabled. If that's not the case, the function prints an error message and return an error. As a result, such modules cannot be loaded.
However, the section could be stripped out during a build process. It would be better to let the modules loaded, because their basic functionalities have no problem[1], though the BTF functionalities will not be supported. Make the function to lower the level of the message from error to warn, and return no error.
[1] https://lore.kernel.org/bpf/[email protected]/
Reported-by Alexander Egorenkov <[email protected]>
Link https://lore.kernel.org/bpf/[email protected]/ open here
Suggested-by Kumar Kartikeya Dwivedi <[email protected]>
Link https://lore.kernel.org/bpf/[email protected]/ open here
Fixes c446fdacb10d ("bpf: fix register_btf_kfunc_id_set for !CONFIG_DEBUG_INFO_BTF")
Cc <[email protected]> # 5.18.x
Signed-off-by SeongJae Park <[email protected]>
Acked-by Jiri Olsa <[email protected]>

Quoted 4 lines

Quoted 1 line
I presume this one is targeted at bpf (rather than bpf-next) tree, right?
You're correct. It's not urgent for us, but I would prefer it to be merged into all affected kernels as early as possible.

Ok, sounds good, bpf tree it is then.

Quoted 17 lines

Quoted 6 lines
Quoted 6 lines
kernel/bpf/btf.c +0 -0
index 6b682b8e4b50..d683f034996f 100644
@@ -7848,14 +7848,10 @@ static int __register_btf_kfunc_id_set(enum btf_kfunc_hook hook,
 
Quoted 10 lines
kernel/bpf/btf.c static int __register_btf_kfunc_id_set(enum btf_kfunc_hook hook, · line 7854
Quoted 6 lines
kernel/bpf/btf.c +0 -4
   	btf = btf_get_module_btf(kset->owner);
   	if (!btf) {
-		if (!kset->owner && IS_ENABLED(CONFIG_DEBUG_INFO_BTF)) {
-			pr_err("missing vmlinux BTF, cannot register kfuncs\n");
-			return -ENOENT;
-		}
Why the above one needs to be changed? Do you also run into this case? vmlinux BTF should be built-in in this case. I understand it's rather the one below for BTF + modules instead, no?

Quoted 3 lines

Again, you're correct. This change is not really needed. I was interpreting Kumar's suggestion merely into code without thinking about his real meaning, sorry. I will restore this in the next spin.

Perfect, I think after your v3 respin it should be good to land.

Thanks,
Daniel

SeongJae Park

author

2023-06-30 21:02 UTC #4 lore raw

Quoted 49 lines · On Fri, 30 Jun 2023 22:52:24 +0200 Daniel Borkmann <[email protected]> wrote:

Quoted 24 lines · SeongJae Park
Quoted 23 lines · On Fri, 30 Jun 2023 16:53:38 +0200 Daniel Borkmann <[email protected]> wrote:
Quoted 21 lines · SeongJae Park
__register_btf_kfunc_id_set() assumes .BTF to be part of the module's .ko file if CONFIG_DEBUG_INFO_BTF is enabled. If that's not the case, the function prints an error message and return an error. As a result, such modules cannot be loaded.
However, the section could be stripped out during a build process. It would be better to let the modules loaded, because their basic functionalities have no problem[1], though the BTF functionalities will not be supported. Make the function to lower the level of the message from error to warn, and return no error.
[1] https://lore.kernel.org/bpf/[email protected]/
Reported-by Alexander Egorenkov <[email protected]>
Link https://lore.kernel.org/bpf/[email protected]/ open here
Suggested-by Kumar Kartikeya Dwivedi <[email protected]>
Link https://lore.kernel.org/bpf/[email protected]/ open here
Fixes c446fdacb10d ("bpf: fix register_btf_kfunc_id_set for !CONFIG_DEBUG_INFO_BTF")
Cc <[email protected]> # 5.18.x
Signed-off-by SeongJae Park <[email protected]>
Acked-by Jiri Olsa <[email protected]>
Quoted 4 lines
Quoted 1 line
I presume this one is targeted at bpf (rather than bpf-next) tree, right?
You're correct. It's not urgent for us, but I would prefer it to be merged into all affected kernels as early as possible.
Ok, sounds good, bpf tree it is then.
Quoted 17 lines
Quoted 16 lines
kernel/bpf/btf.c static int __register_btf_kfunc_id_set(enum btf_kfunc_hook hook, · line 7854
Quoted 12 lines
kernel/bpf/btf.c +0 -4
index 6b682b8e4b50..d683f034996f 100644
@@ -7848,14 +7848,10 @@ static int __register_btf_kfunc_id_set(enum btf_kfunc_hook hook,
 
   	btf = btf_get_module_btf(kset->owner);
   	if (!btf) {
-		if (!kset->owner && IS_ENABLED(CONFIG_DEBUG_INFO_BTF)) {
-			pr_err("missing vmlinux BTF, cannot register kfuncs\n");
-			return -ENOENT;
-		}
Why the above one needs to be changed? Do you also run into this case? vmlinux BTF should be built-in in this case. I understand it's rather the one below for BTF + modules instead, no?

Quoted 5 lines

Quoted 3 lines
Again, you're correct. This change is not really needed. I was interpreting Kumar's suggestion merely into code without thinking about his real meaning, sorry. I will restore this in the next spin.
Perfect, I think after your v3 respin it should be good to land.

Thank you! I will send it by tomorrow, to give people enough time to comment. If you don't want to wait, please let me know :)

Also, please note that this will not cleanly applicable on 6.1.y. I will provide the backport to stable@ as soon as this is merged into the mainline.

Thanks,
SJ

Quoted 4 lines

Thanks,
Daniel

Daniel Borkmann

maintainer

2023-07-03 16:51 UTC #5 lore raw

On 6/30/23 11:02 PM, SeongJae Park wrote:
[...]

Quoted 2 lines

Also, please note that this will not cleanly applicable on 6.1.y. I will provide the backport to stable@ as soon as this is merged into the mainline.

Perfect, thanks!