Re: [PATCH RFC] bpf: Fix trampoline for functions with variable arguments

From
Andrii Nakryiko <hidden>
Date
2021-05-03 22:32:49
Also in
netdev
Reply to
Jiri Olsa, 2021-05-02
On Sun, May 2, 2021 at 2:17 PM Jiri Olsa [off-list ref] wrote:
On Thu, Apr 29, 2021 at 11:28:34PM +0200, Jiri Olsa wrote:
quoted
For functions with variable arguments like:

  void set_worker_desc(const char *fmt, ...)

the BTF data contains void argument at the end:

[4061] FUNC_PROTO '(anon)' ret_type_id=0 vlen=2
        'fmt' type_id=3
        '(anon)' type_id=0

When attaching function with this void argument the btf_distill_func_proto
will set last btf_func_model's argument with size 0 and that
will cause extra loop in save_regs/restore_regs functions and
generate trampoline code like:

  55             push   %rbp
  48 89 e5       mov    %rsp,%rbp
  48 83 ec 10    sub    $0x10,%rsp
  53             push   %rbx
  48 89 7d f0    mov    %rdi,-0x10(%rbp)
  75 f8          jne    0xffffffffa00cf007
                 ^^^ extra jump

It's causing soft lockups/crashes probably depends on what context
is the attached function called, like for set_worker_desc:

  watchdog: BUG: soft lockup - CPU#16 stuck for 22s! [kworker/u40:4:239]
  CPU: 16 PID: 239 Comm: kworker/u40:4 Not tainted 5.12.0-rc4qemu+ #178
  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.14.0-1.fc33 04/01/2014
  Workqueue: writeback wb_workfn
  RIP: 0010:bpf_trampoline_6442464853_0+0xa/0x1000
  Code: Unable to access opcode bytes at RIP 0xffffffffa3597fe0.
  RSP: 0018:ffffc90000687da8 EFLAGS: 00000217
  Call Trace:
   set_worker_desc+0x5/0xb0
   wb_workfn+0x48/0x4d0
   ? psi_group_change+0x41/0x210
   ? __bpf_prog_exit+0x15/0x20
   ? bpf_trampoline_6442458903_0+0x3b/0x1000
   ? update_pasid+0x5/0x90
   ? __switch_to+0x187/0x450
   process_one_work+0x1e7/0x380
   worker_thread+0x50/0x3b0
   ? rescuer_thread+0x380/0x380
   kthread+0x11b/0x140
   ? __kthread_bind_mask+0x60/0x60
   ret_from_fork+0x22/0x30

This patch is removing the void argument from struct btf_func_model
in btf_distill_func_proto, but perhaps we should also check for this
in JIT's save_regs/restore_regs functions.
actualy looks like we need to disable functions with variable arguments
completely, because we don't know how many arguments to save

I tried to disable them in pahole and it's easy fix, will post new fix
Can we still allow access to fixed arguments for such functions and
just disallow the vararg ones?
jirka
Thread (7 messages) 7 messages, 4 authors, last reply 2021-05-05
Keyboard shortcuts
hback out one level
jnext message in thread / next series-diff position
kprevious message in thread / previous series-diff position
ldrill in
Escclose help / fold thread tree
?toggle this help