On Thu, Nov 11, 2021 at 9:02 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> From: Alexei Starovoitov <ast@xxxxxxxxxx>
>
> struct bpf_core_relo is generated by llvm and processed by libbpf.
> It's a de-facto uapi.
> With CO-RE in the kernel the struct bpf_core_relo becomes uapi de-jure.
> Add an ability to pass a set of 'struct bpf_core_relo' to prog_load command
> and let the kernel perform CO-RE relocations.
>
> Note the struct bpf_line_info and struct bpf_func_info have the same
> layout when passed from LLVM to libbpf and from libbpf to the kernel
> except "insn_off" fields means "byte offset" when LLVM generates it.
> Then libbpf converts it to "insn index" to pass to the kernel.
> The struct bpf_core_relo's "insn_off" field is always "byte offset".
>
> Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> ---
> include/linux/bpf.h | 3 ++
> include/uapi/linux/bpf.h | 59 +++++++++++++++++++++++++++-
> kernel/bpf/btf.c | 7 ++++
> kernel/bpf/syscall.c | 2 +-
> kernel/bpf/verifier.c | 71 ++++++++++++++++++++++++++++++++++
> tools/include/uapi/linux/bpf.h | 59 +++++++++++++++++++++++++++-
> tools/lib/bpf/relo_core.h | 53 -------------------------
> 7 files changed, 198 insertions(+), 56 deletions(-)
>
[...]
> static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr)
> {
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1aafb43f61d1..c2246414e182 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -10268,6 +10268,73 @@ static int check_btf_line(struct bpf_verifier_env *env,
> return err;
> }
>
> +static int check_core_relo(struct bpf_verifier_env *env,
> + const union bpf_attr *attr,
> + bpfptr_t uattr)
> +{
> + u32 i, nr_core_relo, ncopy, expected_size, rec_size;
> + struct bpf_core_relo core_relo = {};
> + struct bpf_prog *prog = env->prog;
> + const struct btf *btf = prog->aux->btf;
> + bpfptr_t u_core_relo;
> + int err;
> +
> + nr_core_relo = attr->core_relo_cnt;
> + if (!nr_core_relo)
> + return 0;
> + if (nr_core_relo > INT_MAX / sizeof(struct bpf_core_relo))
> + return -EINVAL;
> +
> + rec_size = attr->core_relo_rec_size;
> + if (rec_size != sizeof(struct bpf_core_relo))
> + return -EINVAL;
For func_info we allow trailing zeroes (in check_btf_func, we check
MIN_BPF_FUNCINFO_SIZE and MAX_FUNCINFO_REC_SIZE). Shouldn't we do
something like that here?
> +
> + u_core_relo = make_bpfptr(attr->core_relo, uattr.is_kernel);
> + expected_size = sizeof(struct bpf_core_relo);
> + ncopy = min_t(u32, expected_size, rec_size);
I'm confused, a few lines above you errored out if expected_size != rec_size...
> +
> + /* Unlike func_info and line_info, copy and apply each CO-RE
> + * relocation record one at a time
> + */
> + for (i = 0; i < nr_core_relo; i++) {
> + /* future proofing when sizeof(bpf_core_relo) changes */
> + err = bpf_check_uarg_tail_zero(u_core_relo, expected_size, rec_size);
> + if (err) {
> + if (err == -E2BIG) {
> + verbose(env, "nonzero tailing record in core_relo");
> + if (copy_to_bpfptr_offset(uattr,
> + offsetof(union bpf_attr, core_relo_rec_size),
> + &expected_size, sizeof(expected_size)))
> + err = -EFAULT;
> + }
> + goto err;
> + }
> +
> + if (copy_from_bpfptr(&core_relo, u_core_relo, ncopy)) {
> + err = -EFAULT;
> + goto err;
> + }
> +
> + if (core_relo.insn_off % 8 || core_relo.insn_off / 8 >= prog->len) {
> + verbose(env, "Invalid core_relo[%u].insn_off:%u prog->len:%u\n",
> + i, core_relo.insn_off, prog->len);
> + err = -EINVAL;
> + goto err;
> + }
> +
> + err = bpf_core_relo_apply(&env->log, btf, &core_relo, i,
> + &prog->insnsi[core_relo.insn_off / 8]);
> + if (err)
> + goto err;
> + bpfptr_add(&u_core_relo, rec_size);
> + }
> +
> + return 0;
don't need this return if you initialize err = 0 at the beginning?
> +
> +err:
> + return err;
> +}
> +
> static int check_btf_info(struct bpf_verifier_env *env,
> const union bpf_attr *attr,
> bpfptr_t uattr)
[...]
