Re: [PATCH bpf-next v2 3/3] bpf, arm64: use bpf_jit_binary_pack_alloc

Song Liu
2023-06-08T16:28:58Z

On Wed, Jun 7, 2023 at 2:18 AM Puranjay Mohan <puranjay12@gmail.com> wrote:
>
[...]
> +
> static inline int epilogue_offset(const struct jit_ctx *ctx)
> {
> int to = ctx->epilogue_offset;
> @@ -701,7 +716,8 @@ static int add_exception_handler(const struct bpf_insn *insn,
> struct jit_ctx *ctx,
> int dst_reg)
> {
> - off_t offset;
> + off_t ins_offset;
> + off_t fixup_offset;

Please add some comments for these two offsets.

> unsigned long pc;
> struct exception_table_entry *ex;
>
> @@ -717,12 +733,11 @@ static int add_exception_handler(const struct bpf_insn *insn,
> return -EINVAL;
>
> ex = &ctx->prog->aux->extable[ctx->exentry_idx];
> - pc = (unsigned long)&ctx->image[ctx->idx - 1];
> + pc = (unsigned long)&ctx->ro_image[ctx->idx - 1];
>
> - offset = pc - (long)&ex->insn;
> - if (WARN_ON_ONCE(offset >= 0 || offset < INT_MIN))
> + ins_offset = pc - (long)&ex->insn;
> + if (WARN_ON_ONCE(ins_offset >= 0 || ins_offset < INT_MIN))
> return -ERANGE;
> - ex->insn = offset;
>
> /*
> * Since the extable follows the program, the fixup offset is always
> @@ -732,11 +747,20 @@ static int add_exception_handler(const struct bpf_insn *insn,
> * modifying the upper bits because the table is already sorted, and
> * isn't part of the main exception table.
> */
> - offset = (long)&ex->fixup - (pc + AARCH64_INSN_SIZE);
> - if (!FIELD_FIT(BPF_FIXUP_OFFSET_MASK, offset))
> + fixup_offset = (long)&ex->fixup - (pc + AARCH64_INSN_SIZE);
> + if (!FIELD_FIT(BPF_FIXUP_OFFSET_MASK, fixup_offset))
> return -ERANGE;
>
> - ex->fixup = FIELD_PREP(BPF_FIXUP_OFFSET_MASK, offset) |
> + /*
> + * The offsets above have been calculated using the RO buffer but we
> + * need to use the R/W buffer for writes.
> + * switch ex to rw buffer for writing.
> + */
> + ex = (void *)ctx->image + ((void *)ex - (void *)ctx->ro_image);
> +
> + ex->insn = ins_offset;
> +
> + ex->fixup = FIELD_PREP(BPF_FIXUP_OFFSET_MASK, fixup_offset) |
> FIELD_PREP(BPF_FIXUP_REG_MASK, dst_reg);
>
> ex->type = EX_TYPE_BPF;
[...]
> /* And we're done. */
> if (bpf_jit_enable > 1)
> bpf_jit_dump(prog->len, prog_size, 2, ctx.image);
>
> - bpf_flush_icache(header, ctx.image + ctx.idx);
> + bpf_flush_icache(ro_header, ctx.ro_image + ctx.idx);
>
> if (!prog->is_func || extra_pass) {
> if (extra_pass && ctx.idx != jit_data->ctx.idx) {
> pr_err_once("multi-func JIT bug %d != %d\n",
> ctx.idx, jit_data->ctx.idx);
> - bpf_jit_binary_free(header);
> prog->bpf_func = NULL;
> prog->jited = 0;
> prog->jited_len = 0;
> + goto out_free_hdr;
> + }
> + if (WARN_ON(bpf_jit_binary_pack_finalize(prog, ro_header,
> + header))) {
> + ro_header = NULL;

I think we need
prog = orig_prog;
here.

Thanks,
Song