| Message ID | 20210316011348.4175708-1-kafai@fb.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Support calling kernel function | expand |
On Tue, Mar 16, 2021 at 12:01 AM Martin KaFai Lau <kafai@fb.com> wrote: > > This patch makes BTF verifier to accept extern func. It is used for > allowing bpf program to call a limited set of kernel functions > in a later patch. > > When writing bpf prog, the extern kernel function needs > to be declared under a ELF section (".ksyms") which is > the same as the current extern kernel variables and that should > keep its usage consistent without requiring to remember another > section name. > > For example, in a bpf_prog.c: > > extern int foo(struct sock *) __attribute__((section(".ksyms"))) > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 > '(anon)' type_id=18 > [25] FUNC 'foo' type_id=24 linkage=extern > [ ... ] > [33] DATASEC '.ksyms' size=0 vlen=1 > type_id=25 offset=0 size=0 > > LLVM will put the "func" type into the BTF datasec ".ksyms". > The current "btf_datasec_check_meta()" assumes everything under > it is a "var" and ensures it has non-zero size ("!vsi->size" test). > The non-zero size check is not true for "func". This patch postpones the > "!vsi-size" test from "btf_datasec_check_meta()" to > "btf_datasec_resolve()" which has all types collected to decide > if a vsi is a "var" or a "func" and then enforce the "vsi->size" > differently. > > If the datasec only has "func", its "t->size" could be zero. > Thus, the current "!t->size" test is no longer valid. The > invalid "t->size" will still be caught by the later > "last_vsi_end_off > t->size" check. This patch also takes this > chance to consolidate other "t->size" tests ("vsi->offset >= t->size" > "vsi->size > t->size", and "t->size < sum") into the existing > "last_vsi_end_off > t->size" test. > > The LLVM will also put those extern kernel function as an extern > linkage func in the BTF: > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 > '(anon)' type_id=18 > [25] FUNC 'foo' type_id=24 linkage=extern > > This patch allows BTF_FUNC_EXTERN in btf_func_check_meta(). > Also extern kernel function declaration does not > necessary have arg name. Another change in btf_func_check() is > to allow extern function having no arg name. > > The btf selftest is adjusted accordingly. New tests are also added. > > The required LLVM patch: https://reviews.llvm.org/D93563 > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > --- High-level question about EXTERN functions in DATASEC. Does kernel need to see them under DATASEC? What if libbpf just removed all EXTERN funcs from under DATASEC and leave them as "free-floating" EXTERN FUNCs in BTF. We need to tag EXTERNs with DATASECs mainly for libbpf to know whether it's .kconfig or .ksym or other type of externs. Does kernel need to care? > kernel/bpf/btf.c | 52 ++++--- > tools/testing/selftests/bpf/prog_tests/btf.c | 154 ++++++++++++++++++- > 2 files changed, 178 insertions(+), 28 deletions(-) > [...] > @@ -3611,9 +3594,28 @@ static int btf_datasec_resolve(struct btf_verifier_env *env, > u32 var_type_id = vsi->type, type_id, type_size = 0; > const struct btf_type *var_type = btf_type_by_id(env->btf, > var_type_id); > - if (!var_type || !btf_type_is_var(var_type)) { > + if (!var_type) { > + btf_verifier_log_vsi(env, v->t, vsi, > + "type not found"); > + return -EINVAL; > + } > + > + if (btf_type_is_func(var_type)) { > + if (vsi->size || vsi->offset) { > + btf_verifier_log_vsi(env, v->t, vsi, > + "Invalid size/offset"); > + return -EINVAL; > + } > + continue; > + } else if (btf_type_is_var(var_type)) { > + if (!vsi->size) { > + btf_verifier_log_vsi(env, v->t, vsi, > + "Invalid size"); > + return -EINVAL; > + } > + } else { > btf_verifier_log_vsi(env, v->t, vsi, > - "Not a VAR kind member"); > + "Neither a VAR nor a FUNC"); > return -EINVAL; can you please structure it as follow (I think it is bit easier to follow the logic then): if (btf_type_is_func()) { ... continue; /* no extra checks */ } if (!btf_type_is_var()) { /* bad, complain, exit */ return -EINVAL; } /* now we deal with extra checks for variables */ That way variable checks are kept all in one place. Also a question: is that ok to enable non-extern functions under DATASEC? Maybe, but that wasn't explicitly mentioned. > } > > @@ -3849,9 +3851,11 @@ static int btf_func_check(struct btf_verifier_env *env, > const struct btf_param *args; > const struct btf *btf; > u16 nr_args, i; > + bool is_extern; > > btf = env->btf; > proto_type = btf_type_by_id(btf, t->type); > + is_extern = btf_type_vlen(t) == BTF_FUNC_EXTERN; using btf_type_vlen(t) for getting func linkage is becoming more and more confusing. Would it be terrible to have btf_func_linkage(t) helper instead? > > if (!proto_type || !btf_type_is_func_proto(proto_type)) { > btf_verifier_log_type(env, t, "Invalid type_id"); > @@ -3861,7 +3865,7 @@ static int btf_func_check(struct btf_verifier_env *env, > args = (const struct btf_param *)(proto_type + 1); > nr_args = btf_type_vlen(proto_type); > for (i = 0; i < nr_args; i++) { > - if (!args[i].name_off && args[i].type) { > + if (!is_extern && !args[i].name_off && args[i].type) { > btf_verifier_log_type(env, t, "Invalid arg#%u", i + 1); > return -EINVAL; > } [...]
On Thu, Mar 18, 2021 at 03:53:38PM -0700, Andrii Nakryiko wrote: > On Tue, Mar 16, 2021 at 12:01 AM Martin KaFai Lau <kafai@fb.com> wrote: > > > > This patch makes BTF verifier to accept extern func. It is used for > > allowing bpf program to call a limited set of kernel functions > > in a later patch. > > > > When writing bpf prog, the extern kernel function needs > > to be declared under a ELF section (".ksyms") which is > > the same as the current extern kernel variables and that should > > keep its usage consistent without requiring to remember another > > section name. > > > > For example, in a bpf_prog.c: > > > > extern int foo(struct sock *) __attribute__((section(".ksyms"))) > > > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 > > '(anon)' type_id=18 > > [25] FUNC 'foo' type_id=24 linkage=extern > > [ ... ] > > [33] DATASEC '.ksyms' size=0 vlen=1 > > type_id=25 offset=0 size=0 > > > > LLVM will put the "func" type into the BTF datasec ".ksyms". > > The current "btf_datasec_check_meta()" assumes everything under > > it is a "var" and ensures it has non-zero size ("!vsi->size" test). > > The non-zero size check is not true for "func". This patch postpones the > > "!vsi-size" test from "btf_datasec_check_meta()" to > > "btf_datasec_resolve()" which has all types collected to decide > > if a vsi is a "var" or a "func" and then enforce the "vsi->size" > > differently. > > > > If the datasec only has "func", its "t->size" could be zero. > > Thus, the current "!t->size" test is no longer valid. The > > invalid "t->size" will still be caught by the later > > "last_vsi_end_off > t->size" check. This patch also takes this > > chance to consolidate other "t->size" tests ("vsi->offset >= t->size" > > "vsi->size > t->size", and "t->size < sum") into the existing > > "last_vsi_end_off > t->size" test. > > > > The LLVM will also put those extern kernel function as an extern > > linkage func in the BTF: > > > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 > > '(anon)' type_id=18 > > [25] FUNC 'foo' type_id=24 linkage=extern > > > > This patch allows BTF_FUNC_EXTERN in btf_func_check_meta(). > > Also extern kernel function declaration does not > > necessary have arg name. Another change in btf_func_check() is > > to allow extern function having no arg name. > > > > The btf selftest is adjusted accordingly. New tests are also added. > > > > The required LLVM patch: https://reviews.llvm.org/D93563 > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > --- > > High-level question about EXTERN functions in DATASEC. Does kernel > need to see them under DATASEC? What if libbpf just removed all EXTERN > funcs from under DATASEC and leave them as "free-floating" EXTERN > FUNCs in BTF. > > We need to tag EXTERNs with DATASECs mainly for libbpf to know whether > it's .kconfig or .ksym or other type of externs. Does kernel need to > care? Although the kernel does not need to know, since the a legit llvm generates it, I go with a proper support in the kernel (e.g. bpftool btf dump can better reflect what was there). > > > kernel/bpf/btf.c | 52 ++++--- > > tools/testing/selftests/bpf/prog_tests/btf.c | 154 ++++++++++++++++++- > > 2 files changed, 178 insertions(+), 28 deletions(-) > > > > [...] > > > @@ -3611,9 +3594,28 @@ static int btf_datasec_resolve(struct btf_verifier_env *env, > > u32 var_type_id = vsi->type, type_id, type_size = 0; > > const struct btf_type *var_type = btf_type_by_id(env->btf, > > var_type_id); > > - if (!var_type || !btf_type_is_var(var_type)) { > > + if (!var_type) { > > + btf_verifier_log_vsi(env, v->t, vsi, > > + "type not found"); > > + return -EINVAL; > > + } > > + > > + if (btf_type_is_func(var_type)) { > > + if (vsi->size || vsi->offset) { > > + btf_verifier_log_vsi(env, v->t, vsi, > > + "Invalid size/offset"); > > + return -EINVAL; > > + } > > + continue; > > + } else if (btf_type_is_var(var_type)) { > > + if (!vsi->size) { > > + btf_verifier_log_vsi(env, v->t, vsi, > > + "Invalid size"); > > + return -EINVAL; > > + } > > + } else { > > btf_verifier_log_vsi(env, v->t, vsi, > > - "Not a VAR kind member"); > > + "Neither a VAR nor a FUNC"); > > return -EINVAL; > > can you please structure it as follow (I think it is bit easier to > follow the logic then): > > if (btf_type_is_func()) { > ... > continue; /* no extra checks */ > } > > if (!btf_type_is_var()) { > /* bad, complain, exit */ > return -EINVAL; > } > > /* now we deal with extra checks for variables */ > > That way variable checks are kept all in one place. > > Also a question: is that ok to enable non-extern functions under > DATASEC? Maybe, but that wasn't explicitly mentioned. The patch does not check. We could reject that for now. > > > } > > > > @@ -3849,9 +3851,11 @@ static int btf_func_check(struct btf_verifier_env *env, > > const struct btf_param *args; > > const struct btf *btf; > > u16 nr_args, i; > > + bool is_extern; > > > > btf = env->btf; > > proto_type = btf_type_by_id(btf, t->type); > > + is_extern = btf_type_vlen(t) == BTF_FUNC_EXTERN; > > using btf_type_vlen(t) for getting func linkage is becoming more and > more confusing. Would it be terrible to have btf_func_linkage(t) > helper instead? I have it in my local v2. and also just return when it is extern. > > > > > if (!proto_type || !btf_type_is_func_proto(proto_type)) { > > btf_verifier_log_type(env, t, "Invalid type_id"); > > @@ -3861,7 +3865,7 @@ static int btf_func_check(struct btf_verifier_env *env, > > args = (const struct btf_param *)(proto_type + 1); > > nr_args = btf_type_vlen(proto_type); > > for (i = 0; i < nr_args; i++) { > > - if (!args[i].name_off && args[i].type) { > > + if (!is_extern && !args[i].name_off && args[i].type) { > > btf_verifier_log_type(env, t, "Invalid arg#%u", i + 1); > > return -EINVAL; > > } > > [...]
On Thu, Mar 18, 2021 at 4:39 PM Martin KaFai Lau <kafai@fb.com> wrote: > > On Thu, Mar 18, 2021 at 03:53:38PM -0700, Andrii Nakryiko wrote: > > On Tue, Mar 16, 2021 at 12:01 AM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > This patch makes BTF verifier to accept extern func. It is used for > > > allowing bpf program to call a limited set of kernel functions > > > in a later patch. > > > > > > When writing bpf prog, the extern kernel function needs > > > to be declared under a ELF section (".ksyms") which is > > > the same as the current extern kernel variables and that should > > > keep its usage consistent without requiring to remember another > > > section name. > > > > > > For example, in a bpf_prog.c: > > > > > > extern int foo(struct sock *) __attribute__((section(".ksyms"))) > > > > > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 > > > '(anon)' type_id=18 > > > [25] FUNC 'foo' type_id=24 linkage=extern > > > [ ... ] > > > [33] DATASEC '.ksyms' size=0 vlen=1 > > > type_id=25 offset=0 size=0 > > > > > > LLVM will put the "func" type into the BTF datasec ".ksyms". > > > The current "btf_datasec_check_meta()" assumes everything under > > > it is a "var" and ensures it has non-zero size ("!vsi->size" test). > > > The non-zero size check is not true for "func". This patch postpones the > > > "!vsi-size" test from "btf_datasec_check_meta()" to > > > "btf_datasec_resolve()" which has all types collected to decide > > > if a vsi is a "var" or a "func" and then enforce the "vsi->size" > > > differently. > > > > > > If the datasec only has "func", its "t->size" could be zero. > > > Thus, the current "!t->size" test is no longer valid. The > > > invalid "t->size" will still be caught by the later > > > "last_vsi_end_off > t->size" check. This patch also takes this > > > chance to consolidate other "t->size" tests ("vsi->offset >= t->size" > > > "vsi->size > t->size", and "t->size < sum") into the existing > > > "last_vsi_end_off > t->size" test. > > > > > > The LLVM will also put those extern kernel function as an extern > > > linkage func in the BTF: > > > > > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 > > > '(anon)' type_id=18 > > > [25] FUNC 'foo' type_id=24 linkage=extern > > > > > > This patch allows BTF_FUNC_EXTERN in btf_func_check_meta(). > > > Also extern kernel function declaration does not > > > necessary have arg name. Another change in btf_func_check() is > > > to allow extern function having no arg name. > > > > > > The btf selftest is adjusted accordingly. New tests are also added. > > > > > > The required LLVM patch: https://reviews.llvm.org/D93563 > > > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > > --- > > > > High-level question about EXTERN functions in DATASEC. Does kernel > > need to see them under DATASEC? What if libbpf just removed all EXTERN > > funcs from under DATASEC and leave them as "free-floating" EXTERN > > FUNCs in BTF. > > > > We need to tag EXTERNs with DATASECs mainly for libbpf to know whether > > it's .kconfig or .ksym or other type of externs. Does kernel need to > > care? > Although the kernel does not need to know, since the a legit llvm generates it, > I go with a proper support in the kernel (e.g. bpftool btf dump can better > reflect what was there). LLVM also generates extern VAR with BTF_VAR_EXTERN, yet libbpf is replacing it with fake INTs. We could do just that here as well. If anyone would want to know all the kernel functions that some BPF program is using, they could do it from the instruction dump, with proper addresses and kernel function names nicely displayed there. That's way more useful, IMO. > > > > > > kernel/bpf/btf.c | 52 ++++--- > > > tools/testing/selftests/bpf/prog_tests/btf.c | 154 ++++++++++++++++++- > > > 2 files changed, 178 insertions(+), 28 deletions(-) > > > > > > > [...] > > > > > @@ -3611,9 +3594,28 @@ static int btf_datasec_resolve(struct btf_verifier_env *env, > > > u32 var_type_id = vsi->type, type_id, type_size = 0; > > > const struct btf_type *var_type = btf_type_by_id(env->btf, > > > var_type_id); > > > - if (!var_type || !btf_type_is_var(var_type)) { > > > + if (!var_type) { > > > + btf_verifier_log_vsi(env, v->t, vsi, > > > + "type not found"); > > > + return -EINVAL; > > > + } > > > + > > > + if (btf_type_is_func(var_type)) { > > > + if (vsi->size || vsi->offset) { > > > + btf_verifier_log_vsi(env, v->t, vsi, > > > + "Invalid size/offset"); > > > + return -EINVAL; > > > + } > > > + continue; > > > + } else if (btf_type_is_var(var_type)) { > > > + if (!vsi->size) { > > > + btf_verifier_log_vsi(env, v->t, vsi, > > > + "Invalid size"); > > > + return -EINVAL; > > > + } > > > + } else { > > > btf_verifier_log_vsi(env, v->t, vsi, > > > - "Not a VAR kind member"); > > > + "Neither a VAR nor a FUNC"); > > > return -EINVAL; > > > > can you please structure it as follow (I think it is bit easier to > > follow the logic then): > > > > if (btf_type_is_func()) { > > ... > > continue; /* no extra checks */ > > } > > > > if (!btf_type_is_var()) { > > /* bad, complain, exit */ > > return -EINVAL; > > } > > > > /* now we deal with extra checks for variables */ > > > > That way variable checks are kept all in one place. > > > > Also a question: is that ok to enable non-extern functions under > > DATASEC? Maybe, but that wasn't explicitly mentioned. > The patch does not check. We could reject that for now. > > > > > > } > > > > > > @@ -3849,9 +3851,11 @@ static int btf_func_check(struct btf_verifier_env *env, > > > const struct btf_param *args; > > > const struct btf *btf; > > > u16 nr_args, i; > > > + bool is_extern; > > > > > > btf = env->btf; > > > proto_type = btf_type_by_id(btf, t->type); > > > + is_extern = btf_type_vlen(t) == BTF_FUNC_EXTERN; > > > > using btf_type_vlen(t) for getting func linkage is becoming more and > > more confusing. Would it be terrible to have btf_func_linkage(t) > > helper instead? > I have it in my local v2. and also just return when it is extern. > > > > > > > > > if (!proto_type || !btf_type_is_func_proto(proto_type)) { > > > btf_verifier_log_type(env, t, "Invalid type_id"); > > > @@ -3861,7 +3865,7 @@ static int btf_func_check(struct btf_verifier_env *env, > > > args = (const struct btf_param *)(proto_type + 1); > > > nr_args = btf_type_vlen(proto_type); > > > for (i = 0; i < nr_args; i++) { > > > - if (!args[i].name_off && args[i].type) { > > > + if (!is_extern && !args[i].name_off && args[i].type) { > > > btf_verifier_log_type(env, t, "Invalid arg#%u", i + 1); > > > return -EINVAL; > > > } > > > > [...]
On Thu, Mar 18, 2021 at 09:13:56PM -0700, Andrii Nakryiko wrote: > On Thu, Mar 18, 2021 at 4:39 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > On Thu, Mar 18, 2021 at 03:53:38PM -0700, Andrii Nakryiko wrote: > > > On Tue, Mar 16, 2021 at 12:01 AM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > This patch makes BTF verifier to accept extern func. It is used for > > > > allowing bpf program to call a limited set of kernel functions > > > > in a later patch. > > > > > > > > When writing bpf prog, the extern kernel function needs > > > > to be declared under a ELF section (".ksyms") which is > > > > the same as the current extern kernel variables and that should > > > > keep its usage consistent without requiring to remember another > > > > section name. > > > > > > > > For example, in a bpf_prog.c: > > > > > > > > extern int foo(struct sock *) __attribute__((section(".ksyms"))) > > > > > > > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 > > > > '(anon)' type_id=18 > > > > [25] FUNC 'foo' type_id=24 linkage=extern > > > > [ ... ] > > > > [33] DATASEC '.ksyms' size=0 vlen=1 > > > > type_id=25 offset=0 size=0 > > > > > > > > LLVM will put the "func" type into the BTF datasec ".ksyms". > > > > The current "btf_datasec_check_meta()" assumes everything under > > > > it is a "var" and ensures it has non-zero size ("!vsi->size" test). > > > > The non-zero size check is not true for "func". This patch postpones the > > > > "!vsi-size" test from "btf_datasec_check_meta()" to > > > > "btf_datasec_resolve()" which has all types collected to decide > > > > if a vsi is a "var" or a "func" and then enforce the "vsi->size" > > > > differently. > > > > > > > > If the datasec only has "func", its "t->size" could be zero. > > > > Thus, the current "!t->size" test is no longer valid. The > > > > invalid "t->size" will still be caught by the later > > > > "last_vsi_end_off > t->size" check. This patch also takes this > > > > chance to consolidate other "t->size" tests ("vsi->offset >= t->size" > > > > "vsi->size > t->size", and "t->size < sum") into the existing > > > > "last_vsi_end_off > t->size" test. > > > > > > > > The LLVM will also put those extern kernel function as an extern > > > > linkage func in the BTF: > > > > > > > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 > > > > '(anon)' type_id=18 > > > > [25] FUNC 'foo' type_id=24 linkage=extern > > > > > > > > This patch allows BTF_FUNC_EXTERN in btf_func_check_meta(). > > > > Also extern kernel function declaration does not > > > > necessary have arg name. Another change in btf_func_check() is > > > > to allow extern function having no arg name. > > > > > > > > The btf selftest is adjusted accordingly. New tests are also added. > > > > > > > > The required LLVM patch: https://reviews.llvm.org/D93563 > > > > > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > > > --- > > > > > > High-level question about EXTERN functions in DATASEC. Does kernel > > > need to see them under DATASEC? What if libbpf just removed all EXTERN > > > funcs from under DATASEC and leave them as "free-floating" EXTERN > > > FUNCs in BTF. > > > > > > We need to tag EXTERNs with DATASECs mainly for libbpf to know whether > > > it's .kconfig or .ksym or other type of externs. Does kernel need to > > > care? > > Although the kernel does not need to know, since the a legit llvm generates it, > > I go with a proper support in the kernel (e.g. bpftool btf dump can better > > reflect what was there). > > LLVM also generates extern VAR with BTF_VAR_EXTERN, yet libbpf is > replacing it with fake INTs. Yep. I noticed the loop in collect_extern() in libbpf. It replaces the var->type with INT. > We could do just that here as well. What to replace in the FUNC case? Regardless, supporting it properly in the kernel is a better way to go instead of asking the userspace to move around it. It is not very complicated to support it in the kernel also. What is the concern of having the kernel to support it? > If anyone would want to know all the kernel functions that some BPF > program is using, they could do it from the instruction dump, with > proper addresses and kernel function names nicely displayed there. > That's way more useful, IMO.
On Thu, Mar 18, 2021 at 10:29 PM Martin KaFai Lau <kafai@fb.com> wrote: > > On Thu, Mar 18, 2021 at 09:13:56PM -0700, Andrii Nakryiko wrote: > > On Thu, Mar 18, 2021 at 4:39 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > On Thu, Mar 18, 2021 at 03:53:38PM -0700, Andrii Nakryiko wrote: > > > > On Tue, Mar 16, 2021 at 12:01 AM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > > > This patch makes BTF verifier to accept extern func. It is used for > > > > > allowing bpf program to call a limited set of kernel functions > > > > > in a later patch. > > > > > > > > > > When writing bpf prog, the extern kernel function needs > > > > > to be declared under a ELF section (".ksyms") which is > > > > > the same as the current extern kernel variables and that should > > > > > keep its usage consistent without requiring to remember another > > > > > section name. > > > > > > > > > > For example, in a bpf_prog.c: > > > > > > > > > > extern int foo(struct sock *) __attribute__((section(".ksyms"))) > > > > > > > > > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 > > > > > '(anon)' type_id=18 > > > > > [25] FUNC 'foo' type_id=24 linkage=extern > > > > > [ ... ] > > > > > [33] DATASEC '.ksyms' size=0 vlen=1 > > > > > type_id=25 offset=0 size=0 > > > > > > > > > > LLVM will put the "func" type into the BTF datasec ".ksyms". > > > > > The current "btf_datasec_check_meta()" assumes everything under > > > > > it is a "var" and ensures it has non-zero size ("!vsi->size" test). > > > > > The non-zero size check is not true for "func". This patch postpones the > > > > > "!vsi-size" test from "btf_datasec_check_meta()" to > > > > > "btf_datasec_resolve()" which has all types collected to decide > > > > > if a vsi is a "var" or a "func" and then enforce the "vsi->size" > > > > > differently. > > > > > > > > > > If the datasec only has "func", its "t->size" could be zero. > > > > > Thus, the current "!t->size" test is no longer valid. The > > > > > invalid "t->size" will still be caught by the later > > > > > "last_vsi_end_off > t->size" check. This patch also takes this > > > > > chance to consolidate other "t->size" tests ("vsi->offset >= t->size" > > > > > "vsi->size > t->size", and "t->size < sum") into the existing > > > > > "last_vsi_end_off > t->size" test. > > > > > > > > > > The LLVM will also put those extern kernel function as an extern > > > > > linkage func in the BTF: > > > > > > > > > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 > > > > > '(anon)' type_id=18 > > > > > [25] FUNC 'foo' type_id=24 linkage=extern > > > > > > > > > > This patch allows BTF_FUNC_EXTERN in btf_func_check_meta(). > > > > > Also extern kernel function declaration does not > > > > > necessary have arg name. Another change in btf_func_check() is > > > > > to allow extern function having no arg name. > > > > > > > > > > The btf selftest is adjusted accordingly. New tests are also added. > > > > > > > > > > The required LLVM patch: https://reviews.llvm.org/D93563 > > > > > > > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > > > > --- > > > > > > > > High-level question about EXTERN functions in DATASEC. Does kernel > > > > need to see them under DATASEC? What if libbpf just removed all EXTERN > > > > funcs from under DATASEC and leave them as "free-floating" EXTERN > > > > FUNCs in BTF. > > > > > > > > We need to tag EXTERNs with DATASECs mainly for libbpf to know whether > > > > it's .kconfig or .ksym or other type of externs. Does kernel need to > > > > care? > > > Although the kernel does not need to know, since the a legit llvm generates it, > > > I go with a proper support in the kernel (e.g. bpftool btf dump can better > > > reflect what was there). > > > > LLVM also generates extern VAR with BTF_VAR_EXTERN, yet libbpf is > > replacing it with fake INTs. > Yep. I noticed the loop in collect_extern() in libbpf. > It replaces the var->type with INT. > > > We could do just that here as well. > What to replace in the FUNC case? if we do that, I'd just replace them with same INTs. Or we can just remove the entire DATASEC. Now it is easier to do with BTF write APIs. Back then it was a major pain. I'd probably get rid of DATASEC altogether instead of that INT replacement, if I had BTF write APIs. > > Regardless, supporting it properly in the kernel is a better way to go > instead of asking the userspace to move around it. It is not very > complicated to support it in the kernel also. > > What is the concern of having the kernel to support it? Just more complicated BTF validation logic, which means that there are higher chances of permitting invalid BTF. And then the question is what can the kernel do with those EXTERNs in BTF? Probably nothing. And that .ksyms section is special, and purely libbpf convention. Ideally kernel should not allow EXTERN funcs in any other DATASEC. Are you willing to hard-code ".ksyms" name in kernel for libbpf's sake? Probably not. The general rule, so far, was that kernel shouldn't see any unresolved EXTERN at all. Now it's neither here nor there. EXTERN funcs are ok, EXTERN vars are not. > > > If anyone would want to know all the kernel functions that some BPF > > program is using, they could do it from the instruction dump, with > > proper addresses and kernel function names nicely displayed there. > > That's way more useful, IMO.
On Fri, Mar 19, 2021 at 02:27:13PM -0700, Andrii Nakryiko wrote: > On Thu, Mar 18, 2021 at 10:29 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > On Thu, Mar 18, 2021 at 09:13:56PM -0700, Andrii Nakryiko wrote: > > > On Thu, Mar 18, 2021 at 4:39 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > On Thu, Mar 18, 2021 at 03:53:38PM -0700, Andrii Nakryiko wrote: > > > > > On Tue, Mar 16, 2021 at 12:01 AM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > > > > > This patch makes BTF verifier to accept extern func. It is used for > > > > > > allowing bpf program to call a limited set of kernel functions > > > > > > in a later patch. > > > > > > > > > > > > When writing bpf prog, the extern kernel function needs > > > > > > to be declared under a ELF section (".ksyms") which is > > > > > > the same as the current extern kernel variables and that should > > > > > > keep its usage consistent without requiring to remember another > > > > > > section name. > > > > > > > > > > > > For example, in a bpf_prog.c: > > > > > > > > > > > > extern int foo(struct sock *) __attribute__((section(".ksyms"))) > > > > > > > > > > > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 > > > > > > '(anon)' type_id=18 > > > > > > [25] FUNC 'foo' type_id=24 linkage=extern > > > > > > [ ... ] > > > > > > [33] DATASEC '.ksyms' size=0 vlen=1 > > > > > > type_id=25 offset=0 size=0 > > > > > > > > > > > > LLVM will put the "func" type into the BTF datasec ".ksyms". > > > > > > The current "btf_datasec_check_meta()" assumes everything under > > > > > > it is a "var" and ensures it has non-zero size ("!vsi->size" test). > > > > > > The non-zero size check is not true for "func". This patch postpones the > > > > > > "!vsi-size" test from "btf_datasec_check_meta()" to > > > > > > "btf_datasec_resolve()" which has all types collected to decide > > > > > > if a vsi is a "var" or a "func" and then enforce the "vsi->size" > > > > > > differently. > > > > > > > > > > > > If the datasec only has "func", its "t->size" could be zero. > > > > > > Thus, the current "!t->size" test is no longer valid. The > > > > > > invalid "t->size" will still be caught by the later > > > > > > "last_vsi_end_off > t->size" check. This patch also takes this > > > > > > chance to consolidate other "t->size" tests ("vsi->offset >= t->size" > > > > > > "vsi->size > t->size", and "t->size < sum") into the existing > > > > > > "last_vsi_end_off > t->size" test. > > > > > > > > > > > > The LLVM will also put those extern kernel function as an extern > > > > > > linkage func in the BTF: > > > > > > > > > > > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 > > > > > > '(anon)' type_id=18 > > > > > > [25] FUNC 'foo' type_id=24 linkage=extern > > > > > > > > > > > > This patch allows BTF_FUNC_EXTERN in btf_func_check_meta(). > > > > > > Also extern kernel function declaration does not > > > > > > necessary have arg name. Another change in btf_func_check() is > > > > > > to allow extern function having no arg name. > > > > > > > > > > > > The btf selftest is adjusted accordingly. New tests are also added. > > > > > > > > > > > > The required LLVM patch: https://reviews.llvm.org/D93563 > > > > > > > > > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > > > > > --- > > > > > > > > > > High-level question about EXTERN functions in DATASEC. Does kernel > > > > > need to see them under DATASEC? What if libbpf just removed all EXTERN > > > > > funcs from under DATASEC and leave them as "free-floating" EXTERN > > > > > FUNCs in BTF. > > > > > > > > > > We need to tag EXTERNs with DATASECs mainly for libbpf to know whether > > > > > it's .kconfig or .ksym or other type of externs. Does kernel need to > > > > > care? > > > > Although the kernel does not need to know, since the a legit llvm generates it, > > > > I go with a proper support in the kernel (e.g. bpftool btf dump can better > > > > reflect what was there). > > > > > > LLVM also generates extern VAR with BTF_VAR_EXTERN, yet libbpf is > > > replacing it with fake INTs. > > Yep. I noticed the loop in collect_extern() in libbpf. > > It replaces the var->type with INT. > > > > > We could do just that here as well. > > What to replace in the FUNC case? > > if we do that, I'd just replace them with same INTs. Or we can just > remove the entire DATASEC. Now it is easier to do with BTF write APIs. > Back then it was a major pain. I'd probably get rid of DATASEC > altogether instead of that INT replacement, if I had BTF write APIs. Do you mean vsi->type = INT? > > > > > Regardless, supporting it properly in the kernel is a better way to go > > instead of asking the userspace to move around it. It is not very > > complicated to support it in the kernel also. > > > > What is the concern of having the kernel to support it? > > Just more complicated BTF validation logic, which means that there are > higher chances of permitting invalid BTF. And then the question is > what can the kernel do with those EXTERNs in BTF? Probably nothing. > And that .ksyms section is special, and purely libbpf convention. > Ideally kernel should not allow EXTERN funcs in any other DATASEC. Are > you willing to hard-code ".ksyms" name in kernel for libbpf's sake? > Probably not. The general rule, so far, was that kernel shouldn't see > any unresolved EXTERN at all. Now it's neither here nor there. EXTERN > funcs are ok, EXTERN vars are not. Exactly, it is libbpf convention. The kernel does not need to enforce it. The kernel only needs to be able to support the debug info generated by llvm and being able to display/dump it later. There are many other things in the BTF that the kernel does not need to know. It is there for debug purpose which the BTF is used for. Yes, the func call can be discovered by instruction dump. It is also nice to see everything in one ksyms datasec also during btf dump. If there is a need to strip everything that the kernel does not need from the BTF, it can all be stripped in another "--strip-debug" like option. To support EXTERN var, the kernel part should be fine. I am only not sure why it has to change the vs->size and vs->offset in libbpf? > > > > > > If anyone would want to know all the kernel functions that some BPF > > > program is using, they could do it from the instruction dump, with > > > proper addresses and kernel function names nicely displayed there. > > > That's way more useful, IMO.
On Fri, Mar 19, 2021 at 3:19 PM Martin KaFai Lau <kafai@fb.com> wrote: > > On Fri, Mar 19, 2021 at 02:27:13PM -0700, Andrii Nakryiko wrote: > > On Thu, Mar 18, 2021 at 10:29 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > On Thu, Mar 18, 2021 at 09:13:56PM -0700, Andrii Nakryiko wrote: > > > > On Thu, Mar 18, 2021 at 4:39 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > > > On Thu, Mar 18, 2021 at 03:53:38PM -0700, Andrii Nakryiko wrote: > > > > > > On Tue, Mar 16, 2021 at 12:01 AM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > > > > > > > This patch makes BTF verifier to accept extern func. It is used for > > > > > > > allowing bpf program to call a limited set of kernel functions > > > > > > > in a later patch. > > > > > > > > > > > > > > When writing bpf prog, the extern kernel function needs > > > > > > > to be declared under a ELF section (".ksyms") which is > > > > > > > the same as the current extern kernel variables and that should > > > > > > > keep its usage consistent without requiring to remember another > > > > > > > section name. > > > > > > > > > > > > > > For example, in a bpf_prog.c: > > > > > > > > > > > > > > extern int foo(struct sock *) __attribute__((section(".ksyms"))) > > > > > > > > > > > > > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 > > > > > > > '(anon)' type_id=18 > > > > > > > [25] FUNC 'foo' type_id=24 linkage=extern > > > > > > > [ ... ] > > > > > > > [33] DATASEC '.ksyms' size=0 vlen=1 > > > > > > > type_id=25 offset=0 size=0 > > > > > > > > > > > > > > LLVM will put the "func" type into the BTF datasec ".ksyms". > > > > > > > The current "btf_datasec_check_meta()" assumes everything under > > > > > > > it is a "var" and ensures it has non-zero size ("!vsi->size" test). > > > > > > > The non-zero size check is not true for "func". This patch postpones the > > > > > > > "!vsi-size" test from "btf_datasec_check_meta()" to > > > > > > > "btf_datasec_resolve()" which has all types collected to decide > > > > > > > if a vsi is a "var" or a "func" and then enforce the "vsi->size" > > > > > > > differently. > > > > > > > > > > > > > > If the datasec only has "func", its "t->size" could be zero. > > > > > > > Thus, the current "!t->size" test is no longer valid. The > > > > > > > invalid "t->size" will still be caught by the later > > > > > > > "last_vsi_end_off > t->size" check. This patch also takes this > > > > > > > chance to consolidate other "t->size" tests ("vsi->offset >= t->size" > > > > > > > "vsi->size > t->size", and "t->size < sum") into the existing > > > > > > > "last_vsi_end_off > t->size" test. > > > > > > > > > > > > > > The LLVM will also put those extern kernel function as an extern > > > > > > > linkage func in the BTF: > > > > > > > > > > > > > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 > > > > > > > '(anon)' type_id=18 > > > > > > > [25] FUNC 'foo' type_id=24 linkage=extern > > > > > > > > > > > > > > This patch allows BTF_FUNC_EXTERN in btf_func_check_meta(). > > > > > > > Also extern kernel function declaration does not > > > > > > > necessary have arg name. Another change in btf_func_check() is > > > > > > > to allow extern function having no arg name. > > > > > > > > > > > > > > The btf selftest is adjusted accordingly. New tests are also added. > > > > > > > > > > > > > > The required LLVM patch: https://reviews.llvm.org/D93563 > > > > > > > > > > > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > > > > > > --- > > > > > > > > > > > > High-level question about EXTERN functions in DATASEC. Does kernel > > > > > > need to see them under DATASEC? What if libbpf just removed all EXTERN > > > > > > funcs from under DATASEC and leave them as "free-floating" EXTERN > > > > > > FUNCs in BTF. > > > > > > > > > > > > We need to tag EXTERNs with DATASECs mainly for libbpf to know whether > > > > > > it's .kconfig or .ksym or other type of externs. Does kernel need to > > > > > > care? > > > > > Although the kernel does not need to know, since the a legit llvm generates it, > > > > > I go with a proper support in the kernel (e.g. bpftool btf dump can better > > > > > reflect what was there). > > > > > > > > LLVM also generates extern VAR with BTF_VAR_EXTERN, yet libbpf is > > > > replacing it with fake INTs. > > > Yep. I noticed the loop in collect_extern() in libbpf. > > > It replaces the var->type with INT. > > > > > > > We could do just that here as well. > > > What to replace in the FUNC case? > > > > if we do that, I'd just replace them with same INTs. Or we can just > > remove the entire DATASEC. Now it is easier to do with BTF write APIs. > > Back then it was a major pain. I'd probably get rid of DATASEC > > altogether instead of that INT replacement, if I had BTF write APIs. > Do you mean vsi->type = INT? yes, that's what existing logic does for EXTERN var > > > > > > > > > Regardless, supporting it properly in the kernel is a better way to go > > > instead of asking the userspace to move around it. It is not very > > > complicated to support it in the kernel also. > > > > > > What is the concern of having the kernel to support it? > > > > Just more complicated BTF validation logic, which means that there are > > higher chances of permitting invalid BTF. And then the question is > > what can the kernel do with those EXTERNs in BTF? Probably nothing. > > And that .ksyms section is special, and purely libbpf convention. > > Ideally kernel should not allow EXTERN funcs in any other DATASEC. Are > > you willing to hard-code ".ksyms" name in kernel for libbpf's sake? > > Probably not. The general rule, so far, was that kernel shouldn't see > > any unresolved EXTERN at all. Now it's neither here nor there. EXTERN > > funcs are ok, EXTERN vars are not. > Exactly, it is libbpf convention. The kernel does not need to enforce it. > The kernel only needs to be able to support the debug info generated by > llvm and being able to display/dump it later. > > There are many other things in the BTF that the kernel does not need to Curious, what are those many other things? > know. It is there for debug purpose which the BTF is used for. Yes, > the func call can be discovered by instruction dump. It is also nice to > see everything in one ksyms datasec also during btf dump. > > If there is a need to strip everything that the kernel does not need > from the BTF, it can all be stripped in another "--strip-debug" like > option. Where does this "--strip-debug" option go? Clang, pahole, or bpftool? Or am I misunderstanding what you are proposing? > > To support EXTERN var, the kernel part should be fine. I am only not > sure why it has to change the vs->size and vs->offset in libbpf? vs->size and vs->offset are adjusted to match int type. Otherwise kernel BTF validation will complain about DATASEC size mismatch. > > > > > > > > > > > If anyone would want to know all the kernel functions that some BPF > > > > program is using, they could do it from the instruction dump, with > > > > proper addresses and kernel function names nicely displayed there. > > > > That's way more useful, IMO.
On Fri, Mar 19, 2021 at 03:29:57PM -0700, Andrii Nakryiko wrote: > On Fri, Mar 19, 2021 at 3:19 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > On Fri, Mar 19, 2021 at 02:27:13PM -0700, Andrii Nakryiko wrote: > > > On Thu, Mar 18, 2021 at 10:29 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > On Thu, Mar 18, 2021 at 09:13:56PM -0700, Andrii Nakryiko wrote: > > > > > On Thu, Mar 18, 2021 at 4:39 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > > > > > On Thu, Mar 18, 2021 at 03:53:38PM -0700, Andrii Nakryiko wrote: > > > > > > > On Tue, Mar 16, 2021 at 12:01 AM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > > > > > > > > > This patch makes BTF verifier to accept extern func. It is used for > > > > > > > > allowing bpf program to call a limited set of kernel functions > > > > > > > > in a later patch. > > > > > > > > > > > > > > > > When writing bpf prog, the extern kernel function needs > > > > > > > > to be declared under a ELF section (".ksyms") which is > > > > > > > > the same as the current extern kernel variables and that should > > > > > > > > keep its usage consistent without requiring to remember another > > > > > > > > section name. > > > > > > > > > > > > > > > > For example, in a bpf_prog.c: > > > > > > > > > > > > > > > > extern int foo(struct sock *) __attribute__((section(".ksyms"))) > > > > > > > > > > > > > > > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 > > > > > > > > '(anon)' type_id=18 > > > > > > > > [25] FUNC 'foo' type_id=24 linkage=extern > > > > > > > > [ ... ] > > > > > > > > [33] DATASEC '.ksyms' size=0 vlen=1 > > > > > > > > type_id=25 offset=0 size=0 > > > > > > > > > > > > > > > > LLVM will put the "func" type into the BTF datasec ".ksyms". > > > > > > > > The current "btf_datasec_check_meta()" assumes everything under > > > > > > > > it is a "var" and ensures it has non-zero size ("!vsi->size" test). > > > > > > > > The non-zero size check is not true for "func". This patch postpones the > > > > > > > > "!vsi-size" test from "btf_datasec_check_meta()" to > > > > > > > > "btf_datasec_resolve()" which has all types collected to decide > > > > > > > > if a vsi is a "var" or a "func" and then enforce the "vsi->size" > > > > > > > > differently. > > > > > > > > > > > > > > > > If the datasec only has "func", its "t->size" could be zero. > > > > > > > > Thus, the current "!t->size" test is no longer valid. The > > > > > > > > invalid "t->size" will still be caught by the later > > > > > > > > "last_vsi_end_off > t->size" check. This patch also takes this > > > > > > > > chance to consolidate other "t->size" tests ("vsi->offset >= t->size" > > > > > > > > "vsi->size > t->size", and "t->size < sum") into the existing > > > > > > > > "last_vsi_end_off > t->size" test. > > > > > > > > > > > > > > > > The LLVM will also put those extern kernel function as an extern > > > > > > > > linkage func in the BTF: > > > > > > > > > > > > > > > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 > > > > > > > > '(anon)' type_id=18 > > > > > > > > [25] FUNC 'foo' type_id=24 linkage=extern > > > > > > > > > > > > > > > > This patch allows BTF_FUNC_EXTERN in btf_func_check_meta(). > > > > > > > > Also extern kernel function declaration does not > > > > > > > > necessary have arg name. Another change in btf_func_check() is > > > > > > > > to allow extern function having no arg name. > > > > > > > > > > > > > > > > The btf selftest is adjusted accordingly. New tests are also added. > > > > > > > > > > > > > > > > The required LLVM patch: https://reviews.llvm.org/D93563 > > > > > > > > > > > > > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > > > > > > > --- > > > > > > > > > > > > > > High-level question about EXTERN functions in DATASEC. Does kernel > > > > > > > need to see them under DATASEC? What if libbpf just removed all EXTERN > > > > > > > funcs from under DATASEC and leave them as "free-floating" EXTERN > > > > > > > FUNCs in BTF. > > > > > > > > > > > > > > We need to tag EXTERNs with DATASECs mainly for libbpf to know whether > > > > > > > it's .kconfig or .ksym or other type of externs. Does kernel need to > > > > > > > care? > > > > > > Although the kernel does not need to know, since the a legit llvm generates it, > > > > > > I go with a proper support in the kernel (e.g. bpftool btf dump can better > > > > > > reflect what was there). > > > > > > > > > > LLVM also generates extern VAR with BTF_VAR_EXTERN, yet libbpf is > > > > > replacing it with fake INTs. > > > > Yep. I noticed the loop in collect_extern() in libbpf. > > > > It replaces the var->type with INT. > > > > > > > > > We could do just that here as well. > > > > What to replace in the FUNC case? > > > > > > if we do that, I'd just replace them with same INTs. Or we can just > > > remove the entire DATASEC. Now it is easier to do with BTF write APIs. > > > Back then it was a major pain. I'd probably get rid of DATASEC > > > altogether instead of that INT replacement, if I had BTF write APIs. > > Do you mean vsi->type = INT? > > yes, that's what existing logic does for EXTERN var There may be no var. > > > > > > > > > > > > > > Regardless, supporting it properly in the kernel is a better way to go > > > > instead of asking the userspace to move around it. It is not very > > > > complicated to support it in the kernel also. > > > > > > > > What is the concern of having the kernel to support it? > > > > > > Just more complicated BTF validation logic, which means that there are > > > higher chances of permitting invalid BTF. And then the question is > > > what can the kernel do with those EXTERNs in BTF? Probably nothing. > > > And that .ksyms section is special, and purely libbpf convention. > > > Ideally kernel should not allow EXTERN funcs in any other DATASEC. Are > > > you willing to hard-code ".ksyms" name in kernel for libbpf's sake? > > > Probably not. The general rule, so far, was that kernel shouldn't see > > > any unresolved EXTERN at all. Now it's neither here nor there. EXTERN > > > funcs are ok, EXTERN vars are not. > > Exactly, it is libbpf convention. The kernel does not need to enforce it. > > The kernel only needs to be able to support the debug info generated by > > llvm and being able to display/dump it later. > > > > There are many other things in the BTF that the kernel does not need to > > Curious, what are those many other things? VAR '_license'. deeper things could be STRUCT 'tcp_congestion_ops' and the types under it. > > > know. It is there for debug purpose which the BTF is used for. Yes, > > the func call can be discovered by instruction dump. It is also nice to > > see everything in one ksyms datasec also during btf dump. > > > > If there is a need to strip everything that the kernel does not need > > from the BTF, it can all be stripped in another "--strip-debug" like > > option. > > Where does this "--strip-debug" option go? Clang, pahole, or bpftool? > Or am I misunderstanding what you are proposing? Could be a libbpf option during load? or it can be done during gen skel? > > > > > To support EXTERN var, the kernel part should be fine. I am only not > > sure why it has to change the vs->size and vs->offset in libbpf? > > vs->size and vs->offset are adjusted to match int type. Otherwise > kernel BTF validation will complain about DATASEC size mismatch. make sense. so if there is no need to replace it with INT, they can be left as is? > > > > > > > > > > > > > > > > > If anyone would want to know all the kernel functions that some BPF > > > > > program is using, they could do it from the instruction dump, with > > > > > proper addresses and kernel function names nicely displayed there. > > > > > That's way more useful, IMO.
On Fri, Mar 19, 2021 at 3:45 PM Martin KaFai Lau <kafai@fb.com> wrote: > > On Fri, Mar 19, 2021 at 03:29:57PM -0700, Andrii Nakryiko wrote: > > On Fri, Mar 19, 2021 at 3:19 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > On Fri, Mar 19, 2021 at 02:27:13PM -0700, Andrii Nakryiko wrote: > > > > On Thu, Mar 18, 2021 at 10:29 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > > > On Thu, Mar 18, 2021 at 09:13:56PM -0700, Andrii Nakryiko wrote: > > > > > > On Thu, Mar 18, 2021 at 4:39 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > > > > > > > On Thu, Mar 18, 2021 at 03:53:38PM -0700, Andrii Nakryiko wrote: > > > > > > > > On Tue, Mar 16, 2021 at 12:01 AM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > > > > > > > > > > > This patch makes BTF verifier to accept extern func. It is used for > > > > > > > > > allowing bpf program to call a limited set of kernel functions > > > > > > > > > in a later patch. > > > > > > > > > > > > > > > > > > When writing bpf prog, the extern kernel function needs > > > > > > > > > to be declared under a ELF section (".ksyms") which is > > > > > > > > > the same as the current extern kernel variables and that should > > > > > > > > > keep its usage consistent without requiring to remember another > > > > > > > > > section name. > > > > > > > > > > > > > > > > > > For example, in a bpf_prog.c: > > > > > > > > > > > > > > > > > > extern int foo(struct sock *) __attribute__((section(".ksyms"))) > > > > > > > > > > > > > > > > > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 > > > > > > > > > '(anon)' type_id=18 > > > > > > > > > [25] FUNC 'foo' type_id=24 linkage=extern > > > > > > > > > [ ... ] > > > > > > > > > [33] DATASEC '.ksyms' size=0 vlen=1 > > > > > > > > > type_id=25 offset=0 size=0 > > > > > > > > > > > > > > > > > > LLVM will put the "func" type into the BTF datasec ".ksyms". > > > > > > > > > The current "btf_datasec_check_meta()" assumes everything under > > > > > > > > > it is a "var" and ensures it has non-zero size ("!vsi->size" test). > > > > > > > > > The non-zero size check is not true for "func". This patch postpones the > > > > > > > > > "!vsi-size" test from "btf_datasec_check_meta()" to > > > > > > > > > "btf_datasec_resolve()" which has all types collected to decide > > > > > > > > > if a vsi is a "var" or a "func" and then enforce the "vsi->size" > > > > > > > > > differently. > > > > > > > > > > > > > > > > > > If the datasec only has "func", its "t->size" could be zero. > > > > > > > > > Thus, the current "!t->size" test is no longer valid. The > > > > > > > > > invalid "t->size" will still be caught by the later > > > > > > > > > "last_vsi_end_off > t->size" check. This patch also takes this > > > > > > > > > chance to consolidate other "t->size" tests ("vsi->offset >= t->size" > > > > > > > > > "vsi->size > t->size", and "t->size < sum") into the existing > > > > > > > > > "last_vsi_end_off > t->size" test. > > > > > > > > > > > > > > > > > > The LLVM will also put those extern kernel function as an extern > > > > > > > > > linkage func in the BTF: > > > > > > > > > > > > > > > > > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 > > > > > > > > > '(anon)' type_id=18 > > > > > > > > > [25] FUNC 'foo' type_id=24 linkage=extern > > > > > > > > > > > > > > > > > > This patch allows BTF_FUNC_EXTERN in btf_func_check_meta(). > > > > > > > > > Also extern kernel function declaration does not > > > > > > > > > necessary have arg name. Another change in btf_func_check() is > > > > > > > > > to allow extern function having no arg name. > > > > > > > > > > > > > > > > > > The btf selftest is adjusted accordingly. New tests are also added. > > > > > > > > > > > > > > > > > > The required LLVM patch: https://reviews.llvm.org/D93563 > > > > > > > > > > > > > > > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > > > > > > > > --- > > > > > > > > > > > > > > > > High-level question about EXTERN functions in DATASEC. Does kernel > > > > > > > > need to see them under DATASEC? What if libbpf just removed all EXTERN > > > > > > > > funcs from under DATASEC and leave them as "free-floating" EXTERN > > > > > > > > FUNCs in BTF. > > > > > > > > > > > > > > > > We need to tag EXTERNs with DATASECs mainly for libbpf to know whether > > > > > > > > it's .kconfig or .ksym or other type of externs. Does kernel need to > > > > > > > > care? > > > > > > > Although the kernel does not need to know, since the a legit llvm generates it, > > > > > > > I go with a proper support in the kernel (e.g. bpftool btf dump can better > > > > > > > reflect what was there). > > > > > > > > > > > > LLVM also generates extern VAR with BTF_VAR_EXTERN, yet libbpf is > > > > > > replacing it with fake INTs. > > > > > Yep. I noticed the loop in collect_extern() in libbpf. > > > > > It replaces the var->type with INT. > > > > > > > > > > > We could do just that here as well. > > > > > What to replace in the FUNC case? > > > > > > > > if we do that, I'd just replace them with same INTs. Or we can just > > > > remove the entire DATASEC. Now it is easier to do with BTF write APIs. > > > > Back then it was a major pain. I'd probably get rid of DATASEC > > > > altogether instead of that INT replacement, if I had BTF write APIs. > > > Do you mean vsi->type = INT? > > > > yes, that's what existing logic does for EXTERN var > There may be no var. > sure, but we have btf__add_var(), if we really want VAR ;) > > > > > > > > > > > > > > > > > > > Regardless, supporting it properly in the kernel is a better way to go > > > > > instead of asking the userspace to move around it. It is not very > > > > > complicated to support it in the kernel also. > > > > > > > > > > What is the concern of having the kernel to support it? > > > > > > > > Just more complicated BTF validation logic, which means that there are > > > > higher chances of permitting invalid BTF. And then the question is > > > > what can the kernel do with those EXTERNs in BTF? Probably nothing. > > > > And that .ksyms section is special, and purely libbpf convention. > > > > Ideally kernel should not allow EXTERN funcs in any other DATASEC. Are > > > > you willing to hard-code ".ksyms" name in kernel for libbpf's sake? > > > > Probably not. The general rule, so far, was that kernel shouldn't see > > > > any unresolved EXTERN at all. Now it's neither here nor there. EXTERN > > > > funcs are ok, EXTERN vars are not. > > > Exactly, it is libbpf convention. The kernel does not need to enforce it. > > > The kernel only needs to be able to support the debug info generated by > > > llvm and being able to display/dump it later. > > > > > > There are many other things in the BTF that the kernel does not need to > > > > Curious, what are those many other things? > VAR '_license'. > deeper things could be STRUCT 'tcp_congestion_ops' and the types under it. > kernel is aware of DATASEC in general, it validates variable sizes and offsets, and datasec size itself. DATASEC can be assigned as value_type_id for maps. So I guess technically you are correct that it doesn't care about VAR _license specifically, but it has to care about DATASEC/VARs in general. Same applies to STRUCT 'tcp_congestion_ops'. I'm fine with extending the kernel with EXTERN funcs, btw. I just don't think it's necessary. But then also let's support EXTERN vars for consistency. > > > > > know. It is there for debug purpose which the BTF is used for. Yes, > > > the func call can be discovered by instruction dump. It is also nice to > > > see everything in one ksyms datasec also during btf dump. > > > > > > If there is a need to strip everything that the kernel does not need > > > from the BTF, it can all be stripped in another "--strip-debug" like > > > option. > > > > Where does this "--strip-debug" option go? Clang, pahole, or bpftool? > > Or am I misunderstanding what you are proposing? > Could be a libbpf option during load? or it can be done during gen skel? libbpf already sanitizes BTF removing and adjusting BTF information (e.g., DATASEC sizes). So that's happening automatically. I wasn't sure what that new stripping option would do, which is why I asked. > > > > > > > > > To support EXTERN var, the kernel part should be fine. I am only not > > > sure why it has to change the vs->size and vs->offset in libbpf? > > > > vs->size and vs->offset are adjusted to match int type. Otherwise > > kernel BTF validation will complain about DATASEC size mismatch. > make sense. so if there is no need to replace it with INT, > they can be left as is? If kernel start supporting EXTERN vars, yes, we won't need to touch it. But of course to support older kernels libbpf will still have to do this. EXTERN vars won't reduce the amount of libbpf logic. > > > > > > > > > > > > > > > > > > > > > > > > If anyone would want to know all the kernel functions that some BPF > > > > > > program is using, they could do it from the instruction dump, with > > > > > > proper addresses and kernel function names nicely displayed there. > > > > > > That's way more useful, IMO.
On Fri, Mar 19, 2021 at 04:02:27PM -0700, Andrii Nakryiko wrote: > On Fri, Mar 19, 2021 at 3:45 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > On Fri, Mar 19, 2021 at 03:29:57PM -0700, Andrii Nakryiko wrote: > > > On Fri, Mar 19, 2021 at 3:19 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > On Fri, Mar 19, 2021 at 02:27:13PM -0700, Andrii Nakryiko wrote: > > > > > On Thu, Mar 18, 2021 at 10:29 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > > > > > On Thu, Mar 18, 2021 at 09:13:56PM -0700, Andrii Nakryiko wrote: > > > > > > > On Thu, Mar 18, 2021 at 4:39 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > > > > > > > > > On Thu, Mar 18, 2021 at 03:53:38PM -0700, Andrii Nakryiko wrote: > > > > > > > > > On Tue, Mar 16, 2021 at 12:01 AM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > > > > > > > > > > > > > This patch makes BTF verifier to accept extern func. It is used for > > > > > > > > > > allowing bpf program to call a limited set of kernel functions > > > > > > > > > > in a later patch. > > > > > > > > > > > > > > > > > > > > When writing bpf prog, the extern kernel function needs > > > > > > > > > > to be declared under a ELF section (".ksyms") which is > > > > > > > > > > the same as the current extern kernel variables and that should > > > > > > > > > > keep its usage consistent without requiring to remember another > > > > > > > > > > section name. > > > > > > > > > > > > > > > > > > > > For example, in a bpf_prog.c: > > > > > > > > > > > > > > > > > > > > extern int foo(struct sock *) __attribute__((section(".ksyms"))) > > > > > > > > > > > > > > > > > > > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 > > > > > > > > > > '(anon)' type_id=18 > > > > > > > > > > [25] FUNC 'foo' type_id=24 linkage=extern > > > > > > > > > > [ ... ] > > > > > > > > > > [33] DATASEC '.ksyms' size=0 vlen=1 > > > > > > > > > > type_id=25 offset=0 size=0 > > > > > > > > > > > > > > > > > > > > LLVM will put the "func" type into the BTF datasec ".ksyms". > > > > > > > > > > The current "btf_datasec_check_meta()" assumes everything under > > > > > > > > > > it is a "var" and ensures it has non-zero size ("!vsi->size" test). > > > > > > > > > > The non-zero size check is not true for "func". This patch postpones the > > > > > > > > > > "!vsi-size" test from "btf_datasec_check_meta()" to > > > > > > > > > > "btf_datasec_resolve()" which has all types collected to decide > > > > > > > > > > if a vsi is a "var" or a "func" and then enforce the "vsi->size" > > > > > > > > > > differently. > > > > > > > > > > > > > > > > > > > > If the datasec only has "func", its "t->size" could be zero. > > > > > > > > > > Thus, the current "!t->size" test is no longer valid. The > > > > > > > > > > invalid "t->size" will still be caught by the later > > > > > > > > > > "last_vsi_end_off > t->size" check. This patch also takes this > > > > > > > > > > chance to consolidate other "t->size" tests ("vsi->offset >= t->size" > > > > > > > > > > "vsi->size > t->size", and "t->size < sum") into the existing > > > > > > > > > > "last_vsi_end_off > t->size" test. > > > > > > > > > > > > > > > > > > > > The LLVM will also put those extern kernel function as an extern > > > > > > > > > > linkage func in the BTF: > > > > > > > > > > > > > > > > > > > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 > > > > > > > > > > '(anon)' type_id=18 > > > > > > > > > > [25] FUNC 'foo' type_id=24 linkage=extern > > > > > > > > > > > > > > > > > > > > This patch allows BTF_FUNC_EXTERN in btf_func_check_meta(). > > > > > > > > > > Also extern kernel function declaration does not > > > > > > > > > > necessary have arg name. Another change in btf_func_check() is > > > > > > > > > > to allow extern function having no arg name. > > > > > > > > > > > > > > > > > > > > The btf selftest is adjusted accordingly. New tests are also added. > > > > > > > > > > > > > > > > > > > > The required LLVM patch: https://reviews.llvm.org/D93563 > > > > > > > > > > > > > > > > > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > > > > > > > > > --- > > > > > > > > > > > > > > > > > > High-level question about EXTERN functions in DATASEC. Does kernel > > > > > > > > > need to see them under DATASEC? What if libbpf just removed all EXTERN > > > > > > > > > funcs from under DATASEC and leave them as "free-floating" EXTERN > > > > > > > > > FUNCs in BTF. > > > > > > > > > > > > > > > > > > We need to tag EXTERNs with DATASECs mainly for libbpf to know whether > > > > > > > > > it's .kconfig or .ksym or other type of externs. Does kernel need to > > > > > > > > > care? > > > > > > > > Although the kernel does not need to know, since the a legit llvm generates it, > > > > > > > > I go with a proper support in the kernel (e.g. bpftool btf dump can better > > > > > > > > reflect what was there). > > > > > > > > > > > > > > LLVM also generates extern VAR with BTF_VAR_EXTERN, yet libbpf is > > > > > > > replacing it with fake INTs. > > > > > > Yep. I noticed the loop in collect_extern() in libbpf. > > > > > > It replaces the var->type with INT. > > > > > > > > > > > > > We could do just that here as well. > > > > > > What to replace in the FUNC case? > > > > > > > > > > if we do that, I'd just replace them with same INTs. Or we can just > > > > > remove the entire DATASEC. Now it is easier to do with BTF write APIs. > > > > > Back then it was a major pain. I'd probably get rid of DATASEC > > > > > altogether instead of that INT replacement, if I had BTF write APIs. > > > > Do you mean vsi->type = INT? > > > > > > yes, that's what existing logic does for EXTERN var > > There may be no var. > > > > sure, but we have btf__add_var(), if we really want VAR ;) > > > > > > > > > > > > > > > > > > > > > > > > > Regardless, supporting it properly in the kernel is a better way to go > > > > > > instead of asking the userspace to move around it. It is not very > > > > > > complicated to support it in the kernel also. > > > > > > > > > > > > What is the concern of having the kernel to support it? > > > > > > > > > > Just more complicated BTF validation logic, which means that there are > > > > > higher chances of permitting invalid BTF. And then the question is > > > > > what can the kernel do with those EXTERNs in BTF? Probably nothing. > > > > > And that .ksyms section is special, and purely libbpf convention. > > > > > Ideally kernel should not allow EXTERN funcs in any other DATASEC. Are > > > > > you willing to hard-code ".ksyms" name in kernel for libbpf's sake? > > > > > Probably not. The general rule, so far, was that kernel shouldn't see > > > > > any unresolved EXTERN at all. Now it's neither here nor there. EXTERN > > > > > funcs are ok, EXTERN vars are not. > > > > Exactly, it is libbpf convention. The kernel does not need to enforce it. > > > > The kernel only needs to be able to support the debug info generated by > > > > llvm and being able to display/dump it later. > > > > > > > > There are many other things in the BTF that the kernel does not need to > > > > > > Curious, what are those many other things? > > VAR '_license'. > > deeper things could be STRUCT 'tcp_congestion_ops' and the types under it. > > > > kernel is aware of DATASEC in general, it validates variable sizes and > offsets, and datasec size itself. Yeah, the kernel still thinks it is data only now. With func in datasec, I think the name "data"sec may be a bit out-dated. > DATASEC can be assigned as > value_type_id for maps. So I guess technically you are correct that it > doesn't care about VAR _license specifically, but it has to care about > DATASEC/VARs in general. Same applies to STRUCT 'tcp_congestion_ops'. > > I'm fine with extending the kernel with EXTERN funcs, btw. I just > don't think it's necessary. But then also let's support EXTERN vars > for consistency. cool. lets explore EXTERN vars support. > > > > > > > > To support EXTERN var, the kernel part should be fine. I am only not > > > > sure why it has to change the vs->size and vs->offset in libbpf? > > > > > > vs->size and vs->offset are adjusted to match int type. Otherwise > > > kernel BTF validation will complain about DATASEC size mismatch. > > make sense. so if there is no need to replace it with INT, > > they can be left as is? > > If kernel start supporting EXTERN vars, yes, we won't need to touch > it. From test_ksyms.c: [22] DATASEC '.ksyms' size=0 vlen=5 type_id=12 offset=0 size=1 type_id=13 offset=0 size=1 For extern, does it make sense for the libbpf to assign 0 to both var offset and size since it does not matter? In the kernel, it can ensure a datasec only has all extern or no extern. array_map_check_btf() will ensure the datasec has no extern. > But of course to support older kernels libbpf will still have to > do this. EXTERN vars won't reduce the amount of libbpf logic.
On Fri, Mar 19, 2021 at 5:13 PM Martin KaFai Lau <kafai@fb.com> wrote: > > On Fri, Mar 19, 2021 at 04:02:27PM -0700, Andrii Nakryiko wrote: > > On Fri, Mar 19, 2021 at 3:45 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > On Fri, Mar 19, 2021 at 03:29:57PM -0700, Andrii Nakryiko wrote: > > > > On Fri, Mar 19, 2021 at 3:19 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > > > On Fri, Mar 19, 2021 at 02:27:13PM -0700, Andrii Nakryiko wrote: > > > > > > On Thu, Mar 18, 2021 at 10:29 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > > > > > > > On Thu, Mar 18, 2021 at 09:13:56PM -0700, Andrii Nakryiko wrote: > > > > > > > > On Thu, Mar 18, 2021 at 4:39 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > > > > > > > > > > > On Thu, Mar 18, 2021 at 03:53:38PM -0700, Andrii Nakryiko wrote: > > > > > > > > > > On Tue, Mar 16, 2021 at 12:01 AM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > > > > > > > > > > > > > > > > > This patch makes BTF verifier to accept extern func. It is used for > > > > > > > > > > > allowing bpf program to call a limited set of kernel functions > > > > > > > > > > > in a later patch. > > > > > > > > > > > > > > > > > > > > > > When writing bpf prog, the extern kernel function needs > > > > > > > > > > > to be declared under a ELF section (".ksyms") which is > > > > > > > > > > > the same as the current extern kernel variables and that should > > > > > > > > > > > keep its usage consistent without requiring to remember another > > > > > > > > > > > section name. > > > > > > > > > > > > > > > > > > > > > > For example, in a bpf_prog.c: > > > > > > > > > > > > > > > > > > > > > > extern int foo(struct sock *) __attribute__((section(".ksyms"))) > > > > > > > > > > > > > > > > > > > > > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 > > > > > > > > > > > '(anon)' type_id=18 > > > > > > > > > > > [25] FUNC 'foo' type_id=24 linkage=extern > > > > > > > > > > > [ ... ] > > > > > > > > > > > [33] DATASEC '.ksyms' size=0 vlen=1 > > > > > > > > > > > type_id=25 offset=0 size=0 > > > > > > > > > > > > > > > > > > > > > > LLVM will put the "func" type into the BTF datasec ".ksyms". > > > > > > > > > > > The current "btf_datasec_check_meta()" assumes everything under > > > > > > > > > > > it is a "var" and ensures it has non-zero size ("!vsi->size" test). > > > > > > > > > > > The non-zero size check is not true for "func". This patch postpones the > > > > > > > > > > > "!vsi-size" test from "btf_datasec_check_meta()" to > > > > > > > > > > > "btf_datasec_resolve()" which has all types collected to decide > > > > > > > > > > > if a vsi is a "var" or a "func" and then enforce the "vsi->size" > > > > > > > > > > > differently. > > > > > > > > > > > > > > > > > > > > > > If the datasec only has "func", its "t->size" could be zero. > > > > > > > > > > > Thus, the current "!t->size" test is no longer valid. The > > > > > > > > > > > invalid "t->size" will still be caught by the later > > > > > > > > > > > "last_vsi_end_off > t->size" check. This patch also takes this > > > > > > > > > > > chance to consolidate other "t->size" tests ("vsi->offset >= t->size" > > > > > > > > > > > "vsi->size > t->size", and "t->size < sum") into the existing > > > > > > > > > > > "last_vsi_end_off > t->size" test. > > > > > > > > > > > > > > > > > > > > > > The LLVM will also put those extern kernel function as an extern > > > > > > > > > > > linkage func in the BTF: > > > > > > > > > > > > > > > > > > > > > > [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 > > > > > > > > > > > '(anon)' type_id=18 > > > > > > > > > > > [25] FUNC 'foo' type_id=24 linkage=extern > > > > > > > > > > > > > > > > > > > > > > This patch allows BTF_FUNC_EXTERN in btf_func_check_meta(). > > > > > > > > > > > Also extern kernel function declaration does not > > > > > > > > > > > necessary have arg name. Another change in btf_func_check() is > > > > > > > > > > > to allow extern function having no arg name. > > > > > > > > > > > > > > > > > > > > > > The btf selftest is adjusted accordingly. New tests are also added. > > > > > > > > > > > > > > > > > > > > > > The required LLVM patch: https://reviews.llvm.org/D93563 > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > > > > > > > > > > --- > > > > > > > > > > > > > > > > > > > > High-level question about EXTERN functions in DATASEC. Does kernel > > > > > > > > > > need to see them under DATASEC? What if libbpf just removed all EXTERN > > > > > > > > > > funcs from under DATASEC and leave them as "free-floating" EXTERN > > > > > > > > > > FUNCs in BTF. > > > > > > > > > > > > > > > > > > > > We need to tag EXTERNs with DATASECs mainly for libbpf to know whether > > > > > > > > > > it's .kconfig or .ksym or other type of externs. Does kernel need to > > > > > > > > > > care? > > > > > > > > > Although the kernel does not need to know, since the a legit llvm generates it, > > > > > > > > > I go with a proper support in the kernel (e.g. bpftool btf dump can better > > > > > > > > > reflect what was there). > > > > > > > > > > > > > > > > LLVM also generates extern VAR with BTF_VAR_EXTERN, yet libbpf is > > > > > > > > replacing it with fake INTs. > > > > > > > Yep. I noticed the loop in collect_extern() in libbpf. > > > > > > > It replaces the var->type with INT. > > > > > > > > > > > > > > > We could do just that here as well. > > > > > > > What to replace in the FUNC case? > > > > > > > > > > > > if we do that, I'd just replace them with same INTs. Or we can just > > > > > > remove the entire DATASEC. Now it is easier to do with BTF write APIs. > > > > > > Back then it was a major pain. I'd probably get rid of DATASEC > > > > > > altogether instead of that INT replacement, if I had BTF write APIs. > > > > > Do you mean vsi->type = INT? > > > > > > > > yes, that's what existing logic does for EXTERN var > > > There may be no var. > > > > > > > sure, but we have btf__add_var(), if we really want VAR ;) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Regardless, supporting it properly in the kernel is a better way to go > > > > > > > instead of asking the userspace to move around it. It is not very > > > > > > > complicated to support it in the kernel also. > > > > > > > > > > > > > > What is the concern of having the kernel to support it? > > > > > > > > > > > > Just more complicated BTF validation logic, which means that there are > > > > > > higher chances of permitting invalid BTF. And then the question is > > > > > > what can the kernel do with those EXTERNs in BTF? Probably nothing. > > > > > > And that .ksyms section is special, and purely libbpf convention. > > > > > > Ideally kernel should not allow EXTERN funcs in any other DATASEC. Are > > > > > > you willing to hard-code ".ksyms" name in kernel for libbpf's sake? > > > > > > Probably not. The general rule, so far, was that kernel shouldn't see > > > > > > any unresolved EXTERN at all. Now it's neither here nor there. EXTERN > > > > > > funcs are ok, EXTERN vars are not. > > > > > Exactly, it is libbpf convention. The kernel does not need to enforce it. > > > > > The kernel only needs to be able to support the debug info generated by > > > > > llvm and being able to display/dump it later. > > > > > > > > > > There are many other things in the BTF that the kernel does not need to > > > > > > > > Curious, what are those many other things? > > > VAR '_license'. > > > deeper things could be STRUCT 'tcp_congestion_ops' and the types under it. > > > > > > > kernel is aware of DATASEC in general, it validates variable sizes and > > offsets, and datasec size itself. > Yeah, the kernel still thinks it is data only now. > With func in datasec, I think the name "data"sec may be a bit out-dated. yep, should have been called SECTION, probably > > > DATASEC can be assigned as > > value_type_id for maps. So I guess technically you are correct that it > > doesn't care about VAR _license specifically, but it has to care about > > DATASEC/VARs in general. Same applies to STRUCT 'tcp_congestion_ops'. > > > > I'm fine with extending the kernel with EXTERN funcs, btw. I just > > don't think it's necessary. But then also let's support EXTERN vars > > for consistency. > cool. lets explore EXTERN vars support. > > > > > > > > > > > To support EXTERN var, the kernel part should be fine. I am only not > > > > > sure why it has to change the vs->size and vs->offset in libbpf? > > > > > > > > vs->size and vs->offset are adjusted to match int type. Otherwise > > > > kernel BTF validation will complain about DATASEC size mismatch. > > > make sense. so if there is no need to replace it with INT, > > > they can be left as is? > > > > If kernel start supporting EXTERN vars, yes, we won't need to touch > > it. > From test_ksyms.c: > [22] DATASEC '.ksyms' size=0 vlen=5 > type_id=12 offset=0 size=1 > type_id=13 offset=0 size=1 > > For extern, does it make sense for the libbpf to assign 0 to > both var offset and size since it does not matter? That's how it is generated and yes, I think that's how it should be kept once kernel supports EXTERN VAR. libbpf is adjusting offsets and sizes in addition to marking VAR itself as GLOBAL_ALLOCATED. If kernel supports EXTERN VAR natively, none of that needs to happen (on newer kernels only, of course). > In the kernel, it can ensure a datasec only has all extern or no extern. > array_map_check_btf() will ensure the datasec has no extern. It certainly makes it less surprising from handling BTF, but it feels like an arbitrary policy, rather than technical restriction. You can mark allocated variables and externs with the same section name and Clang will probably happily generate a single DATASEC with a mix of externs and non-externs. Is that inherently a bad thing? I'm not sure. Basically, I don't know if the kernel should care and enforce or not. > > > But of course to support older kernels libbpf will still have to > > do this. EXTERN vars won't reduce the amount of libbpf logic.
On Sat, Mar 20, 2021 at 10:18:36AM -0700, Andrii Nakryiko wrote: > > From test_ksyms.c: > > [22] DATASEC '.ksyms' size=0 vlen=5 > > type_id=12 offset=0 size=1 > > type_id=13 offset=0 size=1 > > > > For extern, does it make sense for the libbpf to assign 0 to > > both var offset and size since it does not matter? > > That's how it is generated and yes, I think that's how it should be > kept once kernel supports EXTERN VAR. libbpf is adjusting offsets and > sizes in addition to marking VAR itself as GLOBAL_ALLOCATED. If kernel > supports EXTERN VAR natively, none of that needs to happen (on newer > kernels only, of course). > > > In the kernel, it can ensure a datasec only has all extern or no extern. > > array_map_check_btf() will ensure the datasec has no extern. > > It certainly makes it less surprising from handling BTF, but it feels > like an arbitrary policy, rather than technical restriction. You can > mark allocated variables and externs with the same section name and > Clang will probably happily generate a single DATASEC with a mix of > externs and non-externs. Is that inherently a bad thing? I'm not sure. > Basically, I don't know if the kernel should care and enforce or not. I have thought a bit more on this. The offset=0 of extern var can be used in the verification but I think it will still have some open ended questions like arraymap. I will use your suggestion in libbpf and do something similar as the extern VAR: replace the FUNC in datasec with INT (btf__add_var() if needed). > > > > > > But of course to support older kernels libbpf will still have to > > > do this. EXTERN vars won't reduce the amount of libbpf logic.
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 369faeddf1df..96cd24020a38 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -3439,7 +3439,7 @@ static s32 btf_func_check_meta(struct btf_verifier_env *env, return -EINVAL; } - if (btf_type_vlen(t) > BTF_FUNC_GLOBAL) { + if (btf_type_vlen(t) > BTF_FUNC_EXTERN) { btf_verifier_log_type(env, t, "Invalid func linkage"); return -EINVAL; } @@ -3532,7 +3532,7 @@ static s32 btf_datasec_check_meta(struct btf_verifier_env *env, u32 meta_left) { const struct btf_var_secinfo *vsi; - u64 last_vsi_end_off = 0, sum = 0; + u64 last_vsi_end_off = 0; u32 i, meta_needed; meta_needed = btf_type_vlen(t) * sizeof(*vsi); @@ -3543,11 +3543,6 @@ static s32 btf_datasec_check_meta(struct btf_verifier_env *env, return -EINVAL; } - if (!t->size) { - btf_verifier_log_type(env, t, "size == 0"); - return -EINVAL; - } - if (btf_type_kflag(t)) { btf_verifier_log_type(env, t, "Invalid btf_info kind_flag"); return -EINVAL; @@ -3569,19 +3564,13 @@ static s32 btf_datasec_check_meta(struct btf_verifier_env *env, return -EINVAL; } - if (vsi->offset < last_vsi_end_off || vsi->offset >= t->size) { + if (vsi->offset < last_vsi_end_off) { btf_verifier_log_vsi(env, t, vsi, "Invalid offset"); return -EINVAL; } - if (!vsi->size || vsi->size > t->size) { - btf_verifier_log_vsi(env, t, vsi, - "Invalid size"); - return -EINVAL; - } - - last_vsi_end_off = vsi->offset + vsi->size; + last_vsi_end_off = (u64)vsi->offset + vsi->size; if (last_vsi_end_off > t->size) { btf_verifier_log_vsi(env, t, vsi, "Invalid offset+size"); @@ -3589,12 +3578,6 @@ static s32 btf_datasec_check_meta(struct btf_verifier_env *env, } btf_verifier_log_vsi(env, t, vsi, NULL); - sum += vsi->size; - } - - if (t->size < sum) { - btf_verifier_log_type(env, t, "Invalid btf_info size"); - return -EINVAL; } return meta_needed; @@ -3611,9 +3594,28 @@ static int btf_datasec_resolve(struct btf_verifier_env *env, u32 var_type_id = vsi->type, type_id, type_size = 0; const struct btf_type *var_type = btf_type_by_id(env->btf, var_type_id); - if (!var_type || !btf_type_is_var(var_type)) { + if (!var_type) { + btf_verifier_log_vsi(env, v->t, vsi, + "type not found"); + return -EINVAL; + } + + if (btf_type_is_func(var_type)) { + if (vsi->size || vsi->offset) { + btf_verifier_log_vsi(env, v->t, vsi, + "Invalid size/offset"); + return -EINVAL; + } + continue; + } else if (btf_type_is_var(var_type)) { + if (!vsi->size) { + btf_verifier_log_vsi(env, v->t, vsi, + "Invalid size"); + return -EINVAL; + } + } else { btf_verifier_log_vsi(env, v->t, vsi, - "Not a VAR kind member"); + "Neither a VAR nor a FUNC"); return -EINVAL; } @@ -3849,9 +3851,11 @@ static int btf_func_check(struct btf_verifier_env *env, const struct btf_param *args; const struct btf *btf; u16 nr_args, i; + bool is_extern; btf = env->btf; proto_type = btf_type_by_id(btf, t->type); + is_extern = btf_type_vlen(t) == BTF_FUNC_EXTERN; if (!proto_type || !btf_type_is_func_proto(proto_type)) { btf_verifier_log_type(env, t, "Invalid type_id"); @@ -3861,7 +3865,7 @@ static int btf_func_check(struct btf_verifier_env *env, args = (const struct btf_param *)(proto_type + 1); nr_args = btf_type_vlen(proto_type); for (i = 0; i < nr_args; i++) { - if (!args[i].name_off && args[i].type) { + if (!is_extern && !args[i].name_off && args[i].type) { btf_verifier_log_type(env, t, "Invalid arg#%u", i + 1); return -EINVAL; } diff --git a/tools/testing/selftests/bpf/prog_tests/btf.c b/tools/testing/selftests/bpf/prog_tests/btf.c index 0457ae32b270..e469482833b2 100644 --- a/tools/testing/selftests/bpf/prog_tests/btf.c +++ b/tools/testing/selftests/bpf/prog_tests/btf.c @@ -498,7 +498,7 @@ static struct btf_raw_test raw_tests[] = { .value_type_id = 7, .max_entries = 1, .btf_load_err = true, - .err_str = "Invalid size", + .err_str = "Invalid offset+size", }, { .descr = "global data test #10, invalid var size", @@ -696,7 +696,7 @@ static struct btf_raw_test raw_tests[] = { .err_str = "Invalid offset", }, { - .descr = "global data test #15, not var kind", + .descr = "global data test #15, not var/func kind", .raw_types = { /* int */ BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */ @@ -716,7 +716,7 @@ static struct btf_raw_test raw_tests[] = { .value_type_id = 3, .max_entries = 1, .btf_load_err = true, - .err_str = "Not a VAR kind member", + .err_str = "Neither a VAR nor a FUNC", }, { .descr = "global data test #16, invalid var referencing sec", @@ -2803,7 +2803,7 @@ static struct btf_raw_test raw_tests[] = { BTF_FUNC_PROTO_ARG_ENC(NAME_TBD, 1), BTF_FUNC_PROTO_ARG_ENC(NAME_TBD, 2), /* void func(int a, unsigned int b) */ - BTF_TYPE_ENC(NAME_TBD, BTF_INFO_ENC(BTF_KIND_FUNC, 0, 2), 3), /* [4] */ + BTF_TYPE_ENC(NAME_TBD, BTF_INFO_ENC(BTF_KIND_FUNC, 0, 3), 3), /* [4] */ BTF_END_RAW, }, .str_sec = "\0a\0b\0func", @@ -3531,6 +3531,152 @@ static struct btf_raw_test raw_tests[] = { .max_entries = 1, }, +{ + .descr = "datasec: func only", + .raw_types = { + /* int */ + BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */ + /* void (*)(void) */ + BTF_FUNC_PROTO_ENC(0, 0), /* [2] */ + BTF_FUNC_ENC(NAME_NTH(1), 2), /* [3] */ + BTF_FUNC_ENC(NAME_NTH(2), 2), /* [4] */ + /* .ksym section */ + BTF_TYPE_ENC(NAME_NTH(3), BTF_INFO_ENC(BTF_KIND_DATASEC, 0, 2), 0), /* [5] */ + BTF_VAR_SECINFO_ENC(3, 0, 0), + BTF_VAR_SECINFO_ENC(4, 0, 0), + BTF_END_RAW, + }, + BTF_STR_SEC("\0foo1\0foo2\0.ksym\0"), + .map_type = BPF_MAP_TYPE_ARRAY, + .key_size = sizeof(int), + .value_size = sizeof(int), + .key_type_id = 1, + .value_type_id = 1, + .max_entries = 1, +}, + +{ + .descr = "datasec: func and var", + .raw_types = { + /* int */ + BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */ + /* void (*)(void) */ + BTF_FUNC_PROTO_ENC(0, 0), /* [2] */ + BTF_FUNC_ENC(NAME_NTH(1), 2), /* [3] */ + BTF_FUNC_ENC(NAME_NTH(2), 2), /* [4] */ + /* int */ + BTF_VAR_ENC(NAME_NTH(4), 1, 0), /* [5] */ + BTF_VAR_ENC(NAME_NTH(5), 1, 0), /* [6] */ + /* .ksym section */ + BTF_TYPE_ENC(NAME_NTH(3), BTF_INFO_ENC(BTF_KIND_DATASEC, 0, 4), 8), /* [7] */ + BTF_VAR_SECINFO_ENC(3, 0, 0), + BTF_VAR_SECINFO_ENC(4, 0, 0), + BTF_VAR_SECINFO_ENC(5, 0, 4), + BTF_VAR_SECINFO_ENC(6, 4, 4), + BTF_END_RAW, + }, + BTF_STR_SEC("\0foo1\0foo2\0.ksym\0a\0b\0"), + .map_type = BPF_MAP_TYPE_ARRAY, + .key_size = sizeof(int), + .value_size = sizeof(int), + .key_type_id = 1, + .value_type_id = 1, + .max_entries = 1, +}, + +{ + .descr = "datasec: func and var, invalid size/offset for func", + .raw_types = { + /* int */ + BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */ + /* void (*)(void) */ + BTF_FUNC_PROTO_ENC(0, 0), /* [2] */ + BTF_FUNC_ENC(NAME_NTH(1), 2), /* [3] */ + BTF_FUNC_ENC(NAME_NTH(2), 2), /* [4] */ + /* int */ + BTF_VAR_ENC(NAME_NTH(4), 1, 0), /* [5] */ + BTF_VAR_ENC(NAME_NTH(5), 1, 0), /* [6] */ + /* .ksym section */ + BTF_TYPE_ENC(NAME_NTH(3), BTF_INFO_ENC(BTF_KIND_DATASEC, 0, 4), 8), /* [7] */ + BTF_VAR_SECINFO_ENC(3, 0, 0), + BTF_VAR_SECINFO_ENC(5, 0, 4), + BTF_VAR_SECINFO_ENC(4, 4, 0), /* func has non zero vsi->offset */ + BTF_VAR_SECINFO_ENC(6, 4, 4), + BTF_END_RAW, + }, + BTF_STR_SEC("\0foo1\0foo2\0.ksym\0a\0b\0"), + .map_type = BPF_MAP_TYPE_ARRAY, + .key_size = sizeof(int), + .value_size = sizeof(int), + .key_type_id = 1, + .value_type_id = 1, + .max_entries = 1, + .btf_load_err = true, + .err_str = "Invalid size/offset", +}, + +{ + .descr = "datasec: func and var, datasec size 0", + .raw_types = { + /* int */ + BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */ + /* void (*)(void) */ + BTF_FUNC_PROTO_ENC(0, 0), /* [2] */ + BTF_FUNC_ENC(NAME_NTH(1), 2), /* [3] */ + BTF_FUNC_ENC(NAME_NTH(2), 2), /* [4] */ + /* int */ + BTF_VAR_ENC(NAME_NTH(4), 1, 0), /* [5] */ + BTF_VAR_ENC(NAME_NTH(5), 1, 0), /* [6] */ + /* .ksym section */ + BTF_TYPE_ENC(NAME_NTH(3), BTF_INFO_ENC(BTF_KIND_DATASEC, 0, 4), 0), /* [7] */ + BTF_VAR_SECINFO_ENC(3, 0, 0), + BTF_VAR_SECINFO_ENC(4, 0, 0), + BTF_VAR_SECINFO_ENC(5, 0, 4), + BTF_VAR_SECINFO_ENC(6, 4, 4), + BTF_END_RAW, + }, + BTF_STR_SEC("\0foo1\0foo2\0.ksym\0a\0b\0"), + .map_type = BPF_MAP_TYPE_ARRAY, + .key_size = sizeof(int), + .value_size = sizeof(int), + .key_type_id = 1, + .value_type_id = 1, + .max_entries = 1, + .btf_load_err = true, + .err_str = "Invalid offset+size", +}, + +{ + .descr = "datasec: func and var, zero vsi->size for var", + .raw_types = { + /* int */ + BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */ + /* void (*)(void) */ + BTF_FUNC_PROTO_ENC(0, 0), /* [2] */ + BTF_FUNC_ENC(NAME_NTH(1), 2), /* [3] */ + BTF_FUNC_ENC(NAME_NTH(2), 2), /* [4] */ + /* int */ + BTF_VAR_ENC(NAME_NTH(4), 1, 0), /* [5] */ + BTF_VAR_ENC(NAME_NTH(5), 1, 0), /* [6] */ + /* .ksym section */ + BTF_TYPE_ENC(NAME_NTH(3), BTF_INFO_ENC(BTF_KIND_DATASEC, 0, 4), 8), /* [7] */ + BTF_VAR_SECINFO_ENC(3, 0, 0), + BTF_VAR_SECINFO_ENC(4, 0, 0), + BTF_VAR_SECINFO_ENC(5, 0, 0), /* var has zero vsi->size */ + BTF_VAR_SECINFO_ENC(6, 0, 4), + BTF_END_RAW, + }, + BTF_STR_SEC("\0foo1\0foo2\0.ksym\0a\0b\0"), + .map_type = BPF_MAP_TYPE_ARRAY, + .key_size = sizeof(int), + .value_size = sizeof(int), + .key_type_id = 1, + .value_type_id = 1, + .max_entries = 1, + .btf_load_err = true, + .err_str = "Invalid size", +}, + { .descr = "float test #1, well-formed", .raw_types = {
This patch makes BTF verifier to accept extern func. It is used for allowing bpf program to call a limited set of kernel functions in a later patch. When writing bpf prog, the extern kernel function needs to be declared under a ELF section (".ksyms") which is the same as the current extern kernel variables and that should keep its usage consistent without requiring to remember another section name. For example, in a bpf_prog.c: extern int foo(struct sock *) __attribute__((section(".ksyms"))) [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 '(anon)' type_id=18 [25] FUNC 'foo' type_id=24 linkage=extern [ ... ] [33] DATASEC '.ksyms' size=0 vlen=1 type_id=25 offset=0 size=0 LLVM will put the "func" type into the BTF datasec ".ksyms". The current "btf_datasec_check_meta()" assumes everything under it is a "var" and ensures it has non-zero size ("!vsi->size" test). The non-zero size check is not true for "func". This patch postpones the "!vsi-size" test from "btf_datasec_check_meta()" to "btf_datasec_resolve()" which has all types collected to decide if a vsi is a "var" or a "func" and then enforce the "vsi->size" differently. If the datasec only has "func", its "t->size" could be zero. Thus, the current "!t->size" test is no longer valid. The invalid "t->size" will still be caught by the later "last_vsi_end_off > t->size" check. This patch also takes this chance to consolidate other "t->size" tests ("vsi->offset >= t->size" "vsi->size > t->size", and "t->size < sum") into the existing "last_vsi_end_off > t->size" test. The LLVM will also put those extern kernel function as an extern linkage func in the BTF: [24] FUNC_PROTO '(anon)' ret_type_id=15 vlen=1 '(anon)' type_id=18 [25] FUNC 'foo' type_id=24 linkage=extern This patch allows BTF_FUNC_EXTERN in btf_func_check_meta(). Also extern kernel function declaration does not necessary have arg name. Another change in btf_func_check() is to allow extern function having no arg name. The btf selftest is adjusted accordingly. New tests are also added. The required LLVM patch: https://reviews.llvm.org/D93563 Signed-off-by: Martin KaFai Lau <kafai@fb.com> --- kernel/bpf/btf.c | 52 ++++--- tools/testing/selftests/bpf/prog_tests/btf.c | 154 ++++++++++++++++++- 2 files changed, 178 insertions(+), 28 deletions(-)