On Tue, Jan 23, 2024 at 2:32 AM Pu Lehui <pulehui@xxxxxxxxxxxxxxx> wrote:
>
> From: Pu Lehui <pulehui@xxxxxxxxxx>
>
> In __arch_prepare_bpf_trampoline, we emit instructions to store the
> address of im to register and then pass it to __bpf_tramp_enter and
> __bpf_tramp_exit functions. Currently we use fake im in
> arch_bpf_trampoline_size for the dry run, and then allocate new im for
> the real patching. This is fine for architectures that use fixed
> instructions to generate addresses. However, for architectures that use
> dynamic instructions to generate addresses, this may make the front and
> rear images inconsistent, leading to patching overflow. We can extract
> the im allocation ahead of the dry run and pass the allocated im to
> arch_bpf_trampoline_size, so that we can ensure that im is consistent in
> dry run and real patching.
IIUC, this is required because emit_imm() for riscv may generate variable
size instructions (depends on the value of im). I wonder we can fix this by
simply set a special value for fake im in arch_bpf_trampoline_size() to
so that emit_imm() always gives biggest value for the fake im.
>
> Signed-off-by: Pu Lehui <pulehui@xxxxxxxxxx>
> ---
[...]
>
> static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mutex)
> @@ -432,23 +425,27 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
> tr->flags |= BPF_TRAMP_F_ORIG_STACK;
> #endif
>
> - size = arch_bpf_trampoline_size(&tr->func.model, tr->flags,
> + im = kzalloc(sizeof(*im), GFP_KERNEL);
> + if (!im) {
> + err = -ENOMEM;
> + goto out;
> + }
> +
> + size = arch_bpf_trampoline_size(im, &tr->func.model, tr->flags,
> tlinks, tr->func.addr);
> if (size < 0) {
> err = size;
> - goto out;
> + goto out_free_im;
> }
>
> if (size > PAGE_SIZE) {
> err = -E2BIG;
> - goto out;
> + goto out_free_im;
> }
>
> - im = bpf_tramp_image_alloc(tr->key, size);
> - if (IS_ERR(im)) {
> - err = PTR_ERR(im);
> - goto out;
> - }
> + err = bpf_tramp_image_alloc(im, tr->key, size);
> + if (err < 0)
> + goto out_free_im;
I feel this change just makes bpf_trampoline_update() even
more confusing.
>
> err = arch_prepare_bpf_trampoline(im, im->image, im->image + size,
> &tr->func.model, tr->flags, tlinks,
> @@ -496,6 +493,8 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
>
> out_free:
> bpf_tramp_image_free(im);
> +out_free_im:
> + kfree_rcu(im, rcu);
If we goto out_free above, we will call kfree_rcu(im, rcu)
twice, right? Once in bpf_tramp_image_free(), and again
here.
Thanks,
Song
[...]
