On Wed, Apr 05, 2023 at 11:34:53PM +0200, Ilya Leoshkevich wrote:
SNIP
>
> +int bpf_get_kfunc_addr(const struct bpf_prog *prog, u32 func_id,
> + u16 btf_fd_idx, u8 **func_addr)
> +{
> + const struct bpf_kfunc_desc *desc;
> +
> + desc = find_kfunc_desc(prog, func_id, btf_fd_idx);
> + if (!desc)
> + return -EFAULT;
> +
> + *func_addr = (u8 *)desc->addr;
> + return 0;
> +}
> +
> static struct btf *__find_kfunc_desc_btf(struct bpf_verifier_env *env,
> s16 offset)
> {
> @@ -2672,14 +2691,19 @@ static int add_kfunc_call(struct bpf_verifier_env *env, u32 func_id, s16 offset)
> return -EINVAL;
> }
>
> - call_imm = BPF_CALL_IMM(addr);
> - /* Check whether or not the relative offset overflows desc->imm */
> - if ((unsigned long)(s32)call_imm != call_imm) {
> - verbose(env, "address of kernel function %s is out of range\n",
> - func_name);
> - return -EINVAL;
> + if (bpf_jit_supports_far_kfunc_call()) {
> + call_imm = func_id;
> + } else {
> + call_imm = BPF_CALL_IMM(addr);
we compute call_imm again in fixup_kfunc_call, seems like we could store
the address and the func_id in desc and have fixup_kfunc_call do the
insn->imm setup
> + /* Check whether the relative offset overflows desc->imm */
> + if ((unsigned long)(s32)call_imm != call_imm) {
> + verbose(env, "address of kernel function %s is out of range\n",
> + func_name);
> + return -EINVAL;
> + }
> }
>
> +
nit, extra line
> if (bpf_dev_bound_kfunc_id(func_id)) {
> err = bpf_dev_bound_kfunc_check(&env->log, prog_aux);
> if (err)
> @@ -2690,6 +2714,7 @@ static int add_kfunc_call(struct bpf_verifier_env *env, u32 func_id, s16 offset)
> desc->func_id = func_id;
> desc->imm = call_imm;
> desc->offset = offset;
> + desc->addr = addr;
> err = btf_distill_func_proto(&env->log, desc_btf,
> func_proto, func_name,
> &desc->func_model);
> @@ -2699,19 +2724,19 @@ static int add_kfunc_call(struct bpf_verifier_env *env, u32 func_id, s16 offset)
> return err;
> }
>
> -static int kfunc_desc_cmp_by_imm(const void *a, const void *b)
> +static int kfunc_desc_cmp_by_imm_off(const void *a, const void *b)
> {
> const struct bpf_kfunc_desc *d0 = a;
> const struct bpf_kfunc_desc *d1 = b;
>
> - if (d0->imm > d1->imm)
> - return 1;
> - else if (d0->imm < d1->imm)
> - return -1;
> + if (d0->imm != d1->imm)
> + return d0->imm < d1->imm ? -1 : 1;
> + if (d0->offset != d1->offset)
> + return d0->offset < d1->offset ? -1 : 1;
> return 0;
> }
>
SNIP
> +/* replace a generic kfunc with a specialized version if necessary */
> +static void fixup_kfunc_desc(struct bpf_verifier_env *env,
> + struct bpf_kfunc_desc *desc)
> +{
> + struct bpf_prog *prog = env->prog;
> + u32 func_id = desc->func_id;
> + u16 offset = desc->offset;
> + bool seen_direct_write;
> + void *xdp_kfunc;
> + bool is_rdonly;
> +
> + if (bpf_dev_bound_kfunc_id(func_id)) {
> + xdp_kfunc = bpf_dev_bound_resolve_kfunc(prog, func_id);
> + if (xdp_kfunc) {
> + desc->addr = (unsigned long)xdp_kfunc;
> + return;
> + }
> + /* fallback to default kfunc when not supported by netdev */
> + }
> +
> + if (offset)
> + return;
> +
> + if (func_id == special_kfunc_list[KF_bpf_dynptr_from_skb]) {
> + seen_direct_write = env->seen_direct_write;
> + is_rdonly = !may_access_direct_pkt_data(env, NULL, BPF_WRITE);
> +
> + if (is_rdonly)
> + desc->addr = (unsigned long)bpf_dynptr_from_skb_rdonly;
> +
> + /* restore env->seen_direct_write to its original value, since
> + * may_access_direct_pkt_data mutates it
> + */
> + env->seen_direct_write = seen_direct_write;
> + }
could we do this directly in add_kfunc_call?
thanks,
jirka
> +}
> +
> +static void fixup_kfunc_desc_tab(struct bpf_verifier_env *env)
> +{
> + struct bpf_kfunc_desc_tab *tab = env->prog->aux->kfunc_tab;
> + u32 i;
> +
> + if (!tab)
> + return;
> +
> + for (i = 0; i < tab->nr_descs; i++)
> + fixup_kfunc_desc(env, &tab->descs[i]);
> +}
> +
> static int fixup_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
> struct bpf_insn *insn_buf, int insn_idx, int *cnt)
> {
> const struct bpf_kfunc_desc *desc;
> - void *xdp_kfunc;
>
> if (!insn->imm) {
> verbose(env, "invalid kernel function call not eliminated in verifier pass\n");
> @@ -17355,18 +17429,9 @@ static int fixup_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>
> *cnt = 0;
>
> - if (bpf_dev_bound_kfunc_id(insn->imm)) {
> - xdp_kfunc = bpf_dev_bound_resolve_kfunc(env->prog, insn->imm);
> - if (xdp_kfunc) {
> - insn->imm = BPF_CALL_IMM(xdp_kfunc);
> - return 0;
> - }
> -
> - /* fallback to default kfunc when not supported by netdev */
> - }
> -
> - /* insn->imm has the btf func_id. Replace it with
> - * an address (relative to __bpf_call_base).
> + /* insn->imm has the btf func_id. Replace it with an offset relative to
> + * __bpf_call_base, unless the JIT needs to call functions that are
> + * further than 32 bits away (bpf_jit_supports_far_kfunc_call()).
> */
> desc = find_kfunc_desc(env->prog, insn->imm, insn->off);
> if (!desc) {
> @@ -17375,7 +17440,8 @@ static int fixup_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
> return -EFAULT;
> }
>
> - insn->imm = desc->imm;
> + if (!bpf_jit_supports_far_kfunc_call())
> + insn->imm = BPF_CALL_IMM(desc->addr);
> if (insn->off)
> return 0;
> if (desc->func_id == special_kfunc_list[KF_bpf_obj_new_impl]) {
> @@ -17400,17 +17466,6 @@ static int fixup_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
> desc->func_id == special_kfunc_list[KF_bpf_rdonly_cast]) {
> insn_buf[0] = BPF_MOV64_REG(BPF_REG_0, BPF_REG_1);
> *cnt = 1;
> - } else if (desc->func_id == special_kfunc_list[KF_bpf_dynptr_from_skb]) {
> - bool seen_direct_write = env->seen_direct_write;
> - bool is_rdonly = !may_access_direct_pkt_data(env, NULL, BPF_WRITE);
> -
> - if (is_rdonly)
> - insn->imm = BPF_CALL_IMM(bpf_dynptr_from_skb_rdonly);
> -
> - /* restore env->seen_direct_write to its original value, since
> - * may_access_direct_pkt_data mutates it
> - */
> - env->seen_direct_write = seen_direct_write;
> }
> return 0;
> }
> @@ -17433,6 +17488,8 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
> struct bpf_map *map_ptr;
> int i, ret, cnt, delta = 0;
>
> + fixup_kfunc_desc_tab(env);
> +
> for (i = 0; i < insn_cnt; i++, insn++) {
> /* Make divide-by-zero exceptions impossible. */
> if (insn->code == (BPF_ALU64 | BPF_MOD | BPF_X) ||
> @@ -17940,7 +17997,7 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
> }
> }
>
> - sort_kfunc_descs_by_imm(env->prog);
> + sort_kfunc_descs_by_imm_off(env->prog);
>
> return 0;
> }
> --
> 2.39.2
>
