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

[Puranjay Mohan]

Hi Song,

On Thu, Jun 8, 2023 at 6:28 PM Song Liu <song@xxxxxxxxxx> wrote:
>
> On Wed, Jun 7, 2023 at 2:18 AM Puranjay Mohan <puranjay12@xxxxxxxxx> 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.

Here I am using two variables because I need to change from the RO
buffer for calculating offsets
to the RW buffer for writing.

Earlier, a single variable could work because it was being reused for
calculating the second offset
after writing the first one. Here, I can't re-calculate using the same
variable because I have to change
to the RW buffer, and using the same variable would need changing back
to the RO buffer.

So, I am calculating both offsets first, changing to RW buffer and
writing both offsets.

But I will add comments explaining what these offsets are being used for.

>
> >         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.

I agree, this is a mistake from my side.
I will add this in the next version.


Thanks,
Puranjay Mohan