| 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: <8a8fa581-94a9-649d-8c01-f1afd4bc9514@xen0n.name>
Date: Fri, 14 Oct 2022 00:42:58 +0800
From: WANG Xuerui <kernel@...0n.name>
To: Huacai Chen <chenhuacai@...ngson.cn>,
Huacai Chen <chenhuacai@...nel.org>
Cc: loongarch@...ts.linux.dev, Xuefeng Li <lixuefeng@...ngson.cn>,
Tiezhu Yang <yangtiezhu@...ngson.cn>,
Guo Ren <guoren@...nel.org>,
Jiaxun Yang <jiaxun.yang@...goat.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] LoongArch: BPF: Avoid declare variables in switch-case
On 10/13/22 23:40, Huacai Chen wrote:
> Not all compilers support declare variables in switch-case, so move
> declarations to the beginning of a function. Otherwise we may get such
> build errors:
>
> arch/loongarch/net/bpf_jit.c: In function ‘emit_atomic’:
> arch/loongarch/net/bpf_jit.c:362:3: error: a label can only be part of a statement and a declaration is not a statement
> u8 r0 = regmap[BPF_REG_0];
> ^~
> arch/loongarch/net/bpf_jit.c: In function ‘build_insn’:
> arch/loongarch/net/bpf_jit.c:727:3: error: a label can only be part of a statement and a declaration is not a statement
> u8 t7 = -1;
> ^~
> arch/loongarch/net/bpf_jit.c:778:3: error: a label can only be part of a statement and a declaration is not a statement
> int ret;
> ^~~
> arch/loongarch/net/bpf_jit.c:779:3: error: expected expression before ‘u64’
> u64 func_addr;
> ^~~
> arch/loongarch/net/bpf_jit.c:780:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
> bool func_addr_fixed;
> ^~~~
> arch/loongarch/net/bpf_jit.c:784:11: error: ‘func_addr’ undeclared (first use in this function); did you mean ‘in_addr’?
> &func_addr, &func_addr_fixed);
> ^~~~~~~~~
> in_addr
> arch/loongarch/net/bpf_jit.c:784:11: note: each undeclared identifier is reported only once for each function it appears in
> arch/loongarch/net/bpf_jit.c:814:3: error: a label can only be part of a statement and a declaration is not a statement
> u64 imm64 = (u64)(insn + 1)->imm << 32 | (u32)insn->imm;
> ^~~
>
> Signed-off-by: Huacai Chen <chenhuacai@...ngson.cn>
Fixes: 5dc615520c4d ("LoongArch: Add BPF JIT support")
> ---
> arch/loongarch/net/bpf_jit.c | 31 +++++++++++++------------------
> 1 file changed, 13 insertions(+), 18 deletions(-)
>
> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> index 43f0a98efe38..2a9b590f47e6 100644
> --- a/arch/loongarch/net/bpf_jit.c
> +++ b/arch/loongarch/net/bpf_jit.c
> @@ -279,6 +279,7 @@ static void emit_atomic(const struct bpf_insn *insn, struct jit_ctx *ctx)
> const u8 t1 = LOONGARCH_GPR_T1;
> const u8 t2 = LOONGARCH_GPR_T2;
> const u8 t3 = LOONGARCH_GPR_T3;
> + const u8 r0 = regmap[BPF_REG_0];
> const u8 src = regmap[insn->src_reg];
> const u8 dst = regmap[insn->dst_reg];
> const s16 off = insn->off;
> @@ -359,8 +360,6 @@ static void emit_atomic(const struct bpf_insn *insn, struct jit_ctx *ctx)
> break;
> /* r0 = atomic_cmpxchg(dst + off, r0, src); */
> case BPF_CMPXCHG:
> - u8 r0 = regmap[BPF_REG_0];
> -
> move_reg(ctx, t2, r0);
> if (isdw) {
> emit_insn(ctx, lld, r0, t1, 0);
> @@ -390,8 +389,11 @@ static bool is_signed_bpf_cond(u8 cond)
>
> static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool extra_pass)
> {
> - const bool is32 = BPF_CLASS(insn->code) == BPF_ALU ||
> - BPF_CLASS(insn->code) == BPF_JMP32;
> + u8 t0 = -1;
Here "t0" seems to be a versatile temp value, while the "t1" below is
the actual GPR $t1. What about renaming "t0" to something like "tmp" to
reduce confusion? I believe due to things like "t0 = LOONGARCH_GPR_ZERO"
the "t0" is 100% not an actual mapping to $t0.
> + u64 func_addr;
> + bool func_addr_fixed;
> + int i = insn - ctx->prog->insnsi;
> + int ret, jmp_offset;
> const u8 code = insn->code;
> const u8 cond = BPF_OP(code);
> const u8 t1 = LOONGARCH_GPR_T1;
> @@ -400,8 +402,8 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool ext
> const u8 dst = regmap[insn->dst_reg];
> const s16 off = insn->off;
> const s32 imm = insn->imm;
> - int jmp_offset;
> - int i = insn - ctx->prog->insnsi;
> + const u64 imm64 = (u64)(insn + 1)->imm << 32 | (u32)insn->imm;
> + const bool is32 = BPF_CLASS(insn->code) == BPF_ALU || BPF_CLASS(insn->code) == BPF_JMP32;
Please consider reducing diff damage and not touching parts not directly
affected by this change. For example this "is32" declaration and
initialization was moved although not related to this change.
>
> switch (code) {
> /* dst = src */
> @@ -724,24 +726,23 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool ext
> case BPF_JMP32 | BPF_JSGE | BPF_K:
> case BPF_JMP32 | BPF_JSLT | BPF_K:
> case BPF_JMP32 | BPF_JSLE | BPF_K:
> - u8 t7 = -1;
> jmp_offset = bpf2la_offset(i, off, ctx);
> if (imm) {
> move_imm(ctx, t1, imm, false);
> - t7 = t1;
> + t0 = t1;
> } else {
> /* If imm is 0, simply use zero register. */
> - t7 = LOONGARCH_GPR_ZERO;
> + t0 = LOONGARCH_GPR_ZERO;
> }
> move_reg(ctx, t2, dst);
> if (is_signed_bpf_cond(BPF_OP(code))) {
> - emit_sext_32(ctx, t7, is32);
> + emit_sext_32(ctx, t0, is32);
> emit_sext_32(ctx, t2, is32);
> } else {
> - emit_zext_32(ctx, t7, is32);
> + emit_zext_32(ctx, t0, is32);
> emit_zext_32(ctx, t2, is32);
> }
> - if (emit_cond_jmp(ctx, cond, t2, t7, jmp_offset) < 0)
> + if (emit_cond_jmp(ctx, cond, t2, t0, jmp_offset) < 0)
> goto toofar;
> break;
>
> @@ -775,10 +776,6 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool ext
>
> /* function call */
> case BPF_JMP | BPF_CALL:
> - int ret;
> - u64 func_addr;
> - bool func_addr_fixed;
> -
> mark_call(ctx);
> ret = bpf_jit_get_func_addr(ctx->prog, insn, extra_pass,
> &func_addr, &func_addr_fixed);
> @@ -811,8 +808,6 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool ext
>
> /* dst = imm64 */
> case BPF_LD | BPF_IMM | BPF_DW:
> - u64 imm64 = (u64)(insn + 1)->imm << 32 | (u32)insn->imm;
> -
> move_imm(ctx, dst, imm64, is32);
> return 1;
>
--
WANG "xen0n" Xuerui
Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/
Powered by blists - more mailing lists