On 9/13/24 12:44 PM, Alexei Starovoitov wrote:
On Fri, Sep 13, 2024 at 8:03 AM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
+ BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
+ BPF_OP(BPF_ADD) | BPF_K, BPF_REG_AX,
+ 0, 0, 1),
+ BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
+ BPF_JGT | BPF_K, BPF_REG_AX,
+ 0, 4, 1),
+ BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
+ BPF_JEQ | BPF_K, BPF_REG_AX,
+ 0, 1, 0),
+ BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
+ BPF_OP(BPF_MOV) | BPF_K, insn->dst_reg,
+ 0, 0, 0),
+ /* BPF_NEG(LLONG_MIN) == -LLONG_MIN == LLONG_MIN */
+ BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
+ BPF_OP(BPF_NEG) | BPF_K, insn->dst_reg,
lgtm, but all of BPF_OP(..) are confusing.
What's the point?
We use BPF_OP(insn->code) to reuse the code when we create a new opcode,
but BPF_OP(BPF_NEG) == BPF_NEG and BPF_OP(BPF_MOV) == BPF_MOV, so why?
Sorry, I focused on the algorithm and missed this one. Yes, changing
BPF_OP(BPF_NEG) to BPF_NEG and other similar cases are correct.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 69b8d91f5136..068f763dcefb 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -20510,7 +20510,7 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
struct bpf_insn *patchlet;
struct bpf_insn chk_and_sdiv[] = {
BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
- BPF_OP(BPF_NEG) | BPF_K, insn->dst_reg,
+ BPF_NEG | BPF_K, insn->dst_reg,
0, 0, 0),
};
struct bpf_insn chk_and_smod[] = {
@@ -20565,7 +20565,7 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
*/
BPF_MOV64_REG(BPF_REG_AX, insn->src_reg),
BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
- BPF_OP(BPF_ADD) | BPF_K, BPF_REG_AX,
+ BPF_ADD | BPF_K, BPF_REG_AX,
0, 0, 1),
BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
BPF_JGT | BPF_K, BPF_REG_AX,
@@ -20574,11 +20574,11 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
BPF_JEQ | BPF_K, BPF_REG_AX,
0, 1, 0),
BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
- BPF_OP(BPF_MOV) | BPF_K, insn->dst_reg,
+ BPF_MOV | BPF_K, insn->dst_reg,
0, 0, 0),
/* BPF_NEG(LLONG_MIN) == -LLONG_MIN == LLONG_MIN */
BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
- BPF_OP(BPF_NEG) | BPF_K, insn->dst_reg,
+ BPF_NEG | BPF_K, insn->dst_reg,
0, 0, 0),
BPF_JMP_IMM(BPF_JA, 0, 0, 1),
*insn,
@@ -20588,7 +20588,7 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
/* [R,W]x mod -1 -> 0 */
BPF_MOV64_REG(BPF_REG_AX, insn->src_reg),
BPF_RAW_INSN((is64 ? BPF_ALU64 : BPF_ALU) |
- BPF_OP(BPF_ADD) | BPF_K, BPF_REG_AX,
+ BPF_ADD | BPF_K, BPF_REG_AX,
0, 0, 1),
BPF_RAW_INSN((is64 ? BPF_JMP : BPF_JMP32) |
If I'm not missing anything I can remove these BPF_OP wrapping when applying.
wdyt?
Yes, pelase do. Thanks!
