Re: [PATCH bpf] bpf: invalidate unused part of bpf_prog_pack

[Linus Torvalds <torvalds@linux-foundation.org>]

On Thu, Apr 21, 2022 at 12:27 AM Song Liu <song@kernel.org> wrote:
>
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -228,6 +228,28 @@ static void jit_fill_hole(void *area, unsigned int size)
>         memset(area, 0xcc, size);
>  }
>
> +#define INVALID_BUF_SIZE PAGE_SIZE
> +static char invalid_insn_buf[INVALID_BUF_SIZE];
> +
> +static int __init bpf_init_invalid_insn_buf(void)
> +{
> +       jit_fill_hole(invalid_insn_buf, INVALID_BUF_SIZE);
> +       return 0;
> +}
> +pure_initcall(bpf_init_invalid_insn_buf);
> +
> +void bpf_arch_invalidate_text(void *dst, size_t len)
> +{
> +       size_t i = 0;
> +
> +       while (i < len) {
> +               size_t s = min_t(size_t, len - i, INVALID_BUF_SIZE);
> +
> +               bpf_arch_text_copy(dst + i, invalid_insn_buf, s);
> +               i += s;
> +       }
> +}

Why do we need this new infrastructure?

Why bpf_arch_invalidate_text()?

Why not jit_fill_hole() unconditionally?

It seems a bit pointless to have page buffer for containing this data,
when we already have a (trivial) function to fill an area with invalid
instructions.

On x86, it's literally just "memset(0xcc)" (ie all 'int3' instructions).

And on most RISC architectures, it would be some variation of
"memset32(TRAP_INSN)".

And all bpf targets should already have that nicely as that
jit_fill_hole() function, no?

The pack-allocator bpf code already *does* that, and is already passed
that function.

But it's just that it does it too late. Instead of doing it when
allocating a new pack, it does it in the sub-allocator.

Afaik the code in bpf/core.c already has all the information it needs,
and already has that jit_fill_hole() function pointer, but is applying
it at the wrong point.

So I think the fix should be to just pass in that 'bpf_fill_ill_insns'
function pointer all the way to alloc_new_pack(), instead of using it
in bpf_jit_binary_alloc().

NOTE! Once again, I'm not actually all that familiar with the code.
Maybe I'm missing something.

             Linus