Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libbpf: further struct_ops fixes and improvements #6974

Closed

Conversation

kernel-patches-daemon-bpf[bot]
Copy link

Pull request for series with
subject: libbpf: further struct_ops fixes and improvements
version: 1
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=850977

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 329a672
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=850977
version: 1

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 75b0fbf
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=850977
version: 1

libbpf ensures that BPF program references set in map->st_ops->progs[i]
during open phase are always valid STRUCT_OPS programs. This is done in
bpf_object__collect_st_ops_relos(). So there is no need to double-check
that in bpf_map__init_kern_struct_ops().

Simplify the code by removing unnecessary check. Also, we avoid using
local prog variable to keep code similar to the upcoming fix, which adds
similar logic in another part of bpf_map__init_kern_struct_ops().

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
There is yet another corner case where user can set STRUCT_OPS program
reference in STRUCT_OPS map to NULL, but libbpf will fail to disable
autoload for such BPF program. This time it's the case of "new" kernel
which has type information about callback field, but user explicitly
nulled-out program reference from user-space after opening BPF object.

Fix, hopefully, the last remaining unhandled case.

Fixes: 0737df6 ("libbpf: better fix for handling nulled-out struct_ops program")
Fixes: f973fcc ("libbpf: handle nulled-out program in struct_ops correctly")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Add a test which tests the case that was just fixed. Kernel has full
type information about callback, but user explicitly nulls out the
reference to declaratively set BPF program reference.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
strerror_r(), used from libbpf-specific libbpf_strerror_r() wrapper is
documented to return error in two different ways, depending on glibc
version. Take that into account when handling strerror_r()'s own errors,
which happens when we pass some non-standard (internal) kernel error to
it. Before this patch we'd have "ERROR: strerror_r(524)=22", which is
quite confusing. Now for the same situation we'll see a bit less
visually scary "unknown error (-524)".

At least we won't confuse user with irrelevant EINVAL (22).

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Extend libbpf's pre-load checks for BPF programs, detecting more typical
conditions that are destinated to cause BPF program failure. This is an
opportunity to provide more helpful and actionable error message to
users, instead of potentially very confusing BPF verifier log and/or
error.

In this case, we detect struct_ops BPF program that was not referenced
anywhere, but still attempted to be loaded (according to libbpf logic).
Suggest that the program might need to be used in some struct_ops
variable. User will get a message of the following kind:

  libbpf: prog 'test_1_forgotten': SEC("struct_ops") program isn't referenced anywhere, did you forget to use it?

Suggested-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Add a simple test that validates that libbpf will reject isolated
struct_ops program early with helpful warning message.

Also validate that explicit use of such BPF program through BPF skeleton
after BPF object is open won't trigger any warnings.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 93d1c2d
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=850977
version: 1

Drive-by clean up, we shouldn't use meaningless "test_" prefix for
subtest names.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 7e2c7a3
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=850977
version: 1

Pull request is NOT updated. Failed to apply https://patchwork.kernel.org/project/netdevbpf/list/?series=850977
error message:

Cmd('git') failed due to: exit code(128)
  cmdline: git am --3way
  stdout: 'Applying: libbpf: remove unnecessary struct_ops prog validity check
Using index info to reconstruct a base tree...
M	tools/lib/bpf/libbpf.c
Falling back to patching base and 3-way merge...
Auto-merging tools/lib/bpf/libbpf.c
No changes -- Patch already applied.
Applying: libbpf: handle yet another corner case of nulling out struct_ops program
Using index info to reconstruct a base tree...
M	tools/lib/bpf/libbpf.c
Falling back to patching base and 3-way merge...
Auto-merging tools/lib/bpf/libbpf.c
No changes -- Patch already applied.
Applying: selftests/bpf: add another struct_ops callback use case test
Using index info to reconstruct a base tree...
M	tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
Falling back to patching base and 3-way merge...
Auto-merging tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
CONFLICT (content): Merge conflict in tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
Patch failed at 0003 selftests/bpf: add another struct_ops callback use case test
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".'
  stderr: '.git/rebase-apply/patch:85: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch'

conflict:

