On Sun, Sep 13, 2020 at 9:58 PM Hao Luo haoluo@google.com wrote:
Thanks for taking a look, Andrii.
On Fri, Sep 4, 2020 at 12:49 PM Andrii Nakryiko andrii.nakryiko@gmail.com wrote:
On Thu, Sep 3, 2020 at 3:35 PM Hao Luo haoluo@google.com wrote:
Selftests for typed ksyms. Tests two types of ksyms: one is a struct, the other is a plain int. This tests two paths in the kernel. Struct ksyms will be converted into PTR_TO_BTF_ID by the verifier while int typed ksyms will be converted into PTR_TO_MEM.
Signed-off-by: Hao Luo haoluo@google.com
.../testing/selftests/bpf/prog_tests/ksyms.c | 31 +++------ .../selftests/bpf/prog_tests/ksyms_btf.c | 63 +++++++++++++++++++ .../selftests/bpf/progs/test_ksyms_btf.c | 23 +++++++ tools/testing/selftests/bpf/trace_helpers.c | 26 ++++++++ tools/testing/selftests/bpf/trace_helpers.h | 4 ++ 5 files changed, 123 insertions(+), 24 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/ksyms_btf.c create mode 100644 tools/testing/selftests/bpf/progs/test_ksyms_btf.c
[...]
+extern const struct rq runqueues __ksym; /* struct type global var. */ +extern const int bpf_prog_active __ksym; /* int type global var. */
When we add non-per-CPU kernel variables, I wonder if the fact that we have both per-CPU and global kernel variables under the same __ksym section would cause any problems and confusion? It's not clear to me if we need to have a special __percpu_ksym section or not?..
Yeah. Totally agree. I thought about this. I think a separate __percpu_ksym attribute is *probably* more clear. Not sure though. How about we introduce a "__percpu_ksym" and make it an alias to "__ksym" for now? If needed, we make an actual section for it in future.
Let's keep it in __ksym as is. Verifier will have enough insight to produce a meaningful error message, it won't be easy to misuse this feature.
+SEC("raw_tp/sys_enter") +int handler(const void *ctx) +{
out__runqueues = (__u64)&runqueues;out__bpf_prog_active = (__u64)&bpf_prog_active;return 0;+}
+char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c index 4d0e913bbb22..ade555fe8294 100644 --- a/tools/testing/selftests/bpf/trace_helpers.c +++ b/tools/testing/selftests/bpf/trace_helpers.c @@ -90,6 +90,32 @@ long ksym_get_addr(const char *name) return 0; }
+/* open kallsyms and read symbol addresses on the fly. Without caching all symbols,
- this is faster than load + find. */
+int kallsyms_find(const char *sym, unsigned long long *addr) +{
char type, name[500];unsigned long long value;int err = 0;FILE *f;f = fopen("/proc/kallsyms", "r");if (!f)return -ENOENT;while (fscanf(f, "%llx %c %499s%*[^\n]\n", &value, &type, name) > 0) {if (strcmp(name, sym) == 0) {*addr = value;goto out;}}err = -EINVAL;These error codes seem backward to me. If you fail to open /proc/kallsyms, that's an unexpected and invalid situation, so EINVAL makes a bit more sense there. But -ENOENT is clearly for cases where you didn't find what you were looking for, which is exactly this case.
I thought about it. I used -ENOENT for fopen failure because I found -ENOENT is for the case when a file/directory is not found, which is more reasonable in describing fopen error. But your proposal also makes sense and that is what I originally had. It doesn't sound like a big deal, I can switch the order them in v3.
For me, ENOENT is about the logical entity the function is working with. For fopen() that would be file, so if it's not found -- ENOENT. But here, for kallsyms_find it's a ksym. If /proc/kallsyms isn't there or can't be open -- that's unexpected (EINVAL). But if /proc/kallsyms was open but didn't contain the entity we are looking for (requested ksym) -- that's ENOENT.
+out:
fclose(f);return err;+}
void read_trace_pipe(void) { int trace_fd; diff --git a/tools/testing/selftests/bpf/trace_helpers.h b/tools/testing/selftests/bpf/trace_helpers.h index 25ef597dd03f..f62fdef9e589 100644 --- a/tools/testing/selftests/bpf/trace_helpers.h +++ b/tools/testing/selftests/bpf/trace_helpers.h @@ -12,6 +12,10 @@ struct ksym { int load_kallsyms(void); struct ksym *ksym_search(long key); long ksym_get_addr(const char *name);
+/* open kallsyms and find addresses on the fly, faster than load + search. */ +int kallsyms_find(const char *sym, unsigned long long *addr);
void read_trace_pipe(void);
#endif
2.28.0.526.ge36021eeef-goog