Re: [PATCH v6 bpf-next 6/7] bpf: introduce bpf_prog_pack allocator

[Alexei Starovoitov]

On Fri, Jan 21, 2022 at 4:23 PM Song Liu <songliubraving@xxxxxx> wrote:
>
>
>
> > On Jan 21, 2022, at 3:55 PM, Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Fri, Jan 21, 2022 at 11:49 AM Song Liu <song@xxxxxxxxxx> wrote:
> >>
> >> +static struct bpf_binary_header *
> >> +__bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr,
> >> +                      unsigned int alignment,
> >> +                      bpf_jit_fill_hole_t bpf_fill_ill_insns,
> >> +                      u32 round_up_to)
> >> +{
> >> +       struct bpf_binary_header *hdr;
> >> +       u32 size, hole, start;
> >> +
> >> +       WARN_ON_ONCE(!is_power_of_2(alignment) ||
> >> +                    alignment > BPF_IMAGE_ALIGNMENT);
> >> +
> >> +       /* Most of BPF filters are really small, but if some of them
> >> +        * fill a page, allow at least 128 extra bytes to insert a
> >> +        * random section of illegal instructions.
> >> +        */
> >> +       size = round_up(proglen + sizeof(*hdr) + 128, round_up_to);
> >> +
> >> +       if (bpf_jit_charge_modmem(size))
> >> +               return NULL;
> >> +       hdr = bpf_jit_alloc_exec(size);
> >> +       if (!hdr) {
> >> +               bpf_jit_uncharge_modmem(size);
> >> +               return NULL;
> >> +       }
> >> +
> >> +       /* Fill space with illegal/arch-dep instructions. */
> >> +       bpf_fill_ill_insns(hdr, size);
> >> +
> >> +       hdr->size = size;
> >> +       hole = min_t(unsigned int, size - (proglen + sizeof(*hdr)),
> >> +                    PAGE_SIZE - sizeof(*hdr));
> >
> > It probably should be 'round_up_to' instead of PAGE_SIZE ?
>
> Actually, some of these change is not longer needed after the following
> change in v6:
>
>   4. Change fall back round_up_to in bpf_jit_binary_alloc_pack() from
>      BPF_PROG_MAX_PACK_PROG_SIZE to PAGE_SIZE.
>
> My initial thought (last year) was if we allocate more than 2MB (either
> 2.1MB or 3.9MB), we round up to 4MB to save page table entries.
> However, when I revisited this earlier today, I thought we should still
> round up to PAGE_SIZE to save memory
>
> Right now, I am not sure which way is better. What do you think? If we
> round up to PAGE_SIZE, we don't need split out __bpf_jit_binary_alloc().

The less code duplication the better.

> >
> >> +       start = (get_random_int() % hole) & ~(alignment - 1);
> >> +
> >> +       /* Leave a random number of instructions before BPF code. */
> >> +       *image_ptr = &hdr->image[start];
> >> +
> >> +       return hdr;
> >> +}
> >> +
> >> struct bpf_binary_header *
> >> bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr,
> >>                     unsigned int alignment,
> >>                     bpf_jit_fill_hole_t bpf_fill_ill_insns)
> >> +{
> >> +       return __bpf_jit_binary_alloc(proglen, image_ptr, alignment,
> >> +                                     bpf_fill_ill_insns, PAGE_SIZE);
> >> +}
> >> +
> >> +struct bpf_binary_header *
> >> +bpf_jit_binary_alloc_pack(unsigned int proglen, u8 **image_ptr,
> >> +                         unsigned int alignment,
> >> +                         bpf_jit_fill_hole_t bpf_fill_ill_insns)
> >> {
> >>        struct bpf_binary_header *hdr;
> >>        u32 size, hole, start;
> >> @@ -875,11 +1034,16 @@ bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr,
> >>         * fill a page, allow at least 128 extra bytes to insert a
> >>         * random section of illegal instructions.
> >>         */
> >> -       size = round_up(proglen + sizeof(*hdr) + 128, PAGE_SIZE);
> >> +       size = round_up(proglen + sizeof(*hdr) + 128, BPF_PROG_CHUNK_SIZE);
> >> +
> >> +       /* for too big program, use __bpf_jit_binary_alloc. */
> >> +       if (size > BPF_PROG_MAX_PACK_PROG_SIZE)
> >> +               return __bpf_jit_binary_alloc(proglen, image_ptr, alignment,
> >> +                                             bpf_fill_ill_insns, PAGE_SIZE);
> >>
> >>        if (bpf_jit_charge_modmem(size))
> >>                return NULL;
> >> -       hdr = bpf_jit_alloc_exec(size);
> >> +       hdr = bpf_prog_pack_alloc(size);
> >>        if (!hdr) {
> >>                bpf_jit_uncharge_modmem(size);
> >>                return NULL;
> >> @@ -888,9 +1052,8 @@ bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr,
> >>        /* Fill space with illegal/arch-dep instructions. */
> >>        bpf_fill_ill_insns(hdr, size);
> >>
> >> -       hdr->size = size;
> >
> > I'm missing where it's assigned.
> > Looks like hdr->size stays zero, so free is never performed?
>
> This is read only memory, so we set it in bpf_fill_ill_insns(). There was a
> comment in x86/bpf_jit_comp.c. I guess we also need a comment here.

Ahh. Found it. Pls don't do it in fill_insn.
It's the wrong layering.
It feels that callbacks need to be redesigned.
I would operate on rw_header here and use
existing arch specific callback fill_insn to write into rw_image.
Both insns during JITing and 0xcc on both sides of the prog.
Populate rw_header->size (either before or after JITing)
and then do single text_poke_copy to write the whole thing
into the correct spot.