| Message ID | 20200715214312.2266839-3-haoluo@google.com |
|---|---|
| State | RFC |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | BTF support for ksyms | expand |
On Wed, Jul 15, 2020 at 2:46 PM Hao Luo <haoluo@google.com> wrote: > > Extend ksyms.c selftest to make sure BTF enables direct loads of ksyms. > > Note that test is done against the kernel btf extended with kernel VARs. > > Signed-off-by: Hao Luo <haoluo@google.com> > --- > tools/testing/selftests/bpf/prog_tests/ksyms.c | 2 ++ > tools/testing/selftests/bpf/progs/test_ksyms.c | 14 ++++++++++++++ > 2 files changed, 16 insertions(+) > > diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms.c b/tools/testing/selftests/bpf/prog_tests/ksyms.c > index e3d6777226a8..0e7f3bc3b0ae 100644 > --- a/tools/testing/selftests/bpf/prog_tests/ksyms.c > +++ b/tools/testing/selftests/bpf/prog_tests/ksyms.c > @@ -65,6 +65,8 @@ void test_ksyms(void) > "got %llu, exp %llu\n", data->out__btf_size, btf_size); > CHECK(data->out__per_cpu_start != 0, "__per_cpu_start", > "got %llu, exp %llu\n", data->out__per_cpu_start, (__u64)0); > + CHECK(data->out__rq_cpu != 0, "rq_cpu", > + "got %llu, exp %llu\n", data->out__rq_cpu, (__u64)0); > > cleanup: > test_ksyms__destroy(skel); > diff --git a/tools/testing/selftests/bpf/progs/test_ksyms.c b/tools/testing/selftests/bpf/progs/test_ksyms.c > index 6c9cbb5a3bdf..e777603757e5 100644 > --- a/tools/testing/selftests/bpf/progs/test_ksyms.c > +++ b/tools/testing/selftests/bpf/progs/test_ksyms.c > @@ -1,6 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0 > /* Copyright (c) 2019 Facebook */ > > +#include "vmlinux.h" > #include <stdbool.h> > #include <linux/bpf.h> > #include <bpf/bpf_helpers.h> > @@ -9,11 +10,13 @@ __u64 out__bpf_link_fops = -1; > __u64 out__bpf_link_fops1 = -1; > __u64 out__btf_size = -1; > __u64 out__per_cpu_start = -1; > +__u64 out__rq_cpu = -1; > > extern const void bpf_link_fops __ksym; > extern const void __start_BTF __ksym; > extern const void __stop_BTF __ksym; > extern const void __per_cpu_start __ksym; > +extern const void runqueues __ksym; This should ideally look like a real global variable extern: extern const struct rq runqueues __ksym; But that's the case for non-per-cpu variables. You didn't seem to address per-CPU variables in this patch set. How did you intend to handle that? We should look at a possible BPF helper to access such variables as well and how the verifier will prevent direct memory accesses for such variables. We should have some BPF helper that accepts per-CPU PTR_TO_BTF_ID, and returns PTR_TO_BTF_ID, but adjusted to desired CPU. And verifier ideally would allow direct memory access on that resulting PTR_TO_BTF_ID, but not on per-CPU one. Not sure yet how this should look like, but the verifier probably needs to know that variable itself is per-cpu, no? > /* non-existing symbol, weak, default to zero */ > extern const void bpf_link_fops1 __ksym __weak; > > @@ -29,4 +32,15 @@ int handler(const void *ctx) > return 0; > } > > +SEC("tp_btf/sys_enter") > +int handler_tp_btf(const void *ctx) > +{ > + const struct rq *rq = &runqueues; > + > + /* rq now points to the runqueue of cpu 0. */ > + out__rq_cpu = rq->cpu; > + > + return 0; > +} > + > char _license[] SEC("license") = "GPL"; > -- > 2.27.0.389.gc38d7665816-goog >
> > This should ideally look like a real global variable extern: > > extern const struct rq runqueues __ksym; > > > But that's the case for non-per-cpu variables. You didn't seem to > address per-CPU variables in this patch set. How did you intend to > handle that? We should look at a possible BPF helper to access such > variables as well and how the verifier will prevent direct memory > accesses for such variables. > > We should have some BPF helper that accepts per-CPU PTR_TO_BTF_ID, and > returns PTR_TO_BTF_ID, but adjusted to desired CPU. And verifier > ideally would allow direct memory access on that resulting > PTR_TO_BTF_ID, but not on per-CPU one. Not sure yet how this should > look like, but the verifier probably needs to know that variable > itself is per-cpu, no? > Yes, that's what I was unclear about, so I don't have that part in this patchset. But your explanation helped me organize my thoughts. :) Actually, the verifier can tell whether a var is percpu from the DATASEC, since we have encoded "percpu" DATASEC in btf. I think the following should work: We may introduce a new PTR_TO_BTF_VAR_ID. In ld_imm, libbpf replaces ksyms with btf_id. The btf id points to a KIND_VAR. If the pointed VAR is found in the "percpu" DATASEC, dst_reg is set to PTR_TO_BTF_VAR_ID; otherwise, it will be a PTR_TO_BTF_ID. For PTR_TO_BTF_VAR_ID, reg->btf_id is the id of the VAR. For PTR_TO_BTF_ID, reg->btf_id is the id of the actual kernel type. The verifier would reject direct memory access on PTR_TO_BTF_VAR_ID, but the new BPF helper can convert a PTR_TO_BTF_VAR_ID to PTR_TO_BTF_ID. Hao
On Mon, Jul 20, 2020 at 1:28 PM Hao Luo <haoluo@google.com> wrote: > > > > > This should ideally look like a real global variable extern: > > > > extern const struct rq runqueues __ksym; > > > > > > But that's the case for non-per-cpu variables. You didn't seem to > > address per-CPU variables in this patch set. How did you intend to > > handle that? We should look at a possible BPF helper to access such > > variables as well and how the verifier will prevent direct memory > > accesses for such variables. > > > > We should have some BPF helper that accepts per-CPU PTR_TO_BTF_ID, and > > returns PTR_TO_BTF_ID, but adjusted to desired CPU. And verifier > > ideally would allow direct memory access on that resulting > > PTR_TO_BTF_ID, but not on per-CPU one. Not sure yet how this should > > look like, but the verifier probably needs to know that variable > > itself is per-cpu, no? > > > > Yes, that's what I was unclear about, so I don't have that part in > this patchset. But your explanation helped me organize my thoughts. :) > > Actually, the verifier can tell whether a var is percpu from the > DATASEC, since we have encoded "percpu" DATASEC in btf. I think the > following should work: > > We may introduce a new PTR_TO_BTF_VAR_ID. In ld_imm, libbpf replaces > ksyms with btf_id. The btf id points to a KIND_VAR. If the pointed VAR > is found in the "percpu" DATASEC, dst_reg is set to PTR_TO_BTF_VAR_ID; > otherwise, it will be a PTR_TO_BTF_ID. For PTR_TO_BTF_VAR_ID, > reg->btf_id is the id of the VAR. For PTR_TO_BTF_ID, reg->btf_id is > the id of the actual kernel type. The verifier would reject direct > memory access on PTR_TO_BTF_VAR_ID, but the new BPF helper can convert > a PTR_TO_BTF_VAR_ID to PTR_TO_BTF_ID. Sounds good to me as a plan, except that PTR_TO_BTF_VAR_ID is a misleading name. It's always a variable. The per-CPU part is crucial, though, so maybe something like PTR_TO_PERCPU_BTF_ID? > > Hao
Ack. Will have that in v2. Hao On Mon, Jul 20, 2020 at 7:37 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Mon, Jul 20, 2020 at 1:28 PM Hao Luo <haoluo@google.com> wrote: > > > > > > > > This should ideally look like a real global variable extern: > > > > > > extern const struct rq runqueues __ksym; > > > > > > > > > But that's the case for non-per-cpu variables. You didn't seem to > > > address per-CPU variables in this patch set. How did you intend to > > > handle that? We should look at a possible BPF helper to access such > > > variables as well and how the verifier will prevent direct memory > > > accesses for such variables. > > > > > > We should have some BPF helper that accepts per-CPU PTR_TO_BTF_ID, and > > > returns PTR_TO_BTF_ID, but adjusted to desired CPU. And verifier > > > ideally would allow direct memory access on that resulting > > > PTR_TO_BTF_ID, but not on per-CPU one. Not sure yet how this should > > > look like, but the verifier probably needs to know that variable > > > itself is per-cpu, no? > > > > > > > Yes, that's what I was unclear about, so I don't have that part in > > this patchset. But your explanation helped me organize my thoughts. :) > > > > Actually, the verifier can tell whether a var is percpu from the > > DATASEC, since we have encoded "percpu" DATASEC in btf. I think the > > following should work: > > > > We may introduce a new PTR_TO_BTF_VAR_ID. In ld_imm, libbpf replaces > > ksyms with btf_id. The btf id points to a KIND_VAR. If the pointed VAR > > is found in the "percpu" DATASEC, dst_reg is set to PTR_TO_BTF_VAR_ID; > > otherwise, it will be a PTR_TO_BTF_ID. For PTR_TO_BTF_VAR_ID, > > reg->btf_id is the id of the VAR. For PTR_TO_BTF_ID, reg->btf_id is > > the id of the actual kernel type. The verifier would reject direct > > memory access on PTR_TO_BTF_VAR_ID, but the new BPF helper can convert > > a PTR_TO_BTF_VAR_ID to PTR_TO_BTF_ID. > > Sounds good to me as a plan, except that PTR_TO_BTF_VAR_ID is a > misleading name. It's always a variable. The per-CPU part is crucial, > though, so maybe something like PTR_TO_PERCPU_BTF_ID? > > > > > Hao
diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms.c b/tools/testing/selftests/bpf/prog_tests/ksyms.c index e3d6777226a8..0e7f3bc3b0ae 100644 --- a/tools/testing/selftests/bpf/prog_tests/ksyms.c +++ b/tools/testing/selftests/bpf/prog_tests/ksyms.c @@ -65,6 +65,8 @@ void test_ksyms(void) "got %llu, exp %llu\n", data->out__btf_size, btf_size); CHECK(data->out__per_cpu_start != 0, "__per_cpu_start", "got %llu, exp %llu\n", data->out__per_cpu_start, (__u64)0); + CHECK(data->out__rq_cpu != 0, "rq_cpu", + "got %llu, exp %llu\n", data->out__rq_cpu, (__u64)0); cleanup: test_ksyms__destroy(skel); diff --git a/tools/testing/selftests/bpf/progs/test_ksyms.c b/tools/testing/selftests/bpf/progs/test_ksyms.c index 6c9cbb5a3bdf..e777603757e5 100644 --- a/tools/testing/selftests/bpf/progs/test_ksyms.c +++ b/tools/testing/selftests/bpf/progs/test_ksyms.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0 /* Copyright (c) 2019 Facebook */ +#include "vmlinux.h" #include <stdbool.h> #include <linux/bpf.h> #include <bpf/bpf_helpers.h> @@ -9,11 +10,13 @@ __u64 out__bpf_link_fops = -1; __u64 out__bpf_link_fops1 = -1; __u64 out__btf_size = -1; __u64 out__per_cpu_start = -1; +__u64 out__rq_cpu = -1; extern const void bpf_link_fops __ksym; extern const void __start_BTF __ksym; extern const void __stop_BTF __ksym; extern const void __per_cpu_start __ksym; +extern const void runqueues __ksym; /* non-existing symbol, weak, default to zero */ extern const void bpf_link_fops1 __ksym __weak; @@ -29,4 +32,15 @@ int handler(const void *ctx) return 0; } +SEC("tp_btf/sys_enter") +int handler_tp_btf(const void *ctx) +{ + const struct rq *rq = &runqueues; + + /* rq now points to the runqueue of cpu 0. */ + out__rq_cpu = rq->cpu; + + return 0; +} + char _license[] SEC("license") = "GPL";
Extend ksyms.c selftest to make sure BTF enables direct loads of ksyms. Note that test is done against the kernel btf extended with kernel VARs. Signed-off-by: Hao Luo <haoluo@google.com> --- tools/testing/selftests/bpf/prog_tests/ksyms.c | 2 ++ tools/testing/selftests/bpf/progs/test_ksyms.c | 14 ++++++++++++++ 2 files changed, 16 insertions(+)