Re: [PATCH v6 bpf-next 14/21] libbpf: Generate loader program out of BPF ELF file.

On Fri, Jun 11, 2021 at 1:23 PM Andrii Nakryiko
[off-list ref] wrote:

On Thu, May 13, 2021 at 5:36 PM Alexei Starovoitov
[off-list ref] wrote:

quoted

From: Alexei Starovoitov <ast@kernel.org>

The BPF program loading process performed by libbpf is quite complex
and consists of the following steps:
"open" phase:
- parse elf file and remember relocations, sections
- collect externs and ksyms including their btf_ids in prog's BTF
- patch BTF datasec (since llvm couldn't do it)
- init maps (old style map_def, BTF based, global data map, kconfig map)
- collect relocations against progs and maps
"load" phase:
- probe kernel features
- load vmlinux BTF
- resolve externs (kconfig and ksym)
- load program BTF
- init struct_ops
- create maps
- apply CO-RE relocations
- patch ld_imm64 insns with src_reg=PSEUDO_MAP, PSEUDO_MAP_VALUE, PSEUDO_BTF_ID
- reposition subprograms and adjust call insns
- sanitize and load progs

During this process libbpf does sys_bpf() calls to load BTF, create maps,
populate maps and finally load programs.
Instead of actually doing the syscalls generate a trace of what libbpf
would have done and represent it as the "loader program".
The "loader program" consists of single map with:
- union bpf_attr(s)
- BTF bytes
- map value bytes
- insns bytes
and single bpf program that passes bpf_attr(s) and data into bpf_sys_bpf() helper.
Executing such "loader program" via bpf_prog_test_run() command will
replay the sequence of syscalls that libbpf would have done which will result
the same maps created and programs loaded as specified in the elf file.
The "loader program" removes libelf and majority of libbpf dependency from
program loading process.

kconfig, typeless ksym, struct_ops and CO-RE are not supported yet.

The order of relocate_data and relocate_calls had to change, so that
bpf_gen__prog_load() can see all relocations for a given program with
correct insn_idx-es.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/Build              |   2 +-
 tools/lib/bpf/bpf_gen_internal.h |  40 ++
 tools/lib/bpf/gen_loader.c       | 689 +++++++++++++++++++++++++++++++
 tools/lib/bpf/libbpf.c           | 226 ++++++++--
 tools/lib/bpf/libbpf.h           |  12 +
 tools/lib/bpf/libbpf.map         |   1 +
 tools/lib/bpf/libbpf_internal.h  |   2 +
 tools/lib/bpf/skel_internal.h    | 123 ++++++
 8 files changed, 1060 insertions(+), 35 deletions(-)
 create mode 100644 tools/lib/bpf/bpf_gen_internal.h
 create mode 100644 tools/lib/bpf/gen_loader.c
 create mode 100644 tools/lib/bpf/skel_internal.h

[...]

quoted

+void bpf_gen__prog_load(struct bpf_gen *gen,
+                       struct bpf_prog_load_params *load_attr, int prog_idx)
+{
+       int attr_size = offsetofend(union bpf_attr, fd_array);
+       int prog_load_attr, license, insns, func_info, line_info;
+       union bpf_attr attr;
+
+       memset(&attr, 0, attr_size);
+       pr_debug("gen: prog_load: type %d insns_cnt %zd\n",
+                load_attr->prog_type, load_attr->insn_cnt);
+       /* add license string to blob of bytes */
+       license = add_data(gen, load_attr->license, strlen(load_attr->license) + 1);
+       /* add insns to blob of bytes */
+       insns = add_data(gen, load_attr->insns,
+                        load_attr->insn_cnt * sizeof(struct bpf_insn));
+
+       attr.prog_type = load_attr->prog_type;
+       attr.expected_attach_type = load_attr->expected_attach_type;
+       attr.attach_btf_id = load_attr->attach_btf_id;
+       attr.prog_ifindex = load_attr->prog_ifindex;
+       attr.kern_version = 0;
+       attr.insn_cnt = (__u32)load_attr->insn_cnt;
+       attr.prog_flags = load_attr->prog_flags;
+
+       attr.func_info_rec_size = load_attr->func_info_rec_size;
+       attr.func_info_cnt = load_attr->func_info_cnt;
+       func_info = add_data(gen, load_attr->func_info,
+                            attr.func_info_cnt * attr.func_info_rec_size);
+
+       attr.line_info_rec_size = load_attr->line_info_rec_size;
+       attr.line_info_cnt = load_attr->line_info_cnt;
+       line_info = add_data(gen, load_attr->line_info,
+                            attr.line_info_cnt * attr.line_info_rec_size);
+

Hey Alexei,

Coverity ([0] and [1]) is complaining that load_attr->func_info and
load_attr->line_info can be NULL in some cases, which will lead to
NULL deref. I'm not sure if we restrict gen_loader to be only used
with BPF applications that have BTF embedded. If not, then it will
cause a crash, so we need to protect against that. Please take a look.

  [0] https://scan3.coverity.com/reports.htm#v40547/p11903/fileInstanceId=53874059&defectInstanceId=10901198&mergedDefectId=349034
  [1] https://scan3.coverity.com/reports.htm#v40547/p11903/fileInstanceId=53874059&defectInstanceId=10901191&mergedDefectId=349033

Not sure why we have two issues above, they both look identical, but
for completeness I included both.

I cannot access these links.
Looking at the code the func_info can be NULL,
but in such case the line_info_cnt will be zero.
realloc_data_buf() will succeed as a nop and then there will be:
memcpy(gen->data_cur, NULL, 0);
which is ok to do. I double checked.
So this coverity issue looks like a false positive.