On 4/23/21 2:38 PM, Andrii Nakryiko wrote:
On Fri, Apr 23, 2021 at 1:24 PM Yonghong Song <yhs@xxxxxx> wrote:
On 4/23/21 11:53 AM, Andrii Nakryiko wrote:
Prepend <obj_name>.. prefix to each static variable in BTF info during static
linking. This makes them uniquely named for the sake of BPF skeleton use,
allowing to read/write static BPF variables from user-space. This uniqueness
guarantee depends on each linked file name uniqueness, of course. Double dots
separator was chosen both to be different (but similar) to the separator that
Clang is currently using for static variables defined inside functions as well
as to generate a natural (in libbpf parlance, at least) obj__var naming pattern
in BPF skeleton. Static linker also checks for static variable to already
contain ".." separator and skips the rename to allow multi-pass linking and not
keep making variable name ever increasing, if derived object name is changing on
each pass (as is the case for selftests).
This patch also adds opts to bpf_linker__add_file() API, which currently
allows to override object name for a given file and could be extended with other
per-file options in the future. This is not a breaking change because
bpf_linker__add_file() isn't yet released officially.
This patch also includes fixes to few selftests that are already using static
variables. They have to go in in the same patch to not break selftest build.
"in in" => "in"
heh, I knew this would be confusing :) it's "go in" a verb and "in the
same patch" as where they go into. But I'll re-phrase in the next
version.
Keep in mind, this static variable rename only happens during static linking.
For any existing user of BPF skeleton using static variables nothing changes,
because those use cases are using variable names generated by Clang. Only new
users utilizing static linker might need to adjust BPF skeleton use, which
currently will be always new use cases. So ther is no risk of breakage.
static_linked selftests is modified to also validate conflicting static variable
names are handled correctly both during static linking and in BPF skeleton.
Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
---
tools/bpf/bpftool/gen.c | 2 +-
tools/lib/bpf/libbpf.h | 12 +-
tools/lib/bpf/linker.c | 121 +++++++++++++++++-
.../selftests/bpf/prog_tests/skeleton.c | 8 +-
.../selftests/bpf/prog_tests/static_linked.c | 8 +-
.../selftests/bpf/progs/bpf_iter_test_kern4.c | 4 +-
.../selftests/bpf/progs/test_check_mtu.c | 4 +-
.../selftests/bpf/progs/test_cls_redirect.c | 4 +-
.../bpf/progs/test_snprintf_single.c | 2 +-
.../selftests/bpf/progs/test_sockmap_listen.c | 4 +-
.../selftests/bpf/progs/test_static_linked1.c | 6 +-
.../selftests/bpf/progs/test_static_linked2.c | 4 +-
12 files changed, 151 insertions(+), 28 deletions(-)
[...]
+static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
+ const struct bpf_linker_file_opts *opts,
+ struct src_obj *obj)
{
#if __BYTE_ORDER == __LITTLE_ENDIAN
const int host_endianness = ELFDATA2LSB;
@@ -549,6 +613,14 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
obj->filename = filename;
+ if (OPTS_GET(opts, object_name, NULL)) {
+ strncpy(obj->obj_name, opts->object_name, MAX_OBJ_NAME_LEN);
+ obj->obj_name[MAX_OBJ_NAME_LEN - 1] = '\0';
Looks we don't have examples/selftests which actually use this option.
The only place to use bpf_linker__add_file() is bpftool which did not
have option to overwrite the obj file name.
The code looks fine to me though.
Right, I was a bit lazy in adding this to bpftool, but I'm sure we'll
want to support overriding obj file name, at least to resolve object
file name conflicts. One other problem is syntax, I haven't thought
through what's the best way to do this with bpftool. Something like
this would do it:
bpftool gen object dest.o input1.o=custom_obj_name
path/to/file2.o=another_custom_obj_name
But this is too bike-shedding a topic which I want to avoid for now.
Okay. The additional option thing can be done later.
+ } else {
+ get_obj_name(obj->obj_name, filename);
+ }
+ obj->obj_name_len = strlen(obj->obj_name);
+
obj->fd = open(filename, O_RDONLY);
if (obj->fd < 0) {
err = -errno;
@@ -2264,6 +2336,47 @@ static int linker_append_btf(struct bpf_linker *linker, struct src_obj *obj)
obj->btf_type_map[i] = glob_sym->btf_id;
continue;
}
+ } else if (btf_is_var(t) && btf_var(t)->linkage == BTF_VAR_STATIC) {
+ /* Static variables are renamed to include
+ * "<obj_name>.." prefix (note double dots), similarly
+ * to how static variables inside functions are named
+ * "<func_name>.<var_name>" by compiler. This allows to
+ * have unique identifiers for static variables across
+ * all linked object files (assuming unique filenames,
+ * of course), which BPF skeleton relies on.
+ *
+ * So worst case static variable inside the function
+ * will have the form "<obj_name>..<func_name>.<var_name"
<var_name => <var_name>
good catch, will fix
+ * and will get sanitized by BPF skeleton generation
+ * logic to a field with <obj_name>__<func_name>_<var_name>
+ * name. Typical static variable will have a
+ * <obj_name>__<var_name> name, implying arguably nice
+ * per-file scoping.
+ *
+ * If static var name already contains '..', though,
+ * don't rename it, because it was already renamed by
+ * previous linker passes.
+ */
+ name = btf__str_by_offset(obj->btf, t->name_off);
+ if (!strstr(name, "..")) {
+ char new_name[MAX_VAR_NAME_LEN];
+
+ memcpy(new_name, obj->obj_name, obj->obj_name_len);
+ new_name[obj->obj_name_len] = '.';
+ new_name[obj->obj_name_len + 1] = '.';
+ new_name[obj->obj_name_len + 2] = '\0';
+ /* -3 is for '..' separator and terminating '\0' */
+ strncat(new_name, name, MAX_VAR_NAME_LEN - obj->obj_name_len - 3);
+
+ id = btf__add_str(obj->btf, new_name);
+ if (id < 0)
+ return id;
+
+ /* btf__add_str() might invalidate t, so re-fetch */
+ t = btf__type_by_id(obj->btf, i);
+
+ ((struct btf_type *)t)->name_off = id;
+ }
}
[...]
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_test_kern4.c b/tools/testing/selftests/bpf/progs/bpf_iter_test_kern4.c
index ee49493dc125..43bf8ec8ae79 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_test_kern4.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_test_kern4.c
@@ -9,8 +9,8 @@ __u32 map1_id = 0, map2_id = 0;
__u32 map1_accessed = 0, map2_accessed = 0;
__u64 map1_seqnum = 0, map2_seqnum1 = 0, map2_seqnum2 = 0;
-static volatile const __u32 print_len;
-static volatile const __u32 ret1;
+volatile const __u32 print_len = 0;
+volatile const __u32 ret1 = 0;
I am little bit puzzled why bpf_iter_test_kern4.c is impacted. I think
this is not in a static link test, right? The same for a few tests below.
All the selftests are passed through a static linker, so it will
append obj_name to each static variable. So I just minimized use of
static variables to avoid too much code churn. If this variable was
static, it would have to be accessed as
skel->rodata->bpf_iter_test_kern4__print_len, for example.
Okay this should be fine. selftests/bpf specific. I just feel that
some people may get confused if they write/see a single program in
selftest and they have to use obj_varname format and thinking this
is a new standard, but actually it is due to static linking buried
in Makefile. Maybe add a note in selftests/README.rst so we
can point to people if there is confusion.
SEC("iter/bpf_map")
int dump_bpf_map(struct bpf_iter__bpf_map *ctx)
diff --git a/tools/testing/selftests/bpf/progs/test_check_mtu.c b/tools/testing/selftests/bpf/progs/test_check_mtu.c
index c4a9bae96e75..71184af57749 100644
--- a/tools/testing/selftests/bpf/progs/test_check_mtu.c
+++ b/tools/testing/selftests/bpf/progs/test_check_mtu.c
@@ -11,8 +11,8 @@
char _license[] SEC("license") = "GPL";
[...]
