On Thu, Apr 21, 2022 at 12:27 AM Song Liu <song@xxxxxxxxxx> 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