diff --cc tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
index 29e183a80f49,f3c61ebad323..000000000000
--- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
@@@ -5,7 -5,6 +5,10 @@@
  
  #include "struct_ops_module.skel.h"
  #include "struct_ops_nulled_out_cb.skel.h"
++<<<<<<< HEAD
 +#include "struct_ops_forgotten_cb.skel.h"
++=======
++>>>>>>> selftests/bpf: add another struct_ops callback use case test
  
  static void check_map_info(struct bpf_map_info *info)
  {
@@@ -200,59 -199,15 +203,67 @@@ cleanup
  	struct_ops_nulled_out_cb__destroy(skel);
  }
  
++<<<<<<< HEAD
 +/* validate that libbpf generates reasonable error message if struct_ops is
 + * not referenced in any struct_ops map
 + */
 +static void test_struct_ops_forgotten_cb(void)
 +{
 +	struct struct_ops_forgotten_cb *skel;
 +	char *log;
 +	int err;
 +
 +	skel = struct_ops_forgotten_cb__open();
 +	if (!ASSERT_OK_PTR(skel, "skel_open"))
 +		return;
 +
 +	start_libbpf_log_capture();
 +
 +	err = struct_ops_forgotten_cb__load(skel);
 +	if (!ASSERT_ERR(err, "skel_load"))
 +		goto cleanup;
 +
 +	log = stop_libbpf_log_capture();
 +	ASSERT_HAS_SUBSTR(log,
 +			  "prog 'test_1_forgotten': SEC(\"struct_ops\") program isn't referenced anywhere, did you forget to use it?",
 +			  "libbpf_log");
 +	free(log);
 +
 +	struct_ops_forgotten_cb__destroy(skel);
 +
 +	/* now let's programmatically use it, we should be fine now */
 +	skel = struct_ops_forgotten_cb__open();
 +	if (!ASSERT_OK_PTR(skel, "skel_open"))
 +		return;
 +
 +	skel->struct_ops.ops->test_1 = skel->progs.test_1_forgotten; /* not anymore */
 +
 +	err = struct_ops_forgotten_cb__load(skel);
 +	if (!ASSERT_OK(err, "skel_load"))
 +		goto cleanup;
 +
 +cleanup:
 +	struct_ops_forgotten_cb__destroy(skel);
 +}
 +
++=======
++>>>>>>> selftests/bpf: add another struct_ops callback use case test
  void serial_test_struct_ops_module(void)
  {
 -	if (test__start_subtest("test_struct_ops_load"))
 +	if (test__start_subtest("struct_ops_load"))
  		test_struct_ops_load();
 -	if (test__start_subtest("test_struct_ops_not_zeroed"))
 +	if (test__start_subtest("struct_ops_not_zeroed"))
  		test_struct_ops_not_zeroed();
 -	if (test__start_subtest("test_struct_ops_incompatible"))
 +	if (test__start_subtest("struct_ops_incompatible"))
  		test_struct_ops_incompatible();
++<<<<<<< HEAD
 +	if (test__start_subtest("struct_ops_null_out_cb"))
 +		test_struct_ops_nulled_out_cb();
 +	if (test__start_subtest("struct_ops_forgotten_cb"))
 +		test_struct_ops_forgotten_cb();
++=======
+ 	if (test__start_subtest("test_struct_ops_null_out_cb"))
+ 		test_struct_ops_nulled_out_cb();
++>>>>>>> selftests/bpf: add another struct_ops callback use case test
  }
  

@kernel-patches-daemon-bpf
Copy link
Author

At least one diff in series https://patchwork.kernel.org/project/netdevbpf/list/?series=850977 irrelevant now. Closing PR.

facebook-github-bot pushed a commit to facebookincubator/kernel-patches-daemon that referenced this pull request May 9, 2024
Summary:
Upstream git and patchwork can race. A series could
have already been merged+pushed while patchwork reports a relevant
status (ie. !accepted).

Handle the race by checking if a series has already been merged.

Here's a prod example of the race:
* kernel-patches/bpf#6974 (comment)
* P1314577443
* https://patchwork.kernel.org/project/netdevbpf/list/?series=850977&state=*

Reviewed By: chantra

Differential Revision: D57133263

fbshipit-source-id: 09e42a0709c427b4a8942f267070aeaa7b64aa7a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant