Commit 7e2c7a3f authored by Martin KaFai Lau's avatar Martin KaFai Lau

Merge branch 'libbpf: further struct_ops fixes and improvements'

Andrii Nakryiko says:

====================
Fix yet another case of mishandling SEC("struct_ops") programs that were
nulled out programmatically through BPF skeleton by the user.

While at it, add some improvements around detecting and reporting errors,
specifically a common case of declaring SEC("struct_ops") program, but
forgetting to actually make use of it by setting it as a callback
implementation in SEC(".struct_ops") variable (i.e., map) declaration.

A bunch of new selftests are added as well.
====================
Signed-off-by: default avatarMartin KaFai Lau <martin.lau@kernel.org>
parents 93d1c2da 7b9959b8
......@@ -1152,22 +1152,15 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
return -ENOTSUP;
}
prog = st_ops->progs[i];
if (prog) {
if (st_ops->progs[i]) {
/* If we had declaratively set struct_ops callback, we need to
* first validate that it's actually a struct_ops program.
* And then force its autoload to false, because it doesn't have
* force its autoload to false, because it doesn't have
* a chance of succeeding from POV of the current struct_ops map.
* If this program is still referenced somewhere else, though,
* then bpf_object_adjust_struct_ops_autoload() will update its
* autoload accordingly.
*/
if (!is_valid_st_ops_program(obj, prog)) {
pr_warn("struct_ops init_kern %s: member %s is declaratively assigned a non-struct_ops program\n",
map->name, mname);
return -EINVAL;
}
prog->autoload = false;
st_ops->progs[i]->autoload = false;
st_ops->progs[i] = NULL;
}
......@@ -1200,11 +1193,19 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
}
if (btf_is_ptr(mtype)) {
/* Update the value from the shadow type */
prog = *(void **)mdata;
/* just like for !kern_member case above, reset declaratively
* set (at compile time) program's autload to false,
* if user replaced it with another program or NULL
*/
if (st_ops->progs[i] && st_ops->progs[i] != prog)
st_ops->progs[i]->autoload = false;
/* Update the value from the shadow type */
st_ops->progs[i] = prog;
if (!prog)
continue;
if (!is_valid_st_ops_program(obj, prog)) {
pr_warn("struct_ops init_kern %s: member %s is not a struct_ops program\n",
map->name, mname);
......@@ -7371,7 +7372,11 @@ static int bpf_object_load_prog(struct bpf_object *obj, struct bpf_program *prog
__u32 log_level = prog->log_level;
int ret, err;
if (prog->type == BPF_PROG_TYPE_UNSPEC) {
/* Be more helpful by rejecting programs that can't be validated early
* with more meaningful and actionable error message.
*/
switch (prog->type) {
case BPF_PROG_TYPE_UNSPEC:
/*
* The program type must be set. Most likely we couldn't find a proper
* section definition at load time, and thus we didn't infer the type.
......@@ -7379,6 +7384,15 @@ static int bpf_object_load_prog(struct bpf_object *obj, struct bpf_program *prog
pr_warn("prog '%s': missing BPF prog type, check ELF section name '%s'\n",
prog->name, prog->sec_name);
return -EINVAL;
case BPF_PROG_TYPE_STRUCT_OPS:
if (prog->attach_btf_id == 0) {
pr_warn("prog '%s': SEC(\"struct_ops\") program isn't referenced anywhere, did you forget to use it?\n",
prog->name);
return -EINVAL;
}
break;
default:
break;
}
if (!insns || !insns_cnt)
......
......@@ -2,6 +2,7 @@
#undef _GNU_SOURCE
#include <string.h>
#include <stdio.h>
#include <errno.h>
#include "str_error.h"
/* make sure libbpf doesn't use kernel-only integer typedefs */
......@@ -15,7 +16,18 @@
char *libbpf_strerror_r(int err, char *dst, int len)
{
int ret = strerror_r(err < 0 ? -err : err, dst, len);
if (ret)
snprintf(dst, len, "ERROR: strerror_r(%d)=%d", err, ret);
/* on glibc <2.13, ret == -1 and errno is set, if strerror_r() can't
* handle the error, on glibc >=2.13 *positive* (errno-like) error
* code is returned directly
*/
if (ret == -1)
ret = errno;
if (ret) {
if (ret == EINVAL)
/* strerror_r() doesn't recognize this specific error */
snprintf(dst, len, "unknown error (%d)", err < 0 ? err : -err);
else
snprintf(dst, len, "ERROR: strerror_r(%d)=%d", err, ret);
}
return dst;
}
......@@ -4,6 +4,8 @@
#include <time.h>
#include "struct_ops_module.skel.h"
#include "struct_ops_nulled_out_cb.skel.h"
#include "struct_ops_forgotten_cb.skel.h"
static void check_map_info(struct bpf_map_info *info)
{
......@@ -174,13 +176,83 @@ static void test_struct_ops_incompatible(void)
struct_ops_module__destroy(skel);
}
/* validate that it's ok to "turn off" callback that kernel supports */
static void test_struct_ops_nulled_out_cb(void)
{
struct struct_ops_nulled_out_cb *skel;
int err;
skel = struct_ops_nulled_out_cb__open();
if (!ASSERT_OK_PTR(skel, "skel_open"))
return;
/* kernel knows about test_1, but we still null it out */
skel->struct_ops.ops->test_1 = NULL;
err = struct_ops_nulled_out_cb__load(skel);
if (!ASSERT_OK(err, "skel_load"))
goto cleanup;
ASSERT_FALSE(bpf_program__autoload(skel->progs.test_1_turn_off), "prog_autoload");
ASSERT_LT(bpf_program__fd(skel->progs.test_1_turn_off), 0, "prog_fd");
cleanup:
struct_ops_nulled_out_cb__destroy(skel);
}
/* 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);
}
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();
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();
}
// SPDX-License-Identifier: GPL-2.0
/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
#include <vmlinux.h>
#include <bpf/bpf_tracing.h>
#include "../bpf_testmod/bpf_testmod.h"
char _license[] SEC("license") = "GPL";
SEC("struct_ops/test_1")
int BPF_PROG(test_1_forgotten)
{
return 0;
}
SEC(".struct_ops.link")
struct bpf_testmod_ops ops = {
/* we forgot to reference test_1_forgotten above, oops */
};
// SPDX-License-Identifier: GPL-2.0
/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
#include <vmlinux.h>
#include <bpf/bpf_tracing.h>
#include "../bpf_testmod/bpf_testmod.h"
char _license[] SEC("license") = "GPL";
int rand;
int arr[1];
SEC("struct_ops/test_1")
int BPF_PROG(test_1_turn_off)
{
return arr[rand]; /* potentially way out of range access */
}
SEC(".struct_ops.link")
struct bpf_testmod_ops ops = {
.test_1 = (void *)test_1_turn_off,
};
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment