On Sun, Sep 13, 2020 at 10:01 PM Hao Luo haoluo@google.com wrote:
Thanks for review, Andrii.
One question, should I add bpf_{per, this}_cpu_ptr() to the bpf_base_func_proto() in kernel/bpf/helpers.c?
Yes, probably, but given it allows poking at kernel memory, it probably needs to be guarded by perfmon_capable() check, similar to bpf_get_current_task_proto.
On Fri, Sep 4, 2020 at 1:04 PM Andrii Nakryiko andrii.nakryiko@gmail.com wrote:
On Thu, Sep 3, 2020 at 3:35 PM Hao Luo haoluo@google.com wrote:
Add bpf_per_cpu_ptr() to help bpf programs access percpu vars. bpf_per_cpu_ptr() has the same semantic as per_cpu_ptr() in the kernel except that it may return NULL. This happens when the cpu parameter is out of range. So the caller must check the returned value.
Acked-by: Andrii Nakryiko andriin@fb.com Signed-off-by: Hao Luo haoluo@google.com
include/linux/bpf.h | 3 ++ include/linux/btf.h | 11 ++++++ include/uapi/linux/bpf.h | 17 +++++++++ kernel/bpf/btf.c | 10 ------ kernel/bpf/verifier.c | 66 +++++++++++++++++++++++++++++++--- kernel/trace/bpf_trace.c | 18 ++++++++++ tools/include/uapi/linux/bpf.h | 17 +++++++++ 7 files changed, 128 insertions(+), 14 deletions(-)
[...]
@@ -5002,6 +5016,30 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn regs[BPF_REG_0].type = PTR_TO_MEM_OR_NULL; regs[BPF_REG_0].id = ++env->id_gen; regs[BPF_REG_0].mem_size = meta.mem_size;
} else if (fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL) {Given this is internal implementation detail, this return type is fine, but I'm wondering if it would be better to just make PTR_TO_BTF_ID to allow not just structs? E.g., if we have an int, just allow reading those 4 bytes.
Not sure what the implications are in terms of implementation, but conceptually that shouldn't be a problem, given we do have BTF type ID describing size and all.
Yeah. Totally agree. I looked at it initially. My take is PTR_TO_BTF_ID is meant for struct types. It required some code refactoring to break this assumption. I can add it to my TODO list and investigate it if this makes more sense.
PTR_TO_BTF_ID was *implemented* for struct, but at least naming-wise nothing suggests it has to be restricted to structs. But yeah, this should be a separate change, don't block your patches on that.
const struct btf_type *t;
[...]