| Message ID | xunylf0o872l.fsf@redhat.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | BPF |
| Headers | show |
| Series | PPC jit and pseudo_btf_id | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next | success | VM_Test |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| netdev/tree_selection | success | Not a local patch |
Hi Yauheni, Yauheni Kaliuta wrote: > Hi! > > I get kernel oops on my setup due to unresolved pseudo_btf_id for > ld_imm64 (see 4976b718c355 ("bpf: Introduce pseudo_btf_id")) for > example for `test_progs -t for_each/hash_map` where callback > address is passed to a bpf helper: > > > [ 425.853991] kernel tried to execute user page (100000014) - exploit attempt? (uid: 0) > [ 425.854173] BUG: Unable to handle kernel instruction fetch > [ 425.854255] Faulting instruction address: 0x100000014 > [ 425.854339] Oops: Kernel access of bad area, sig: 11 [#1] > [ 425.854423] LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries > [ 425.854529] Modules linked in: tun bpf_testmod(OE) nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 rfkill ip_set nf_tables libcrc32c nfnetlink nvram virtio_balloon vmx_crypto ext4 mbcache jbd2 sr_mod cdrom sg bochs_drm drm_vram_helper drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops cec drm_ttm_helper ttm drm virtio_net net_failover virtio_blk drm_panel_orientation_quirks failover virtio_scsi > [ 425.855276] CPU: 31 PID: 8935 Comm: test_progs Tainted: G OE --------- --- 5.14.0+ #7 > [ 425.855419] NIP: 0000000100000014 LR: c000000000385554 CTR: 0000000100000016 > [ 425.855539] REGS: c000000022e8b740 TRAP: 0400 Tainted: G OE --------- --- (5.14.0+) > [ 425.855681] MSR: 8000000040009033 <SF,EE,ME,IR,DR,RI,LE> CR: 24482844 XER: 20000000 > [ 425.855816] CFAR: c000000000385550 IRQMASK: 0 > [ 425.855816] GPR00: c000000000381b20 c000000022e8b9e0 c000000002a45f00 c0000000248fa800 > [ 425.855816] GPR04: c00000007cead8b0 c00000007cead8b8 c000000022e8ba80 0000000000000000 > [ 425.855816] GPR08: 0000000000000000 0000000000000000 c00000002269acc0 0000000000000000 > [ 425.855816] GPR12: 0000000100000016 c00000003ffca300 0000000000000000 0000000000000000 > [ 425.855816] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000001 > [ 425.855816] GPR20: 0000000000000000 0000000000000000 0000000000000000 c000000022e8bbb0 > [ 425.855816] GPR24: c000000022e8bbb4 0000000000000000 0000000000000008 c000000022e8ba80 > [ 425.855816] GPR28: c0000000248fa800 0000000100000016 c00000007cead880 0000000000000001 > [ 425.856853] NIP [0000000100000014] 0x100000014 > [ 425.856937] LR [c000000000385554] bpf_for_each_hash_elem+0x134/0x220 > [ 425.857047] Call Trace: > [ 425.857089] [c000000022e8b9e0] [8000000000000106] 0x8000000000000106 (unreliable) > [ 425.857215] [c000000022e8ba40] [c000000000381b20] bpf_for_each_map_elem+0x30/0x50 > [ 425.857340] [c000000022e8ba60] [c008000001cddb8c] bpf_prog_458e9855eab74599_F+0x68/0x24dc > [ 425.857464] [c000000022e8bad0] [c000000000c46a9c] bpf_test_run+0x2bc/0x440 > [ 425.857573] [c000000022e8bb90] [c000000000c47cbc] bpf_prog_test_run_skb+0x3fc/0x790 > [ 425.857698] [c000000022e8bc30] [c000000000363178] __sys_bpf+0xfd8/0x2690 > [ 425.857805] [c000000022e8bd90] [c0000000003648dc] sys_bpf+0x2c/0x40 > [ 425.857910] [c000000022e8bdb0] [c000000000030c9c] system_call_exception+0x15c/0x300 > [ 425.858034] [c000000022e8be10] [c00000000000c6cc] system_call_common+0xec/0x250 > [ 425.858160] --- interrupt: c00 at 0x7fff7e751ea4 Thanks for the problem report. I noticed this recently and have prepared a fix as part of a larger patchset. > > The simple patch fixes it for me: > > diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c > index 90ce75f0f1e2..554c26480387 100644 > --- a/arch/powerpc/net/bpf_jit_comp.c > +++ b/arch/powerpc/net/bpf_jit_comp.c > @@ -201,8 +201,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp) > */ > bpf_jit_fixup_subprog_calls(fp, code_base, &cgctx, addrs); > > - /* There is no need to perform the usual passes. */ > - goto skip_codegen_passes; > + /* Due to pseudo_btf_id resolving, regenerate */ > } > > /* Code generation passes 1-2 */ > @@ -222,7 +221,6 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp) > proglen - (cgctx.idx * 4), cgctx.seen); > } > > -skip_codegen_passes: > if (bpf_jit_enable > 1) > /* > * Note that we output the base address of the code_base > > > > Do I miss something? The problem with the above approach is that we generate variable number of instructions for certain BPF instructions and so, unless we reserve space for maximum number of powerpc instructions beforehand, we risk writing past the end of the allocated buffer. Can you please check if the below patch fixes the issue for you? Thanks, Naveen --- diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c index 463f99ecaa459e..e16d421ce22a65 100644 --- a/arch/powerpc/net/bpf_jit_comp.c +++ b/arch/powerpc/net/bpf_jit_comp.c @@ -66,7 +66,15 @@ static int bpf_jit_fixup_subprog_calls(struct bpf_prog *fp, u32 *image, * of the JITed sequence remains unchanged. */ ctx->idx = tmp_idx; + } else if (insn[i].code == (BPF_LD | BPF_IMM | BPF_DW)) { + func_addr = ((u64)(u32) insn[i].imm) | (((u64)(u32) insn[i+1].imm) << 32); + tmp_idx = ctx->idx; + ctx->idx = addrs[i] / 4; + PPC_LI64(b2p[insn[i].dst_reg], func_addr); + ctx->idx = tmp_idx; + i++; } + } return 0; diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c index 74b465cc7a84d0..4d3973cd78b46f 100644 --- a/arch/powerpc/net/bpf_jit_comp64.c +++ b/arch/powerpc/net/bpf_jit_comp64.c @@ -324,6 +324,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * u64 imm64; u32 true_cond; u32 tmp_idx; + int j; /* * addrs[] maps a BPF bytecode address into a real offset from @@ -858,7 +859,11 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * (((u64)(u32) insn[i+1].imm) << 32); /* Adjust for two bpf instructions */ addrs[++i] = ctx->idx * 4; + tmp_idx = ctx->idx; PPC_LI64(dst_reg, imm64); + /* padding to allow full 5 instructions for later patching */ + for (j = ctx->idx - tmp_idx; j < 5; j++) + EMIT(PPC_RAW_NOP()); break; /*
Hi! On Tue, Dec 14, 2021 at 2:11 PM Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> wrote: > > Hi Yauheni, > > > Yauheni Kaliuta wrote: > > Hi! > > > > I get kernel oops on my setup due to unresolved pseudo_btf_id for > > ld_imm64 (see 4976b718c355 ("bpf: Introduce pseudo_btf_id")) for > > example for `test_progs -t for_each/hash_map` where callback > > address is passed to a bpf helper: > > > > > > [ 425.853991] kernel tried to execute user page (100000014) - exploit attempt? (uid: 0) > > [ 425.854173] BUG: Unable to handle kernel instruction fetch > > [ 425.854255] Faulting instruction address: 0x100000014 [...] > > Thanks for the problem report. I noticed this recently and have prepared > a fix as part of a larger patchset. > Ah, cool! That was actually my main question, do I miss the fix somewhere :) > > > > The simple patch fixes it for me: > > > > diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c > > index 90ce75f0f1e2..554c26480387 100644 > > --- a/arch/powerpc/net/bpf_jit_comp.c > > +++ b/arch/powerpc/net/bpf_jit_comp.c > > @@ -201,8 +201,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp) > > */ > > bpf_jit_fixup_subprog_calls(fp, code_base, &cgctx, addrs); > > > > - /* There is no need to perform the usual passes. */ > > - goto skip_codegen_passes; > > + /* Due to pseudo_btf_id resolving, regenerate */ > > } > > > > /* Code generation passes 1-2 */ > > @@ -222,7 +221,6 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp) > > proglen - (cgctx.idx * 4), cgctx.seen); > > } > > > > -skip_codegen_passes: > > if (bpf_jit_enable > 1) > > /* > > * Note that we output the base address of the code_base > > > > > > > > Do I miss something? > > The problem with the above approach is that we generate variable number > of instructions for certain BPF instructions and so, unless we reserve > space for maximum number of powerpc instructions beforehand, we risk > writing past the end of the allocated buffer. Yes, agree. > Can you please check if the below patch fixes the issue for you? It does, thanks! I was actually thinking later about something similar and I wonder about naming. Should the function be renamed to more generic, and is it really for func_addr only or can be any generic value? > > > Thanks, > Naveen > > > --- > diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c > index 463f99ecaa459e..e16d421ce22a65 100644 > --- a/arch/powerpc/net/bpf_jit_comp.c > +++ b/arch/powerpc/net/bpf_jit_comp.c > @@ -66,7 +66,15 @@ static int bpf_jit_fixup_subprog_calls(struct bpf_prog *fp, u32 *image, > * of the JITed sequence remains unchanged. > */ > ctx->idx = tmp_idx; > + } else if (insn[i].code == (BPF_LD | BPF_IMM | BPF_DW)) { > + func_addr = ((u64)(u32) insn[i].imm) | (((u64)(u32) insn[i+1].imm) << 32); > + tmp_idx = ctx->idx; > + ctx->idx = addrs[i] / 4; > + PPC_LI64(b2p[insn[i].dst_reg], func_addr); > + ctx->idx = tmp_idx; > + i++; > } > + > } > > return 0; > diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c > index 74b465cc7a84d0..4d3973cd78b46f 100644 > --- a/arch/powerpc/net/bpf_jit_comp64.c > +++ b/arch/powerpc/net/bpf_jit_comp64.c > @@ -324,6 +324,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * > u64 imm64; > u32 true_cond; > u32 tmp_idx; > + int j; > > /* > * addrs[] maps a BPF bytecode address into a real offset from > @@ -858,7 +859,11 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * > (((u64)(u32) insn[i+1].imm) << 32); > /* Adjust for two bpf instructions */ > addrs[++i] = ctx->idx * 4; > + tmp_idx = ctx->idx; > PPC_LI64(dst_reg, imm64); > + /* padding to allow full 5 instructions for later patching */ > + for (j = ctx->idx - tmp_idx; j < 5; j++) > + EMIT(PPC_RAW_NOP()); > break; > > /* >
Yauheni Kaliuta wrote: >> Can you please check if the below patch fixes the issue for you? > > It does, thanks! > > I was actually thinking later about something similar and I wonder > about naming. Should the function be renamed to more generic, and is > it really for func_addr only or can be any generic value? Good point. Looking at jit_subprogs(), it looks like the extra pass fixes up addresses of subprog calls, as well as that of other bpf functions. So, I agree that it makes sense to change the function name. func_addr looks to still be correct though. Thanks for testing this. I will update this patch and post it along with a few other fixes. Regards, Naveen
diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c index 90ce75f0f1e2..554c26480387 100644 --- a/arch/powerpc/net/bpf_jit_comp.c +++ b/arch/powerpc/net/bpf_jit_comp.c @@ -201,8 +201,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp) */ bpf_jit_fixup_subprog_calls(fp, code_base, &cgctx, addrs); - /* There is no need to perform the usual passes. */ - goto skip_codegen_passes; + /* Due to pseudo_btf_id resolving, regenerate */ } /* Code generation passes 1-2 */ @@ -222,7 +221,6 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp) proglen - (cgctx.idx * 4), cgctx.seen); } -skip_codegen_passes: if (bpf_jit_enable > 1) /* * Note that we output the base address of the code_base
Hi! I get kernel oops on my setup due to unresolved pseudo_btf_id for ld_imm64 (see 4976b718c355 ("bpf: Introduce pseudo_btf_id")) for example for `test_progs -t for_each/hash_map` where callback address is passed to a bpf helper: [ 425.853991] kernel tried to execute user page (100000014) - exploit attempt? (uid: 0) [ 425.854173] BUG: Unable to handle kernel instruction fetch [ 425.854255] Faulting instruction address: 0x100000014 [ 425.854339] Oops: Kernel access of bad area, sig: 11 [#1] [ 425.854423] LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries [ 425.854529] Modules linked in: tun bpf_testmod(OE) nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 rfkill ip_set nf_tables libcrc32c nfnetlink nvram virtio_balloon vmx_crypto ext4 mbcache jbd2 sr_mod cdrom sg bochs_drm drm_vram_helper drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops cec drm_ttm_helper ttm drm virtio_net net_failover virtio_blk drm_panel_orientation_quirks failover virtio_scsi [ 425.855276] CPU: 31 PID: 8935 Comm: test_progs Tainted: G OE --------- --- 5.14.0+ #7 [ 425.855419] NIP: 0000000100000014 LR: c000000000385554 CTR: 0000000100000016 [ 425.855539] REGS: c000000022e8b740 TRAP: 0400 Tainted: G OE --------- --- (5.14.0+) [ 425.855681] MSR: 8000000040009033 <SF,EE,ME,IR,DR,RI,LE> CR: 24482844 XER: 20000000 [ 425.855816] CFAR: c000000000385550 IRQMASK: 0 [ 425.855816] GPR00: c000000000381b20 c000000022e8b9e0 c000000002a45f00 c0000000248fa800 [ 425.855816] GPR04: c00000007cead8b0 c00000007cead8b8 c000000022e8ba80 0000000000000000 [ 425.855816] GPR08: 0000000000000000 0000000000000000 c00000002269acc0 0000000000000000 [ 425.855816] GPR12: 0000000100000016 c00000003ffca300 0000000000000000 0000000000000000 [ 425.855816] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000001 [ 425.855816] GPR20: 0000000000000000 0000000000000000 0000000000000000 c000000022e8bbb0 [ 425.855816] GPR24: c000000022e8bbb4 0000000000000000 0000000000000008 c000000022e8ba80 [ 425.855816] GPR28: c0000000248fa800 0000000100000016 c00000007cead880 0000000000000001 [ 425.856853] NIP [0000000100000014] 0x100000014 [ 425.856937] LR [c000000000385554] bpf_for_each_hash_elem+0x134/0x220 [ 425.857047] Call Trace: [ 425.857089] [c000000022e8b9e0] [8000000000000106] 0x8000000000000106 (unreliable) [ 425.857215] [c000000022e8ba40] [c000000000381b20] bpf_for_each_map_elem+0x30/0x50 [ 425.857340] [c000000022e8ba60] [c008000001cddb8c] bpf_prog_458e9855eab74599_F+0x68/0x24dc [ 425.857464] [c000000022e8bad0] [c000000000c46a9c] bpf_test_run+0x2bc/0x440 [ 425.857573] [c000000022e8bb90] [c000000000c47cbc] bpf_prog_test_run_skb+0x3fc/0x790 [ 425.857698] [c000000022e8bc30] [c000000000363178] __sys_bpf+0xfd8/0x2690 [ 425.857805] [c000000022e8bd90] [c0000000003648dc] sys_bpf+0x2c/0x40 [ 425.857910] [c000000022e8bdb0] [c000000000030c9c] system_call_exception+0x15c/0x300 [ 425.858034] [c000000022e8be10] [c00000000000c6cc] system_call_common+0xec/0x250 [ 425.858160] --- interrupt: c00 at 0x7fff7e751ea4 The simple patch fixes it for me: Do I miss something? Thanks!