| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <240d153b-8893-ae7c-03ea-bdaf1ae3c696@csgroup.eu>
Date: Tue, 21 Sep 2021 16:02:46 +0200
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Hari Bathini <hbathini@...ux.ibm.com>, naveen.n.rao@...ux.ibm.com,
mpe@...erman.id.au, ast@...nel.org, daniel@...earbox.net
Cc: paulus@...ba.org, andrii@...nel.org, kafai@...com,
songliubraving@...com, yhs@...com, john.fastabend@...il.com,
kpsingh@...nel.org, netdev@...r.kernel.org, bpf@...r.kernel.org,
linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v3 3/8] bpf powerpc: refactor JIT compiler code
Le 21/09/2021 à 15:29, Hari Bathini a écrit :
> Refactor powerpc LDX JITing code to simplify adding BPF_PROBE_MEM
> support.
>
> Signed-off-by: Hari Bathini <hbathini@...ux.ibm.com>
> ---
>
> Changes in v3:
> * Dropped ST(X) JITing coderefactoring and ensured the changes are
> minimal and relevant to BPF_PROBE_MEM support.
> * Added a default for size switch case to ensure compiler would
> not complain.
>
>
> arch/powerpc/net/bpf_jit_comp32.c | 42 ++++++++++++++++++++-----------
> arch/powerpc/net/bpf_jit_comp64.c | 40 +++++++++++++++++++----------
> 2 files changed, 55 insertions(+), 27 deletions(-)
>
> diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c
> index b60b59426a24..6e4956cd52e7 100644
> --- a/arch/powerpc/net/bpf_jit_comp32.c
> +++ b/arch/powerpc/net/bpf_jit_comp32.c
> @@ -282,6 +282,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
> u32 src_reg = bpf_to_ppc(ctx, insn[i].src_reg);
> u32 src_reg_h = src_reg - 1;
> u32 tmp_reg = bpf_to_ppc(ctx, TMP_REG);
> + u32 size = BPF_SIZE(code);
> s16 off = insn[i].off;
> s32 imm = insn[i].imm;
> bool func_addr_fixed;
> @@ -810,23 +811,36 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
> * BPF_LDX
> */
> case BPF_LDX | BPF_MEM | BPF_B: /* dst = *(u8 *)(ul) (src + off) */
> - EMIT(PPC_RAW_LBZ(dst_reg, src_reg, off));
> - if (!fp->aux->verifier_zext)
> - EMIT(PPC_RAW_LI(dst_reg_h, 0));
> - break;
> + fallthrough;
> case BPF_LDX | BPF_MEM | BPF_H: /* dst = *(u16 *)(ul) (src + off) */
> - EMIT(PPC_RAW_LHZ(dst_reg, src_reg, off));
> - if (!fp->aux->verifier_zext)
> - EMIT(PPC_RAW_LI(dst_reg_h, 0));
> - break;
> + fallthrough;
> case BPF_LDX | BPF_MEM | BPF_W: /* dst = *(u32 *)(ul) (src + off) */
> - EMIT(PPC_RAW_LWZ(dst_reg, src_reg, off));
> - if (!fp->aux->verifier_zext)
> - EMIT(PPC_RAW_LI(dst_reg_h, 0));
> - break;
> + fallthrough;
> case BPF_LDX | BPF_MEM | BPF_DW: /* dst = *(u64 *)(ul) (src + off) */
> - EMIT(PPC_RAW_LWZ(dst_reg_h, src_reg, off));
> - EMIT(PPC_RAW_LWZ(dst_reg, src_reg, off + 4));
> + switch (size) {
> + case BPF_B:
> + EMIT(PPC_RAW_LBZ(dst_reg, src_reg, off));
> + break;
> + case BPF_H:
> + EMIT(PPC_RAW_LHZ(dst_reg, src_reg, off));
> + break;
> + case BPF_W:
> + EMIT(PPC_RAW_LWZ(dst_reg, src_reg, off));
> + break;
> + case BPF_DW:
> + EMIT(PPC_RAW_LWZ(dst_reg_h, src_reg, off));
> + EMIT(PPC_RAW_LWZ(dst_reg, src_reg, off + 4));
> + break;
> + /*
> + * With size not being an enum and BPF_B/H/W/DW being macros, ensure no
> + * compiler warning/error by adding a default case that never reaches.
> + */
Thinking about it once more, in fact this is already bounded by the
upper switch/case, so there is no possibility to end up here with
something else than the 4 cases and that's probably the reason why GCC
says nothing about it, so I now think that a default is unnecessary and
should not be added. Sorry for that.
> + default:
> + break;
> + }
> +
> + if (size != BPF_DW && !fp->aux->verifier_zext)
> + EMIT(PPC_RAW_LI(dst_reg_h, 0));
> break;
>
> /*
> diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
> index 2a87da50d9a4..991eb43d4cd2 100644
> --- a/arch/powerpc/net/bpf_jit_comp64.c
> +++ b/arch/powerpc/net/bpf_jit_comp64.c
> @@ -285,6 +285,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
> u32 code = insn[i].code;
> u32 dst_reg = b2p[insn[i].dst_reg];
> u32 src_reg = b2p[insn[i].src_reg];
> + u32 size = BPF_SIZE(code);
> s16 off = insn[i].off;
> s32 imm = insn[i].imm;
> bool func_addr_fixed;
> @@ -716,25 +717,38 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
> */
> /* dst = *(u8 *)(ul) (src + off) */
> case BPF_LDX | BPF_MEM | BPF_B:
> - EMIT(PPC_RAW_LBZ(dst_reg, src_reg, off));
> - if (insn_is_zext(&insn[i + 1]))
> - addrs[++i] = ctx->idx * 4;
> - break;
> + fallthrough;
> /* dst = *(u16 *)(ul) (src + off) */
> case BPF_LDX | BPF_MEM | BPF_H:
> - EMIT(PPC_RAW_LHZ(dst_reg, src_reg, off));
> - if (insn_is_zext(&insn[i + 1]))
> - addrs[++i] = ctx->idx * 4;
> - break;
> + fallthrough;
> /* dst = *(u32 *)(ul) (src + off) */
> case BPF_LDX | BPF_MEM | BPF_W:
> - EMIT(PPC_RAW_LWZ(dst_reg, src_reg, off));
> - if (insn_is_zext(&insn[i + 1]))
> - addrs[++i] = ctx->idx * 4;
> - break;
> + fallthrough;
> /* dst = *(u64 *)(ul) (src + off) */
> case BPF_LDX | BPF_MEM | BPF_DW:
> - PPC_BPF_LL(dst_reg, src_reg, off);
> + switch (size) {
> + case BPF_B:
> + EMIT(PPC_RAW_LBZ(dst_reg, src_reg, off));
> + break;
> + case BPF_H:
> + EMIT(PPC_RAW_LHZ(dst_reg, src_reg, off));
> + break;
> + case BPF_W:
> + EMIT(PPC_RAW_LWZ(dst_reg, src_reg, off));
> + break;
> + case BPF_DW:
> + PPC_BPF_LL(dst_reg, src_reg, off);
> + break;
> + /*
> + * With size not being an enum and BPF_B/H/W/DW being macros, ensure no
> + * compiler warning/error by adding a default case that never reaches.
> + */
Same
> + default:
> + break;
> + }
> +
> + if (size != BPF_DW && insn_is_zext(&insn[i + 1]))
> + addrs[++i] = ctx->idx * 4;
> break;
>
> /*
>
Powered by blists - more mailing lists