On 2024/1/30 1:58, Song Liu wrote:
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.
Hi Song,
Thanks for your review. Yes, I had the same idea as you at first, emit
biggist count instructions when ctx->insns is NULL, but this may lead to
memory waste. So try moving out of IM to get a fixed IM address, maybe
other architectures require it too. If you feel it is inappropriate, I
will withdraw it.
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.
Oops, sorry, forgot to remove kfree_rcu in bpf_tramp_image_free in this
version.
Thanks,
Song
[...]
