* [PATCH 0/3] bpf: simple tests and fixes
@ 2025-11-10 15:30 Marat Khalili
2025-11-10 15:30 ` [PATCH 1/3] bpf: fix signed shift overflows in ARM JIT Marat Khalili
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Marat Khalili @ 2025-11-10 15:30 UTC (permalink / raw)
To: Konstantin Ananyev; +Cc: Stephen Hemminger, dev
Add simple unit-style tests for rte_bpf_load, and fix some minor
discovered bugs.
Create new source file for new tests since app/test/test_bpf.c was
growing rather large, and tests there look more like functional rather
than unit ones.
Marat Khalili (3):
bpf: fix signed shift overflows in ARM JIT
bpf: disallow empty program
bpf: make add/subtract one program validate
app/test/meson.build | 1 +
app/test/test_bpf_simple.c | 189 +++++++++++++++++++++++++++++++++++++
lib/bpf/bpf_jit_arm64.c | 188 ++++++++++++++++++++----------------
lib/bpf/bpf_load.c | 2 +-
lib/bpf/bpf_validate.c | 28 ++++--
5 files changed, 318 insertions(+), 90 deletions(-)
create mode 100644 app/test/test_bpf_simple.c
--
2.43.0
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 1/3] bpf: fix signed shift overflows in ARM JIT 2025-11-10 15:30 [PATCH 0/3] bpf: simple tests and fixes Marat Khalili @ 2025-11-10 15:30 ` Marat Khalili 2025-11-11 6:25 ` Jerin Jacob 2025-11-10 15:30 ` [PATCH 2/3] bpf: disallow empty program Marat Khalili 2025-11-10 15:30 ` [PATCH 3/3] bpf: make add/subtract one program validate Marat Khalili 2 siblings, 1 reply; 13+ messages in thread From: Marat Khalili @ 2025-11-10 15:30 UTC (permalink / raw) To: Konstantin Ananyev; +Cc: Stephen Hemminger, dev Left shifts of integer literals and bool values overwriting the sign bit were used multiple times in bpf_jit_arm64.c. E.g.: insn = (!!is64) << 31; where is64 has type bool (double bang is a no-op here). The operand of left shift was promoted to type int, which when 32-bit wide cannot represent the result. Similarly literal integers have int type by default. Sanitizer produced the following diagnostic during runtime (for various lines): lib/bpf/bpf_jit_arm64.c:241:18: runtime error: left shift of 1 by 31 places cannot be represented in type 'int' To fix the issue use RTE_BIT32 and similar macros instead. Signed-off-by: Marat Khalili <marat.khalili@huawei.com> --- lib/bpf/bpf_jit_arm64.c | 188 +++++++++++++++++++++++----------------- 1 file changed, 107 insertions(+), 81 deletions(-) diff --git a/lib/bpf/bpf_jit_arm64.c b/lib/bpf/bpf_jit_arm64.c index 96b8cd2e03..5f43db0170 100644 --- a/lib/bpf/bpf_jit_arm64.c +++ b/lib/bpf/bpf_jit_arm64.c @@ -28,7 +28,33 @@ #define A64_ZR 31 #define check_imm(n, val) (((val) >= 0) ? !!((val) >> (n)) : !!((~val) >> (n))) -#define mask_imm(n, val) ((val) & ((1 << (n)) - 1)) +#define mask_imm(n, val) ((val) & (RTE_BIT32(n) - 1)) + +/** + * Get the uint32_t shifted value. + * + * Works similarly to RTE_SHIFT_VAL32 but accepts non-literal arguments. + * Performs identically to RTE_SHIFT_VAL32 with literal arguments. + * + * @param val + * The value to be shifted, can be non-literal. + * @param nr + * The shift number in range of 0 to (32 - width of val). + */ +#define SHIFT_VAR32(val, nr) ((uint32_t)(val) << (nr)) + +/** + * Get the uint64_t shifted value. + * + * Works similarly to RTE_SHIFT_VAL64 but accepts non-literal arguments. + * Performs identically to RTE_SHIFT_VAL64 with literal arguments. + * + * @param val + * The value to be shifted, can be non-literal. + * @param nr + * The shift number in range of 0 to (64 - width of val). + */ +#define SHIFT_VAR64(val, nr) ((uint64_t)(val) << (nr)) struct ebpf_a64_map { uint32_t off; /* eBPF to arm64 insn offset mapping for jump */ @@ -238,12 +264,12 @@ emit_add_sub_imm(struct a64_jit_ctx *ctx, bool is64, bool sub, uint8_t rd, uint32_t insn, imm; imm = mask_imm(12, imm12); - insn = (!!is64) << 31; - insn |= (!!sub) << 30; + insn = SHIFT_VAR32(is64, 31); + insn |= SHIFT_VAR32(sub, 30); insn |= 0x11000000; insn |= rd; - insn |= rn << 5; - insn |= imm << 10; + insn |= SHIFT_VAR32(rn, 5); + insn |= SHIFT_VAR32(imm, 10); emit_insn(ctx, insn, check_reg(rd) || check_reg(rn) || check_imm(12, imm12)); @@ -279,16 +305,16 @@ emit_ls_pair_64(struct a64_jit_ctx *ctx, uint8_t rt, uint8_t rt2, uint8_t rn, { uint32_t insn; - insn = (!!load) << 22; - insn |= (!!pre_index) << 24; + insn = SHIFT_VAR32(load, 22); + insn |= SHIFT_VAR32(pre_index, 24); insn |= 0xa8800000; insn |= rt; - insn |= rn << 5; - insn |= rt2 << 10; + insn |= SHIFT_VAR32(rn, 5); + insn |= SHIFT_VAR32(rt2, 10); if (push) - insn |= 0x7e << 15; /* 0x7e means -2 with imm7 */ + insn |= SHIFT_VAR32(0x7e, 15); /* 0x7e means -2 with imm7 */ else - insn |= 0x2 << 15; + insn |= SHIFT_VAR32(0x2, 15); emit_insn(ctx, insn, check_reg(rn) || check_reg(rt) || check_reg(rt2)); @@ -317,11 +343,11 @@ mov_imm(struct a64_jit_ctx *ctx, bool is64, uint8_t rd, uint8_t type, { uint32_t insn; - insn = (!!is64) << 31; - insn |= type << 29; - insn |= 0x25 << 23; - insn |= (shift/16) << 21; - insn |= imm16 << 5; + insn = SHIFT_VAR32(is64, 31); + insn |= SHIFT_VAR32(type, 29); + insn |= SHIFT_VAR32(0x25, 23); + insn |= SHIFT_VAR32(shift/16, 21); + insn |= SHIFT_VAR32(imm16, 5); insn |= rd; emit_insn(ctx, insn, check_reg(rd) || check_mov_hw(is64, shift)); @@ -334,7 +360,7 @@ emit_mov_imm32(struct a64_jit_ctx *ctx, bool is64, uint8_t rd, uint32_t val) uint16_t lower = val & 0xffff; /* Positive number */ - if ((val & 1UL << 31) == 0) { + if ((val & RTE_BIT32(31)) == 0) { mov_imm(ctx, is64, rd, A64_MOVZ, lower, 0); if (upper) mov_imm(ctx, is64, rd, A64_MOVK, upper, 16); @@ -393,21 +419,21 @@ emit_ls(struct a64_jit_ctx *ctx, uint8_t sz, uint8_t rt, uint8_t rn, uint8_t rm, { uint32_t insn; - insn = 0x1c1 << 21; + insn = SHIFT_VAR32(0x1c1, 21); if (load) - insn |= 1 << 22; + insn |= RTE_BIT32(22); if (sz == BPF_B) - insn |= 0 << 30; + insn |= SHIFT_VAR32(0, 30); else if (sz == BPF_H) - insn |= 1 << 30; + insn |= SHIFT_VAR32(1, 30); else if (sz == BPF_W) - insn |= 2 << 30; + insn |= SHIFT_VAR32(2, 30); else if (sz == EBPF_DW) - insn |= 3 << 30; + insn |= SHIFT_VAR32(3, 30); - insn |= rm << 16; - insn |= 0x1a << 10; /* LSL and S = 0 */ - insn |= rn << 5; + insn |= SHIFT_VAR32(rm, 16); + insn |= SHIFT_VAR32(0x1a, 10); /* LSL and S = 0 */ + insn |= SHIFT_VAR32(rn, 5); insn |= rt; emit_insn(ctx, insn, check_reg(rt) || check_reg(rn) || check_reg(rm) || @@ -436,10 +462,10 @@ emit_add_sub(struct a64_jit_ctx *ctx, bool is64, uint8_t rd, uint8_t rn, { uint32_t insn; - insn = (!!is64) << 31; - insn |= op << 21; /* shift == 0 */ - insn |= rm << 16; - insn |= rn << 5; + insn = SHIFT_VAR32(is64, 31); + insn |= SHIFT_VAR32(op, 21); /* shift == 0 */ + insn |= SHIFT_VAR32(rm, 16); + insn |= SHIFT_VAR32(rn, 5); insn |= rd; emit_insn(ctx, insn, check_reg(rd) || check_reg(rm)); @@ -468,11 +494,11 @@ emit_mul(struct a64_jit_ctx *ctx, bool is64, uint8_t rd, uint8_t rm) { uint32_t insn; - insn = (!!is64) << 31; - insn |= 0xd8 << 21; - insn |= rm << 16; - insn |= A64_ZR << 10; - insn |= rd << 5; + insn = SHIFT_VAR32(is64, 31); + insn |= SHIFT_VAR32(0xd8, 21); + insn |= SHIFT_VAR32(rm, 16); + insn |= SHIFT_VAR32(A64_ZR, 10); + insn |= SHIFT_VAR32(rd, 5); insn |= rd; emit_insn(ctx, insn, check_reg(rd) || check_reg(rm)); @@ -489,11 +515,11 @@ emit_data_process_two_src(struct a64_jit_ctx *ctx, bool is64, uint8_t rd, { uint32_t insn; - insn = (!!is64) << 31; - insn |= 0xd6 << 21; - insn |= rm << 16; - insn |= op << 10; - insn |= rn << 5; + insn = SHIFT_VAR32(is64, 31); + insn |= SHIFT_VAR32(0xd6, 21); + insn |= SHIFT_VAR32(rm, 16); + insn |= SHIFT_VAR32(op, 10); + insn |= SHIFT_VAR32(rn, 5); insn |= rd; emit_insn(ctx, insn, check_reg(rd) || check_reg(rm)); @@ -532,14 +558,14 @@ emit_bitfield(struct a64_jit_ctx *ctx, bool is64, uint8_t rd, uint8_t rn, { uint32_t insn; - insn = (!!is64) << 31; + insn = SHIFT_VAR32(is64, 31); if (insn) - insn |= 1 << 22; /* Set N bit when is64 is set */ - insn |= op << 29; - insn |= 0x26 << 23; - insn |= immr << 16; - insn |= imms << 10; - insn |= rn << 5; + insn |= RTE_BIT32(22); /* Set N bit when is64 is set */ + insn |= SHIFT_VAR32(op, 29); + insn |= SHIFT_VAR32(0x26, 23); + insn |= SHIFT_VAR32(immr, 16); + insn |= SHIFT_VAR32(imms, 10); + insn |= SHIFT_VAR32(rn, 5); insn |= rd; emit_insn(ctx, insn, check_reg(rd) || check_reg(rn) || @@ -578,11 +604,11 @@ emit_logical(struct a64_jit_ctx *ctx, bool is64, uint8_t rd, { uint32_t insn; - insn = (!!is64) << 31; - insn |= op << 29; - insn |= 0x50 << 21; - insn |= rm << 16; - insn |= rd << 5; + insn = SHIFT_VAR32(is64, 31); + insn |= SHIFT_VAR32(op, 29); + insn |= SHIFT_VAR32(0x50, 21); + insn |= SHIFT_VAR32(rm, 16); + insn |= SHIFT_VAR32(rd, 5); insn |= rd; emit_insn(ctx, insn, check_reg(rd) || check_reg(rm)); @@ -612,12 +638,12 @@ emit_msub(struct a64_jit_ctx *ctx, bool is64, uint8_t rd, uint8_t rn, { uint32_t insn; - insn = (!!is64) << 31; - insn |= 0xd8 << 21; - insn |= rm << 16; - insn |= 0x1 << 15; - insn |= ra << 10; - insn |= rn << 5; + insn = SHIFT_VAR32(is64, 31); + insn |= SHIFT_VAR32(0xd8, 21); + insn |= SHIFT_VAR32(rm, 16); + insn |= SHIFT_VAR32(0x1, 15); + insn |= SHIFT_VAR32(ra, 10); + insn |= SHIFT_VAR32(rn, 5); insn |= rd; emit_insn(ctx, insn, check_reg(rd) || check_reg(rn) || check_reg(rm) || @@ -638,7 +664,7 @@ emit_blr(struct a64_jit_ctx *ctx, uint8_t rn) uint32_t insn; insn = 0xd63f0000; - insn |= rn << 5; + insn |= SHIFT_VAR32(rn, 5); emit_insn(ctx, insn, check_reg(rn)); } @@ -669,22 +695,22 @@ emit_rev(struct a64_jit_ctx *ctx, uint8_t rd, int32_t imm) uint32_t insn; insn = 0xdac00000; - insn |= rd << 5; + insn |= SHIFT_VAR32(rd, 5); insn |= rd; switch (imm) { case 16: - insn |= 1 << 10; + insn |= SHIFT_VAR32(1, 10); emit_insn(ctx, insn, check_reg(rd)); emit_zero_extend(ctx, rd, 16); break; case 32: - insn |= 2 << 10; + insn |= SHIFT_VAR32(2, 10); emit_insn(ctx, insn, check_reg(rd)); /* Upper 32 bits already cleared */ break; case 64: - insn |= 3 << 10; + insn |= SHIFT_VAR32(3, 10); emit_insn(ctx, insn, check_reg(rd)); break; default: @@ -933,9 +959,9 @@ emit_cbnz(struct a64_jit_ctx *ctx, bool is64, uint8_t rt, int32_t imm19) uint32_t insn, imm; imm = mask_imm(19, imm19); - insn = (!!is64) << 31; - insn |= 0x35 << 24; - insn |= imm << 5; + insn = SHIFT_VAR32(is64, 31); + insn |= SHIFT_VAR32(0x35, 24); + insn |= SHIFT_VAR32(imm, 5); insn |= rt; emit_insn(ctx, insn, check_reg(rt) || check_imm(19, imm19)); @@ -947,7 +973,7 @@ emit_b(struct a64_jit_ctx *ctx, int32_t imm26) uint32_t insn, imm; imm = mask_imm(26, imm26); - insn = 0x5 << 26; + insn = SHIFT_VAR32(0x5, 26); insn |= imm; emit_insn(ctx, insn, check_imm(26, imm26)); @@ -971,9 +997,9 @@ emit_stadd(struct a64_jit_ctx *ctx, bool is64, uint8_t rs, uint8_t rn) uint32_t insn; insn = 0xb820001f; - insn |= (!!is64) << 30; - insn |= rs << 16; - insn |= rn << 5; + insn |= SHIFT_VAR32(is64, 30); + insn |= SHIFT_VAR32(rs, 16); + insn |= SHIFT_VAR32(rn, 5); emit_insn(ctx, insn, check_reg(rs) || check_reg(rn)); } @@ -984,8 +1010,8 @@ emit_ldxr(struct a64_jit_ctx *ctx, bool is64, uint8_t rt, uint8_t rn) uint32_t insn; insn = 0x885f7c00; - insn |= (!!is64) << 30; - insn |= rn << 5; + insn |= SHIFT_VAR32(is64, 30); + insn |= SHIFT_VAR32(rn, 5); insn |= rt; emit_insn(ctx, insn, check_reg(rt) || check_reg(rn)); @@ -998,9 +1024,9 @@ emit_stxr(struct a64_jit_ctx *ctx, bool is64, uint8_t rs, uint8_t rt, uint32_t insn; insn = 0x88007c00; - insn |= (!!is64) << 30; - insn |= rs << 16; - insn |= rn << 5; + insn |= SHIFT_VAR32(is64, 30); + insn |= SHIFT_VAR32(rs, 16); + insn |= SHIFT_VAR32(rn, 5); insn |= rt; emit_insn(ctx, insn, check_reg(rs) || check_reg(rt) || check_reg(rn)); @@ -1051,9 +1077,9 @@ emit_cmp_tst(struct a64_jit_ctx *ctx, bool is64, uint8_t rn, uint8_t rm, uint32_t insn; insn = opc; - insn |= (!!is64) << 31; - insn |= rm << 16; - insn |= rn << 5; + insn |= SHIFT_VAR32(is64, 31); + insn |= SHIFT_VAR32(rm, 16); + insn |= SHIFT_VAR32(rn, 5); emit_insn(ctx, insn, check_reg(rn) || check_reg(rm)); } @@ -1076,8 +1102,8 @@ emit_b_cond(struct a64_jit_ctx *ctx, uint8_t cond, int32_t imm19) uint32_t insn, imm; imm = mask_imm(19, imm19); - insn = 0x15 << 26; - insn |= imm << 5; + insn = SHIFT_VAR32(0x15, 26); + insn |= SHIFT_VAR32(imm, 5); insn |= cond; emit_insn(ctx, insn, check_cond(cond) || check_imm(19, imm19)); @@ -1301,7 +1327,7 @@ emit(struct a64_jit_ctx *ctx, struct rte_bpf *bpf) break; /* dst = imm64 */ case (BPF_LD | BPF_IMM | EBPF_DW): - u64 = ((uint64_t)ins[1].imm << 32) | (uint32_t)imm; + u64 = SHIFT_VAR64(ins[1].imm, 32) | (uint32_t)imm; emit_mov_imm(ctx, 1, dst, u64); i++; break; -- 2.43.0 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] bpf: fix signed shift overflows in ARM JIT 2025-11-10 15:30 ` [PATCH 1/3] bpf: fix signed shift overflows in ARM JIT Marat Khalili @ 2025-11-11 6:25 ` Jerin Jacob 2025-11-11 7:53 ` Morten Brørup 2025-11-11 10:10 ` Marat Khalili 0 siblings, 2 replies; 13+ messages in thread From: Jerin Jacob @ 2025-11-11 6:25 UTC (permalink / raw) To: Marat Khalili; +Cc: Konstantin Ananyev, Stephen Hemminger, dev On Mon, Nov 10, 2025 at 9:01 PM Marat Khalili <marat.khalili@huawei.com> wrote: > > Left shifts of integer literals and bool values overwriting the sign bit > were used multiple times in bpf_jit_arm64.c. E.g.: > > insn = (!!is64) << 31; > > where is64 has type bool (double bang is a no-op here). The operand of > left shift was promoted to type int, which when 32-bit wide cannot > represent the result. Similarly literal integers have int type by > default. Sanitizer produced the following diagnostic during runtime > (for various lines): > > lib/bpf/bpf_jit_arm64.c:241:18: runtime error: left shift of 1 by 31 > places cannot be represented in type 'int' Wonder why none of the tests in app/test/test_bpf.c able to catch this? The generated ARM opcode looks OK (otherwise tests wont pass). Could you check what is missing in the app/test/test_bpf.c? Also SHIFT_VAR32 needs goto common code. > > To fix the issue use RTE_BIT32 and similar macros instead. > > Signed-off-by: Marat Khalili <marat.khalili@huawei.com> > --- > lib/bpf/bpf_jit_arm64.c | 188 +++++++++++++++++++++++----------------- > 1 file changed, 107 insertions(+), 81 deletions(-) > > diff --git a/lib/bpf/bpf_jit_arm64.c b/lib/bpf/bpf_jit_arm64.c > index 96b8cd2e03..5f43db0170 100644 > --- a/lib/bpf/bpf_jit_arm64.c > +++ b/lib/bpf/bpf_jit_arm64.c > @@ -28,7 +28,33 @@ > #define A64_ZR 31 > > #define check_imm(n, val) (((val) >= 0) ? !!((val) >> (n)) : !!((~val) >> (n))) > -#define mask_imm(n, val) ((val) & ((1 << (n)) - 1)) > +#define mask_imm(n, val) ((val) & (RTE_BIT32(n) - 1)) > + > +/** > + * Get the uint32_t shifted value. > + * > + * Works similarly to RTE_SHIFT_VAL32 but accepts non-literal arguments. > + * Performs identically to RTE_SHIFT_VAL32 with literal arguments. > + * > + * @param val > + * The value to be shifted, can be non-literal. > + * @param nr > + * The shift number in range of 0 to (32 - width of val). > + */ > +#define SHIFT_VAR32(val, nr) ((uint32_t)(val) << (nr)) > + > +/** > + * Get the uint64_t shifted value. > + * > + * Works similarly to RTE_SHIFT_VAL64 but accepts non-literal arguments. > + * Performs identically to RTE_SHIFT_VAL64 with literal arguments. > + * > + * @param val > + * The value to be shifted, can be non-literal. > + * @param nr > + * The shift number in range of 0 to (64 - width of val). > + */ > +#define SHIFT_VAR64(val, nr) ((uint64_t)(val) << (nr)) > > struct ebpf_a64_map { > uint32_t off; /* eBPF to arm64 insn offset mapping for jump */ > @@ -238,12 +264,12 @@ emit_add_sub_imm(struct a64_jit_ctx *ctx, bool is64, bool sub, uint8_t rd, > uint32_t insn, imm; > > imm = mask_imm(12, imm12); > - insn = (!!is64) << 31; > - insn |= (!!sub) << 30; > + insn = SHIFT_VAR32(is64, 31); > + insn |= SHIFT_VAR32(sub, 30); > insn |= 0x11000000; > insn |= rd; > - insn |= rn << 5; > - insn |= imm << 10; > + insn |= SHIFT_VAR32(rn, 5); > + insn |= SHIFT_VAR32(imm, 10); > > emit_insn(ctx, insn, > check_reg(rd) || check_reg(rn) || check_imm(12, imm12)); > @@ -279,16 +305,16 @@ emit_ls_pair_64(struct a64_jit_ctx *ctx, uint8_t rt, uint8_t rt2, uint8_t rn, > { > uint32_t insn; > > - insn = (!!load) << 22; > - insn |= (!!pre_index) << 24; > + insn = SHIFT_VAR32(load, 22); > + insn |= SHIFT_VAR32(pre_index, 24); > insn |= 0xa8800000; > insn |= rt; > - insn |= rn << 5; > - insn |= rt2 << 10; > + insn |= SHIFT_VAR32(rn, 5); > + insn |= SHIFT_VAR32(rt2, 10); > if (push) > - insn |= 0x7e << 15; /* 0x7e means -2 with imm7 */ > + insn |= SHIFT_VAR32(0x7e, 15); /* 0x7e means -2 with imm7 */ > else > - insn |= 0x2 << 15; > + insn |= SHIFT_VAR32(0x2, 15); > > emit_insn(ctx, insn, check_reg(rn) || check_reg(rt) || check_reg(rt2)); > > @@ -317,11 +343,11 @@ mov_imm(struct a64_jit_ctx *ctx, bool is64, uint8_t rd, uint8_t type, > { > uint32_t insn; > > - insn = (!!is64) << 31; > - insn |= type << 29; > - insn |= 0x25 << 23; > - insn |= (shift/16) << 21; > - insn |= imm16 << 5; > + insn = SHIFT_VAR32(is64, 31); > + insn |= SHIFT_VAR32(type, 29); > + insn |= SHIFT_VAR32(0x25, 23); > + insn |= SHIFT_VAR32(shift/16, 21); > + insn |= SHIFT_VAR32(imm16, 5); > insn |= rd; > > emit_insn(ctx, insn, check_reg(rd) || check_mov_hw(is64, shift)); > @@ -334,7 +360,7 @@ emit_mov_imm32(struct a64_jit_ctx *ctx, bool is64, uint8_t rd, uint32_t val) > uint16_t lower = val & 0xffff; > > /* Positive number */ > - if ((val & 1UL << 31) == 0) { > + if ((val & RTE_BIT32(31)) == 0) { > mov_imm(ctx, is64, rd, A64_MOVZ, lower, 0); > if (upper) > mov_imm(ctx, is64, rd, A64_MOVK, upper, 16); > @@ -393,21 +419,21 @@ emit_ls(struct a64_jit_ctx *ctx, uint8_t sz, uint8_t rt, uint8_t rn, uint8_t rm, > { > uint32_t insn; > > - insn = 0x1c1 << 21; > + insn = SHIFT_VAR32(0x1c1, 21); > if (load) > - insn |= 1 << 22; > + insn |= RTE_BIT32(22); > if (sz == BPF_B) > - insn |= 0 << 30; > + insn |= SHIFT_VAR32(0, 30); > else if (sz == BPF_H) > - insn |= 1 << 30; > + insn |= SHIFT_VAR32(1, 30); > else if (sz == BPF_W) > - insn |= 2 << 30; > + insn |= SHIFT_VAR32(2, 30); > else if (sz == EBPF_DW) > - insn |= 3 << 30; > + insn |= SHIFT_VAR32(3, 30); > > - insn |= rm << 16; > - insn |= 0x1a << 10; /* LSL and S = 0 */ > - insn |= rn << 5; > + insn |= SHIFT_VAR32(rm, 16); > + insn |= SHIFT_VAR32(0x1a, 10); /* LSL and S = 0 */ > + insn |= SHIFT_VAR32(rn, 5); > insn |= rt; > > emit_insn(ctx, insn, check_reg(rt) || check_reg(rn) || check_reg(rm) || > @@ -436,10 +462,10 @@ emit_add_sub(struct a64_jit_ctx *ctx, bool is64, uint8_t rd, uint8_t rn, > { > uint32_t insn; > > - insn = (!!is64) << 31; > - insn |= op << 21; /* shift == 0 */ > - insn |= rm << 16; > - insn |= rn << 5; > + insn = SHIFT_VAR32(is64, 31); > + insn |= SHIFT_VAR32(op, 21); /* shift == 0 */ > + insn |= SHIFT_VAR32(rm, 16); > + insn |= SHIFT_VAR32(rn, 5); > insn |= rd; > > emit_insn(ctx, insn, check_reg(rd) || check_reg(rm)); > @@ -468,11 +494,11 @@ emit_mul(struct a64_jit_ctx *ctx, bool is64, uint8_t rd, uint8_t rm) > { > uint32_t insn; > > - insn = (!!is64) << 31; > - insn |= 0xd8 << 21; > - insn |= rm << 16; > - insn |= A64_ZR << 10; > - insn |= rd << 5; > + insn = SHIFT_VAR32(is64, 31); > + insn |= SHIFT_VAR32(0xd8, 21); > + insn |= SHIFT_VAR32(rm, 16); > + insn |= SHIFT_VAR32(A64_ZR, 10); > + insn |= SHIFT_VAR32(rd, 5); > insn |= rd; > > emit_insn(ctx, insn, check_reg(rd) || check_reg(rm)); > @@ -489,11 +515,11 @@ emit_data_process_two_src(struct a64_jit_ctx *ctx, bool is64, uint8_t rd, > { > uint32_t insn; > > - insn = (!!is64) << 31; > - insn |= 0xd6 << 21; > - insn |= rm << 16; > - insn |= op << 10; > - insn |= rn << 5; > + insn = SHIFT_VAR32(is64, 31); > + insn |= SHIFT_VAR32(0xd6, 21); > + insn |= SHIFT_VAR32(rm, 16); > + insn |= SHIFT_VAR32(op, 10); > + insn |= SHIFT_VAR32(rn, 5); > insn |= rd; > > emit_insn(ctx, insn, check_reg(rd) || check_reg(rm)); > @@ -532,14 +558,14 @@ emit_bitfield(struct a64_jit_ctx *ctx, bool is64, uint8_t rd, uint8_t rn, > { > uint32_t insn; > > - insn = (!!is64) << 31; > + insn = SHIFT_VAR32(is64, 31); > if (insn) > - insn |= 1 << 22; /* Set N bit when is64 is set */ > - insn |= op << 29; > - insn |= 0x26 << 23; > - insn |= immr << 16; > - insn |= imms << 10; > - insn |= rn << 5; > + insn |= RTE_BIT32(22); /* Set N bit when is64 is set */ > + insn |= SHIFT_VAR32(op, 29); > + insn |= SHIFT_VAR32(0x26, 23); > + insn |= SHIFT_VAR32(immr, 16); > + insn |= SHIFT_VAR32(imms, 10); > + insn |= SHIFT_VAR32(rn, 5); > insn |= rd; > > emit_insn(ctx, insn, check_reg(rd) || check_reg(rn) || > @@ -578,11 +604,11 @@ emit_logical(struct a64_jit_ctx *ctx, bool is64, uint8_t rd, > { > uint32_t insn; > > - insn = (!!is64) << 31; > - insn |= op << 29; > - insn |= 0x50 << 21; > - insn |= rm << 16; > - insn |= rd << 5; > + insn = SHIFT_VAR32(is64, 31); > + insn |= SHIFT_VAR32(op, 29); > + insn |= SHIFT_VAR32(0x50, 21); > + insn |= SHIFT_VAR32(rm, 16); > + insn |= SHIFT_VAR32(rd, 5); > insn |= rd; > > emit_insn(ctx, insn, check_reg(rd) || check_reg(rm)); > @@ -612,12 +638,12 @@ emit_msub(struct a64_jit_ctx *ctx, bool is64, uint8_t rd, uint8_t rn, > { > uint32_t insn; > > - insn = (!!is64) << 31; > - insn |= 0xd8 << 21; > - insn |= rm << 16; > - insn |= 0x1 << 15; > - insn |= ra << 10; > - insn |= rn << 5; > + insn = SHIFT_VAR32(is64, 31); > + insn |= SHIFT_VAR32(0xd8, 21); > + insn |= SHIFT_VAR32(rm, 16); > + insn |= SHIFT_VAR32(0x1, 15); > + insn |= SHIFT_VAR32(ra, 10); > + insn |= SHIFT_VAR32(rn, 5); > insn |= rd; > > emit_insn(ctx, insn, check_reg(rd) || check_reg(rn) || check_reg(rm) || > @@ -638,7 +664,7 @@ emit_blr(struct a64_jit_ctx *ctx, uint8_t rn) > uint32_t insn; > > insn = 0xd63f0000; > - insn |= rn << 5; > + insn |= SHIFT_VAR32(rn, 5); > > emit_insn(ctx, insn, check_reg(rn)); > } > @@ -669,22 +695,22 @@ emit_rev(struct a64_jit_ctx *ctx, uint8_t rd, int32_t imm) > uint32_t insn; > > insn = 0xdac00000; > - insn |= rd << 5; > + insn |= SHIFT_VAR32(rd, 5); > insn |= rd; > > switch (imm) { > case 16: > - insn |= 1 << 10; > + insn |= SHIFT_VAR32(1, 10); > emit_insn(ctx, insn, check_reg(rd)); > emit_zero_extend(ctx, rd, 16); > break; > case 32: > - insn |= 2 << 10; > + insn |= SHIFT_VAR32(2, 10); > emit_insn(ctx, insn, check_reg(rd)); > /* Upper 32 bits already cleared */ > break; > case 64: > - insn |= 3 << 10; > + insn |= SHIFT_VAR32(3, 10); > emit_insn(ctx, insn, check_reg(rd)); > break; > default: > @@ -933,9 +959,9 @@ emit_cbnz(struct a64_jit_ctx *ctx, bool is64, uint8_t rt, int32_t imm19) > uint32_t insn, imm; > > imm = mask_imm(19, imm19); > - insn = (!!is64) << 31; > - insn |= 0x35 << 24; > - insn |= imm << 5; > + insn = SHIFT_VAR32(is64, 31); > + insn |= SHIFT_VAR32(0x35, 24); > + insn |= SHIFT_VAR32(imm, 5); > insn |= rt; > > emit_insn(ctx, insn, check_reg(rt) || check_imm(19, imm19)); > @@ -947,7 +973,7 @@ emit_b(struct a64_jit_ctx *ctx, int32_t imm26) > uint32_t insn, imm; > > imm = mask_imm(26, imm26); > - insn = 0x5 << 26; > + insn = SHIFT_VAR32(0x5, 26); > insn |= imm; > > emit_insn(ctx, insn, check_imm(26, imm26)); > @@ -971,9 +997,9 @@ emit_stadd(struct a64_jit_ctx *ctx, bool is64, uint8_t rs, uint8_t rn) > uint32_t insn; > > insn = 0xb820001f; > - insn |= (!!is64) << 30; > - insn |= rs << 16; > - insn |= rn << 5; > + insn |= SHIFT_VAR32(is64, 30); > + insn |= SHIFT_VAR32(rs, 16); > + insn |= SHIFT_VAR32(rn, 5); > > emit_insn(ctx, insn, check_reg(rs) || check_reg(rn)); > } > @@ -984,8 +1010,8 @@ emit_ldxr(struct a64_jit_ctx *ctx, bool is64, uint8_t rt, uint8_t rn) > uint32_t insn; > > insn = 0x885f7c00; > - insn |= (!!is64) << 30; > - insn |= rn << 5; > + insn |= SHIFT_VAR32(is64, 30); > + insn |= SHIFT_VAR32(rn, 5); > insn |= rt; > > emit_insn(ctx, insn, check_reg(rt) || check_reg(rn)); > @@ -998,9 +1024,9 @@ emit_stxr(struct a64_jit_ctx *ctx, bool is64, uint8_t rs, uint8_t rt, > uint32_t insn; > > insn = 0x88007c00; > - insn |= (!!is64) << 30; > - insn |= rs << 16; > - insn |= rn << 5; > + insn |= SHIFT_VAR32(is64, 30); > + insn |= SHIFT_VAR32(rs, 16); > + insn |= SHIFT_VAR32(rn, 5); > insn |= rt; > > emit_insn(ctx, insn, check_reg(rs) || check_reg(rt) || check_reg(rn)); > @@ -1051,9 +1077,9 @@ emit_cmp_tst(struct a64_jit_ctx *ctx, bool is64, uint8_t rn, uint8_t rm, > uint32_t insn; > > insn = opc; > - insn |= (!!is64) << 31; > - insn |= rm << 16; > - insn |= rn << 5; > + insn |= SHIFT_VAR32(is64, 31); > + insn |= SHIFT_VAR32(rm, 16); > + insn |= SHIFT_VAR32(rn, 5); > > emit_insn(ctx, insn, check_reg(rn) || check_reg(rm)); > } > @@ -1076,8 +1102,8 @@ emit_b_cond(struct a64_jit_ctx *ctx, uint8_t cond, int32_t imm19) > uint32_t insn, imm; > > imm = mask_imm(19, imm19); > - insn = 0x15 << 26; > - insn |= imm << 5; > + insn = SHIFT_VAR32(0x15, 26); > + insn |= SHIFT_VAR32(imm, 5); > insn |= cond; > > emit_insn(ctx, insn, check_cond(cond) || check_imm(19, imm19)); > @@ -1301,7 +1327,7 @@ emit(struct a64_jit_ctx *ctx, struct rte_bpf *bpf) > break; > /* dst = imm64 */ > case (BPF_LD | BPF_IMM | EBPF_DW): > - u64 = ((uint64_t)ins[1].imm << 32) | (uint32_t)imm; > + u64 = SHIFT_VAR64(ins[1].imm, 32) | (uint32_t)imm; > emit_mov_imm(ctx, 1, dst, u64); > i++; > break; > -- > 2.43.0 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 1/3] bpf: fix signed shift overflows in ARM JIT 2025-11-11 6:25 ` Jerin Jacob @ 2025-11-11 7:53 ` Morten Brørup 2025-11-11 10:10 ` Marat Khalili 1 sibling, 0 replies; 13+ messages in thread From: Morten Brørup @ 2025-11-11 7:53 UTC (permalink / raw) To: Jerin Jacob, Marat Khalili; +Cc: Konstantin Ananyev, Stephen Hemminger, dev > From: Jerin Jacob [mailto:jerinjacobk@gmail.com] > Sent: Tuesday, 11 November 2025 07.25 > > On Mon, Nov 10, 2025 at 9:01 PM Marat Khalili > <marat.khalili@huawei.com> wrote: > > > > Left shifts of integer literals and bool values overwriting the sign > bit > > were used multiple times in bpf_jit_arm64.c. E.g.: > > > > insn = (!!is64) << 31; > > > > where is64 has type bool (double bang is a no-op here). The operand > of > > left shift was promoted to type int, which when 32-bit wide cannot > > represent the result. Similarly literal integers have int type by > > default. Sanitizer produced the following diagnostic during runtime > > (for various lines): > > > > lib/bpf/bpf_jit_arm64.c:241:18: runtime error: left shift of 1 by > 31 > > places cannot be represented in type 'int' > > Wonder why none of the tests in app/test/test_bpf.c able to catch > this? The generated ARM opcode looks OK (otherwise tests wont pass). > Could you check what is missing in the app/test/test_bpf.c? > > Also SHIFT_VAR32 needs goto common code. > > > > > > To fix the issue use RTE_BIT32 and similar macros instead. > > > > Signed-off-by: Marat Khalili <marat.khalili@huawei.com> > > --- > > lib/bpf/bpf_jit_arm64.c | 188 +++++++++++++++++++++++--------------- > -- > > 1 file changed, 107 insertions(+), 81 deletions(-) > > > > diff --git a/lib/bpf/bpf_jit_arm64.c b/lib/bpf/bpf_jit_arm64.c > > index 96b8cd2e03..5f43db0170 100644 > > --- a/lib/bpf/bpf_jit_arm64.c > > +++ b/lib/bpf/bpf_jit_arm64.c > > @@ -28,7 +28,33 @@ > > #define A64_ZR 31 > > > > #define check_imm(n, val) (((val) >= 0) ? !!((val) >> (n)) : > !!((~val) >> (n))) > > -#define mask_imm(n, val) ((val) & ((1 << (n)) - 1)) > > +#define mask_imm(n, val) ((val) & (RTE_BIT32(n) - 1)) > > + > > +/** > > + * Get the uint32_t shifted value. > > + * > > + * Works similarly to RTE_SHIFT_VAL32 but accepts non-literal > arguments. > > + * Performs identically to RTE_SHIFT_VAL32 with literal arguments. > > + * > > + * @param val > > + * The value to be shifted, can be non-literal. > > + * @param nr > > + * The shift number in range of 0 to (32 - width of val). > > + */ > > +#define SHIFT_VAR32(val, nr) ((uint32_t)(val) << (nr)) > > + > > +/** > > + * Get the uint64_t shifted value. > > + * > > + * Works similarly to RTE_SHIFT_VAL64 but accepts non-literal > arguments. > > + * Performs identically to RTE_SHIFT_VAL64 with literal arguments. > > + * > > + * @param val > > + * The value to be shifted, can be non-literal. > > + * @param nr > > + * The shift number in range of 0 to (64 - width of val). > > + */ > > +#define SHIFT_VAR64(val, nr) ((uint64_t)(val) << (nr)) Better replace the RTE_SHIFT_VAL32/VAL64 macros with these, to support both literal and non-literal arguments. ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 1/3] bpf: fix signed shift overflows in ARM JIT 2025-11-11 6:25 ` Jerin Jacob 2025-11-11 7:53 ` Morten Brørup @ 2025-11-11 10:10 ` Marat Khalili 2025-11-11 16:29 ` Jerin Jacob 2025-11-11 16:31 ` Jerin Jacob 1 sibling, 2 replies; 13+ messages in thread From: Marat Khalili @ 2025-11-11 10:10 UTC (permalink / raw) To: Jerin Jacob; +Cc: Konstantin Ananyev, Stephen Hemminger, dev > On Mon, Nov 10, 2025 at 9:01 PM Marat Khalili <marat.khalili@huawei.com> wrote: > > > > Left shifts of integer literals and bool values overwriting the sign bit > > were used multiple times in bpf_jit_arm64.c. E.g.: > > > > insn = (!!is64) << 31; > > > > where is64 has type bool (double bang is a no-op here). The operand of > > left shift was promoted to type int, which when 32-bit wide cannot > > represent the result. Similarly literal integers have int type by > > default. Sanitizer produced the following diagnostic during runtime > > (for various lines): > > > > lib/bpf/bpf_jit_arm64.c:241:18: runtime error: left shift of 1 by 31 > > places cannot be represented in type 'int' > > Wonder why none of the tests in app/test/test_bpf.c able to catch > this? The generated ARM opcode looks OK (otherwise tests wont pass). > Could you check what is missing in the app/test/test_bpf.c? That tests do trigger it. RTE>>bpf_autotest ../../src/lib/bpf/bpf_jit_arm64.c:320:18: runtime error: left shift of 1 by 31 places cannot be represented in type 'int' #0 0x22bf59c in mov_imm ../../src/lib/bpf/bpf_jit_arm64.c:320 #1 0x22bfae0 in emit_mov_imm ../../src/lib/bpf/bpf_jit_arm64.c:378 #2 0x22c3bfc in emit ../../src/lib/bpf/bpf_jit_arm64.c:1321 #3 0x22c4314 in __rte_bpf_jit_arm64 ../../src/lib/bpf/bpf_jit_arm64.c:1414 #4 0x22775c0 in __rte_bpf_jit ../../src/lib/bpf/bpf.c:44 #5 0x228fbdc in rte_bpf_load ../../src/lib/bpf/bpf_load.c:115 #6 0x6312f8 in run_test ../../src/app/test/test_bpf.c:3225 #7 0x631848 in test_bpf ../../src/app/test/test_bpf.c:3269 [...] RTE>>bpf_convert_autotest ../../src/lib/bpf/bpf_jit_arm64.c:241:18: runtime error: left shift of 1 by 31 places cannot be represented in type 'int' #0 0x22bf04c in emit_add_sub_imm ../../src/lib/bpf/bpf_jit_arm64.c:241 #1 0x22bf270 in emit_mov ../../src/lib/bpf/bpf_jit_arm64.c:267 #2 0x22bf2a4 in emit_mov_64 ../../src/lib/bpf/bpf_jit_arm64.c:273 #3 0x22c1a68 in emit_prologue_no_call ../../src/lib/bpf/bpf_jit_arm64.c:884 #4 0x22c1c9c in emit_prologue ../../src/lib/bpf/bpf_jit_arm64.c:904 #5 0x22c327c in emit ../../src/lib/bpf/bpf_jit_arm64.c:1136 #6 0x22c4314 in __rte_bpf_jit_arm64 ../../src/lib/bpf/bpf_jit_arm64.c:1414 #7 0x22775c0 in __rte_bpf_jit ../../src/lib/bpf/bpf.c:44 #8 0x228fbdc in rte_bpf_load ../../src/lib/bpf/bpf_load.c:115 #9 0x631e7c in test_bpf_match ../../src/app/test/test_bpf.c:3328 #10 0x632ce8 in test_bpf_filter_sanity ../../src/app/test/test_bpf.c:3376 #11 0x6331c4 in test_bpf_convert ../../src/app/test/test_bpf.c:3487 [...] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] bpf: fix signed shift overflows in ARM JIT 2025-11-11 10:10 ` Marat Khalili @ 2025-11-11 16:29 ` Jerin Jacob 2025-11-11 16:31 ` Jerin Jacob 1 sibling, 0 replies; 13+ messages in thread From: Jerin Jacob @ 2025-11-11 16:29 UTC (permalink / raw) To: Marat Khalili; +Cc: Konstantin Ananyev, Stephen Hemminger, dev On Tue, Nov 11, 2025 at 3:40 PM Marat Khalili <marat.khalili@huawei.com> wrote: > > > On Mon, Nov 10, 2025 at 9:01 PM Marat Khalili <marat.khalili@huawei.com> wrote: > > > > > > Left shifts of integer literals and bool values overwriting the sign bit > > > were used multiple times in bpf_jit_arm64.c. E.g.: > > > > > > insn = (!!is64) << 31; > > > > > > where is64 has type bool (double bang is a no-op here). The operand of > > > left shift was promoted to type int, which when 32-bit wide cannot > > > represent the result. Similarly literal integers have int type by > > > default. Sanitizer produced the following diagnostic during runtime > > > (for various lines): > > > > > > lib/bpf/bpf_jit_arm64.c:241:18: runtime error: left shift of 1 by 31 > > > places cannot be represented in type 'int' > > > > Wonder why none of the tests in app/test/test_bpf.c able to catch > > this? The generated ARM opcode looks OK (otherwise tests wont pass). > > Could you check what is missing in the app/test/test_bpf.c? > > That tests do trigger it. > > RTE>>bpf_autotest > ../../src/lib/bpf/bpf_jit_arm64.c:320:18: runtime error: left shift of 1 by 31 places cannot be represented in type 'int' > #0 0x22bf59c in mov_imm ../../src/lib/bpf/bpf_jit_arm64.c:320 > #1 0x22bfae0 in emit_mov_imm ../../src/lib/bpf/bpf_jit_arm64.c:378 > #2 0x22c3bfc in emit ../../src/lib/bpf/bpf_jit_arm64.c:1321 > #3 0x22c4314 in __rte_bpf_jit_arm64 ../../src/lib/bpf/bpf_jit_arm64.c:1414 > #4 0x22775c0 in __rte_bpf_jit ../../src/lib/bpf/bpf.c:44 > #5 0x228fbdc in rte_bpf_load ../../src/lib/bpf/bpf_load.c:115 > #6 0x6312f8 in run_test ../../src/app/test/test_bpf.c:3225 > #7 0x631848 in test_bpf ../../src/app/test/test_bpf.c:3269 > [...] > > RTE>>bpf_convert_autotest > ../../src/lib/bpf/bpf_jit_arm64.c:241:18: runtime error: left shift of 1 by 31 places cannot be represented in type 'int' > #0 0x22bf04c in emit_add_sub_imm ../../src/lib/bpf/bpf_jit_arm64.c:241 > #1 0x22bf270 in emit_mov ../../src/lib/bpf/bpf_jit_arm64.c:267 > #2 0x22bf2a4 in emit_mov_64 ../../src/lib/bpf/bpf_jit_arm64.c:273 > #3 0x22c1a68 in emit_prologue_no_call ../../src/lib/bpf/bpf_jit_arm64.c:884 > #4 0x22c1c9c in emit_prologue ../../src/lib/bpf/bpf_jit_arm64.c:904 > #5 0x22c327c in emit ../../src/lib/bpf/bpf_jit_arm64.c:1136 > #6 0x22c4314 in __rte_bpf_jit_arm64 ../../src/lib/bpf/bpf_jit_arm64.c:1414 > #7 0x22775c0 in __rte_bpf_jit ../../src/lib/bpf/bpf.c:44 > #8 0x228fbdc in rte_bpf_load ../../src/lib/bpf/bpf_load.c:115 > #9 0x631e7c in test_bpf_match ../../src/app/test/test_bpf.c:3328 > #10 0x632ce8 in test_bpf_filter_sanity ../../src/app/test/test_bpf.c:3376 > #11 0x6331c4 in test_bpf_convert ../../src/app/test/test_bpf.c:3487 > [...] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] bpf: fix signed shift overflows in ARM JIT 2025-11-11 10:10 ` Marat Khalili 2025-11-11 16:29 ` Jerin Jacob @ 2025-11-11 16:31 ` Jerin Jacob 2025-11-11 16:39 ` Marat Khalili 1 sibling, 1 reply; 13+ messages in thread From: Jerin Jacob @ 2025-11-11 16:31 UTC (permalink / raw) To: Marat Khalili; +Cc: Konstantin Ananyev, Stephen Hemminger, dev On Tue, Nov 11, 2025 at 3:40 PM Marat Khalili <marat.khalili@huawei.com> wrote: > > > On Mon, Nov 10, 2025 at 9:01 PM Marat Khalili <marat.khalili@huawei.com> wrote: > > > > > > Left shifts of integer literals and bool values overwriting the sign bit > > > were used multiple times in bpf_jit_arm64.c. E.g.: > > > > > > insn = (!!is64) << 31; > > > > > > where is64 has type bool (double bang is a no-op here). The operand of > > > left shift was promoted to type int, which when 32-bit wide cannot > > > represent the result. Similarly literal integers have int type by > > > default. Sanitizer produced the following diagnostic during runtime > > > (for various lines): > > > > > > lib/bpf/bpf_jit_arm64.c:241:18: runtime error: left shift of 1 by 31 > > > places cannot be represented in type 'int' > > > > Wonder why none of the tests in app/test/test_bpf.c able to catch > > this? The generated ARM opcode looks OK (otherwise tests wont pass). > > Could you check what is missing in the app/test/test_bpf.c? > > That tests do trigger it. > > RTE>>bpf_autotest > ../../src/lib/bpf/bpf_jit_arm64.c:320:18: runtime error: left shift of 1 by 31 places cannot be represented in type 'int' This is when ASAN is enabled. Right? Is it really generating BAD opcode(i.e emiter generating bad OPcode) and test fails? > #0 0x22bf59c in mov_imm ../../src/lib/bpf/bpf_jit_arm64.c:320 > #1 0x22bfae0 in emit_mov_imm ../../src/lib/bpf/bpf_jit_arm64.c:378 > #2 0x22c3bfc in emit ../../src/lib/bpf/bpf_jit_arm64.c:1321 > #3 0x22c4314 in __rte_bpf_jit_arm64 ../../src/lib/bpf/bpf_jit_arm64.c:1414 > #4 0x22775c0 in __rte_bpf_jit ../../src/lib/bpf/bpf.c:44 > #5 0x228fbdc in rte_bpf_load ../../src/lib/bpf/bpf_load.c:115 > #6 0x6312f8 in run_test ../../src/app/test/test_bpf.c:3225 > #7 0x631848 in test_bpf ../../src/app/test/test_bpf.c:3269 > [...] > > RTE>>bpf_convert_autotest > ../../src/lib/bpf/bpf_jit_arm64.c:241:18: runtime error: left shift of 1 by 31 places cannot be represented in type 'int' > #0 0x22bf04c in emit_add_sub_imm ../../src/lib/bpf/bpf_jit_arm64.c:241 > #1 0x22bf270 in emit_mov ../../src/lib/bpf/bpf_jit_arm64.c:267 > #2 0x22bf2a4 in emit_mov_64 ../../src/lib/bpf/bpf_jit_arm64.c:273 > #3 0x22c1a68 in emit_prologue_no_call ../../src/lib/bpf/bpf_jit_arm64.c:884 > #4 0x22c1c9c in emit_prologue ../../src/lib/bpf/bpf_jit_arm64.c:904 > #5 0x22c327c in emit ../../src/lib/bpf/bpf_jit_arm64.c:1136 > #6 0x22c4314 in __rte_bpf_jit_arm64 ../../src/lib/bpf/bpf_jit_arm64.c:1414 > #7 0x22775c0 in __rte_bpf_jit ../../src/lib/bpf/bpf.c:44 > #8 0x228fbdc in rte_bpf_load ../../src/lib/bpf/bpf_load.c:115 > #9 0x631e7c in test_bpf_match ../../src/app/test/test_bpf.c:3328 > #10 0x632ce8 in test_bpf_filter_sanity ../../src/app/test/test_bpf.c:3376 > #11 0x6331c4 in test_bpf_convert ../../src/app/test/test_bpf.c:3487 > [...] ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 1/3] bpf: fix signed shift overflows in ARM JIT 2025-11-11 16:31 ` Jerin Jacob @ 2025-11-11 16:39 ` Marat Khalili 2025-11-12 5:23 ` Jerin Jacob 0 siblings, 1 reply; 13+ messages in thread From: Marat Khalili @ 2025-11-11 16:39 UTC (permalink / raw) To: Jerin Jacob; +Cc: Konstantin Ananyev, Stephen Hemminger, dev > -----Original Message----- > From: Jerin Jacob <jerinjacobk@gmail.com> > Sent: Tuesday 11 November 2025 16:31 > To: Marat Khalili <marat.khalili@huawei.com> > Cc: Konstantin Ananyev <konstantin.ananyev@huawei.com>; Stephen Hemminger <stephen@networkplumber.org>; > dev@dpdk.org > Subject: Re: [PATCH 1/3] bpf: fix signed shift overflows in ARM JIT > > On Tue, Nov 11, 2025 at 3:40 PM Marat Khalili <marat.khalili@huawei.com> wrote: > > > > > On Mon, Nov 10, 2025 at 9:01 PM Marat Khalili <marat.khalili@huawei.com> wrote: > > > > > > > > Left shifts of integer literals and bool values overwriting the sign bit > > > > were used multiple times in bpf_jit_arm64.c. E.g.: > > > > > > > > insn = (!!is64) << 31; > > > > > > > > where is64 has type bool (double bang is a no-op here). The operand of > > > > left shift was promoted to type int, which when 32-bit wide cannot > > > > represent the result. Similarly literal integers have int type by > > > > default. Sanitizer produced the following diagnostic during runtime > > > > (for various lines): > > > > > > > > lib/bpf/bpf_jit_arm64.c:241:18: runtime error: left shift of 1 by 31 > > > > places cannot be represented in type 'int' > > > > > > Wonder why none of the tests in app/test/test_bpf.c able to catch > > > this? The generated ARM opcode looks OK (otherwise tests wont pass). > > > Could you check what is missing in the app/test/test_bpf.c? > > > > That tests do trigger it. > > > > RTE>>bpf_autotest > > ../../src/lib/bpf/bpf_jit_arm64.c:320:18: runtime error: left shift of 1 by 31 places cannot be > represented in type 'int' > > This is when ASAN is enabled. Right? More likely UBSAN, but yes. > Is it really generating BAD > opcode(i.e emiter generating bad OPcode) and test fails? If allowed to finish the test does not fail. However, I have not compared DPDK or JIT compiled code with and without changes. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] bpf: fix signed shift overflows in ARM JIT 2025-11-11 16:39 ` Marat Khalili @ 2025-11-12 5:23 ` Jerin Jacob 0 siblings, 0 replies; 13+ messages in thread From: Jerin Jacob @ 2025-11-12 5:23 UTC (permalink / raw) To: Marat Khalili; +Cc: Konstantin Ananyev, Stephen Hemminger, dev On Tue, Nov 11, 2025 at 10:09 PM Marat Khalili <marat.khalili@huawei.com> wrote: > > > -----Original Message----- > > From: Jerin Jacob <jerinjacobk@gmail.com> > > Sent: Tuesday 11 November 2025 16:31 > > To: Marat Khalili <marat.khalili@huawei.com> > > Cc: Konstantin Ananyev <konstantin.ananyev@huawei.com>; Stephen Hemminger <stephen@networkplumber.org>; > > dev@dpdk.org > > Subject: Re: [PATCH 1/3] bpf: fix signed shift overflows in ARM JIT > > > > On Tue, Nov 11, 2025 at 3:40 PM Marat Khalili <marat.khalili@huawei.com> wrote: > > > > > > > On Mon, Nov 10, 2025 at 9:01 PM Marat Khalili <marat.khalili@huawei.com> wrote: > > > > > > > > > > Left shifts of integer literals and bool values overwriting the sign bit > > > > > were used multiple times in bpf_jit_arm64.c. E.g.: > > > > > > > > > > insn = (!!is64) << 31; > > > > > > > > > > where is64 has type bool (double bang is a no-op here). The operand of > > > > > left shift was promoted to type int, which when 32-bit wide cannot > > > > > represent the result. Similarly literal integers have int type by > > > > > default. Sanitizer produced the following diagnostic during runtime > > > > > (for various lines): > > > > > > > > > > lib/bpf/bpf_jit_arm64.c:241:18: runtime error: left shift of 1 by 31 > > > > > places cannot be represented in type 'int' > > > > > > > > Wonder why none of the tests in app/test/test_bpf.c able to catch > > > > this? The generated ARM opcode looks OK (otherwise tests wont pass). > > > > Could you check what is missing in the app/test/test_bpf.c? > > > > > > That tests do trigger it. > > > > > > RTE>>bpf_autotest > > > ../../src/lib/bpf/bpf_jit_arm64.c:320:18: runtime error: left shift of 1 by 31 places cannot be > > represented in type 'int' > > > > This is when ASAN is enabled. Right? > > More likely UBSAN, but yes. > > > Is it really generating BAD > > opcode(i.e emiter generating bad OPcode) and test fails? > > If allowed to finish the test does not fail. Does that mean, it is a false positive? Right? Anyway, There is no harm in the new patch, Change the patch to use SHIFT macros from EAL. > However, I have not compared DPDK or JIT compiled code with and without changes. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] bpf: disallow empty program 2025-11-10 15:30 [PATCH 0/3] bpf: simple tests and fixes Marat Khalili 2025-11-10 15:30 ` [PATCH 1/3] bpf: fix signed shift overflows in ARM JIT Marat Khalili @ 2025-11-10 15:30 ` Marat Khalili 2025-11-10 16:40 ` Stephen Hemminger 2025-11-10 15:30 ` [PATCH 3/3] bpf: make add/subtract one program validate Marat Khalili 2 siblings, 1 reply; 13+ messages in thread From: Marat Khalili @ 2025-11-10 15:30 UTC (permalink / raw) To: Konstantin Ananyev; +Cc: Stephen Hemminger, dev Add tests for some simple cases: * Program with no instructions; * Program with only EXIT instruction but no return value set; * Program with return value set but no EXIT instruction; * Minimal valid program with return value set and an EXIT instruction. Fix found bugs: * a program with no instructions was accepted; * a program with no EXIT instruction read outside the buffer. Signed-off-by: Marat Khalili <marat.khalili@huawei.com> --- app/test/meson.build | 1 + app/test/test_bpf_simple.c | 131 +++++++++++++++++++++++++++++++++++++ lib/bpf/bpf_load.c | 2 +- lib/bpf/bpf_validate.c | 20 ++++-- 4 files changed, 149 insertions(+), 5 deletions(-) create mode 100644 app/test/test_bpf_simple.c diff --git a/app/test/meson.build b/app/test/meson.build index 8df8d3edd1..9d48431ba0 100644 --- a/app/test/meson.build +++ b/app/test/meson.build @@ -35,6 +35,7 @@ source_file_deps = { 'test_bitset.c': [], 'test_bitratestats.c': ['metrics', 'bitratestats', 'ethdev'] + sample_packet_forward_deps, 'test_bpf.c': ['bpf', 'net'], + 'test_bpf_simple.c': ['bpf'], 'test_byteorder.c': [], 'test_cfgfile.c': ['cfgfile'], 'test_cksum.c': ['net'], diff --git a/app/test/test_bpf_simple.c b/app/test/test_bpf_simple.c new file mode 100644 index 0000000000..576a6ed029 --- /dev/null +++ b/app/test/test_bpf_simple.c @@ -0,0 +1,131 @@ +/* SPDX-License-Identifier: BSD-3-Clause + * Copyright(c) 2025 Huawei Technologies Co., Ltd + */ + +#include "test.h" + +#include <rte_bpf.h> +#include <rte_errno.h> + +/* Tests of most simple BPF programs (no instructions, one instruction etc.) */ + +/* + * Try to load a simple bpf program from the instructions array. + * + * When `expected_errno` is zero, expect it to load successfully. + * When `expected_errno` is non-zero, expect it to fail with this `rte_errno`. + * + * @param nb_ins + * Number of instructions in the `ins` array. + * @param ins + * BPF instructions array. + * @param expected_errno + * Expected result. + * @return + * TEST_SUCCESS on success, error code on failure. + */ +static int +simple_bpf_load_test(uint32_t nb_ins, const struct ebpf_insn *ins, + int expected_errno) +{ + const struct rte_bpf_prm prm = { + .ins = ins, + .nb_ins = nb_ins, + .prog_arg = { + .type = RTE_BPF_ARG_RAW, + .size = sizeof(uint64_t), + }, + }; + + struct rte_bpf *const bpf = rte_bpf_load(&prm); + const int actual_errno = rte_errno; + rte_bpf_destroy(bpf); + + if (expected_errno != 0) { + RTE_TEST_ASSERT_EQUAL(bpf, NULL, + "expect rte_bpf_load() == NULL"); + RTE_TEST_ASSERT_EQUAL(actual_errno, expected_errno, + "expect rte_errno == %d, found %d", + expected_errno, actual_errno); + } else + RTE_TEST_ASSERT_NOT_EQUAL(bpf, NULL, + "expect rte_bpf_load() != NULL"); + + return TEST_SUCCESS; +} + +/* + * Try and load completely empty BPF program. + * Should fail because there is no EXIT (and also return value is undefined). + */ +static int +test_simple_no_instructions(void) +{ + static const struct ebpf_insn ins[] = {}; + return simple_bpf_load_test(RTE_DIM(ins), ins, EINVAL); +} + +REGISTER_FAST_TEST(bpf_simple_no_instructions_autotest, true, true, + test_simple_no_instructions); + +/* + * Try and load a BPF program comprising single EXIT instruction. + * Should fail because the return value is undefined. + */ +static int +test_simple_exit_only(void) +{ + static const struct ebpf_insn ins[] = { + { + .code = (BPF_JMP | EBPF_EXIT), + }, + }; + return simple_bpf_load_test(RTE_DIM(ins), ins, EINVAL); +} + +REGISTER_FAST_TEST(bpf_simple_exit_only_autotest, true, true, + test_simple_exit_only); + +/* + * Try and load a BPF program with no EXIT instruction. + * Should fail because of this. + */ +static int +test_simple_no_exit(void) +{ + static const struct ebpf_insn ins[] = { + { + /* Set return value to the program argument. */ + .code = (EBPF_ALU64 | EBPF_MOV | BPF_X), + .src_reg = EBPF_REG_1, + .dst_reg = EBPF_REG_0, + }, + }; + return simple_bpf_load_test(RTE_DIM(ins), ins, EINVAL); +} + +REGISTER_FAST_TEST(bpf_simple_no_exit_autotest, true, true, + test_simple_no_exit); + +/* + * Try and load smallest possible valid BPF program. + */ +static int +test_simple_minimal_working(void) +{ + static const struct ebpf_insn ins[] = { + { + /* Set return value to the program argument. */ + .code = (EBPF_ALU64 | EBPF_MOV | BPF_X), + .src_reg = EBPF_REG_1, + .dst_reg = EBPF_REG_0, + }, + { + .code = (BPF_JMP | EBPF_EXIT), + }, + }; + return simple_bpf_load_test(RTE_DIM(ins), ins, 0); +} + +REGISTER_FAST_TEST(bpf_simple_minimal_working_autotest, true, true, + test_simple_minimal_working); diff --git a/lib/bpf/bpf_load.c b/lib/bpf/bpf_load.c index 556e613762..6983c026af 100644 --- a/lib/bpf/bpf_load.c +++ b/lib/bpf/bpf_load.c @@ -88,7 +88,7 @@ rte_bpf_load(const struct rte_bpf_prm *prm) int32_t rc; uint32_t i; - if (prm == NULL || prm->ins == NULL || + if (prm == NULL || prm->ins == NULL || prm->nb_ins == 0 || (prm->nb_xsym != 0 && prm->xsym == NULL)) { rte_errno = EINVAL; return NULL; diff --git a/lib/bpf/bpf_validate.c b/lib/bpf/bpf_validate.c index 4f47d6dc7b..23444b3eaa 100644 --- a/lib/bpf/bpf_validate.c +++ b/lib/bpf/bpf_validate.c @@ -1827,7 +1827,7 @@ add_edge(struct bpf_verifier *bvf, struct inst_node *node, uint32_t nidx) { uint32_t ne; - if (nidx > bvf->prm->nb_ins) { + if (nidx >= bvf->prm->nb_ins) { RTE_BPF_LOG_LINE(ERR, "%s: program boundary violation at pc: %u, next pc: %u", __func__, get_node_idx(bvf, node), nidx); @@ -1886,14 +1886,20 @@ get_prev_node(struct bpf_verifier *bvf, struct inst_node *node) * Control Flow Graph (CFG). * Information collected at this path would be used later * to determine is there any loops, and/or unreachable instructions. + * PREREQUISITE: there is at least one node. */ static void dfs(struct bpf_verifier *bvf) { struct inst_node *next, *node; - node = bvf->in; - while (node != NULL) { + RTE_ASSERT(bvf->nb_nodes != 0); + /* + * Since there is at least one node, node with index 0 always exists; + * it is our program entry point. + */ + node = &bvf->in[0]; + do { if (node->colour == WHITE) set_node_colour(bvf, node, GREY); @@ -1923,7 +1929,7 @@ dfs(struct bpf_verifier *bvf) } } else node = NULL; - } + } while (node != NULL); } /* @@ -2062,6 +2068,12 @@ validate(struct bpf_verifier *bvf) if (rc != 0) return rc; + if (bvf->nb_nodes == 0) { + RTE_BPF_LOG_LINE(ERR, "%s(%p) the program is empty", + __func__, bvf); + return -EINVAL; + } + dfs(bvf); RTE_LOG(DEBUG, BPF, "%s(%p) stats:\n" -- 2.43.0 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] bpf: disallow empty program 2025-11-10 15:30 ` [PATCH 2/3] bpf: disallow empty program Marat Khalili @ 2025-11-10 16:40 ` Stephen Hemminger 2025-11-10 16:46 ` Marat Khalili 0 siblings, 1 reply; 13+ messages in thread From: Stephen Hemminger @ 2025-11-10 16:40 UTC (permalink / raw) To: Marat Khalili; +Cc: Konstantin Ananyev, dev On Mon, 10 Nov 2025 15:30:45 +0000 Marat Khalili <marat.khalili@huawei.com> wrote: > Add tests for some simple cases: > * Program with no instructions; > * Program with only EXIT instruction but no return value set; > * Program with return value set but no EXIT instruction; > * Minimal valid program with return value set and an EXIT instruction. > > Fix found bugs: > * a program with no instructions was accepted; > * a program with no EXIT instruction read outside the buffer. > > Signed-off-by: Marat Khalili <marat.khalili@huawei.com> Can this just be folded into existing test_bpf file please. ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 2/3] bpf: disallow empty program 2025-11-10 16:40 ` Stephen Hemminger @ 2025-11-10 16:46 ` Marat Khalili 0 siblings, 0 replies; 13+ messages in thread From: Marat Khalili @ 2025-11-10 16:46 UTC (permalink / raw) To: Stephen Hemminger; +Cc: Konstantin Ananyev, dev > Can this just be folded into existing test_bpf file please. Sure; any more issues? :) ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] bpf: make add/subtract one program validate 2025-11-10 15:30 [PATCH 0/3] bpf: simple tests and fixes Marat Khalili 2025-11-10 15:30 ` [PATCH 1/3] bpf: fix signed shift overflows in ARM JIT Marat Khalili 2025-11-10 15:30 ` [PATCH 2/3] bpf: disallow empty program Marat Khalili @ 2025-11-10 15:30 ` Marat Khalili 2 siblings, 0 replies; 13+ messages in thread From: Marat Khalili @ 2025-11-10 15:30 UTC (permalink / raw) To: Konstantin Ananyev; +Cc: Stephen Hemminger, dev Add tests loading simple BPF programs adding or subtracting one to its argument and fix triggered signed integer overflow undefined behaviours: lib/bpf/bpf_validate.c:324:24: runtime error: signed integer overflow: 1 + 9223372036854775807 cannot be represented in type 'long int' lib/bpf/bpf_validate.c:352:24: runtime error: signed integer overflow: -9223372036854775808 - 1 cannot be represented in type 'long int' As a minimal possible fix perform operation on unsigned integers where overflow is well-defined, which was probably the original intent. Signed-off-by: Marat Khalili <marat.khalili@huawei.com> --- app/test/test_bpf_simple.c | 58 ++++++++++++++++++++++++++++++++++++++ lib/bpf/bpf_validate.c | 8 +++--- 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/app/test/test_bpf_simple.c b/app/test/test_bpf_simple.c index 576a6ed029..d4c5bbdc6e 100644 --- a/app/test/test_bpf_simple.c +++ b/app/test/test_bpf_simple.c @@ -129,3 +129,61 @@ test_simple_minimal_working(void) REGISTER_FAST_TEST(bpf_simple_minimal_working_autotest, true, true, test_simple_minimal_working); + +/* + * Try and load valid BPF program adding one to the argument. + */ +static int +test_simple_add_one(void) +{ + static const struct ebpf_insn ins[] = { + { + /* Set return value to one. */ + .code = (EBPF_ALU64 | EBPF_MOV | BPF_K), + .dst_reg = EBPF_REG_0, + .imm = 1, + }, + { + /* Add program argument to the return value. */ + .code = (EBPF_ALU64 | BPF_ADD | BPF_X), + .src_reg = EBPF_REG_1, + .dst_reg = EBPF_REG_0, + }, + { + .code = (BPF_JMP | EBPF_EXIT), + }, + }; + return simple_bpf_load_test(RTE_DIM(ins), ins, 0); +} + +REGISTER_FAST_TEST(bpf_simple_add_one_autotest, true, true, + test_simple_add_one); + +/* + * Try and load valid BPF program subtracting one from the argument. + */ +static int +test_simple_subtract_one(void) +{ + static const struct ebpf_insn ins[] = { + { + /* Subtract one from the program argument. */ + .code = (EBPF_ALU64 | BPF_SUB | BPF_K), + .dst_reg = EBPF_REG_1, + .imm = 1, + }, + { + /* Set return value to the result. */ + .code = (EBPF_ALU64 | EBPF_MOV | BPF_X), + .src_reg = EBPF_REG_1, + .dst_reg = EBPF_REG_0, + }, + { + .code = (BPF_JMP | EBPF_EXIT), + }, + }; + return simple_bpf_load_test(RTE_DIM(ins), ins, 0); +} + +REGISTER_FAST_TEST(bpf_simple_subtract_one_autotest, true, true, + test_simple_subtract_one); diff --git a/lib/bpf/bpf_validate.c b/lib/bpf/bpf_validate.c index 23444b3eaa..47ad6fef0f 100644 --- a/lib/bpf/bpf_validate.c +++ b/lib/bpf/bpf_validate.c @@ -243,8 +243,8 @@ eval_add(struct bpf_reg_val *rd, const struct bpf_reg_val *rs, uint64_t msk) rv.u.min = (rd->u.min + rs->u.min) & msk; rv.u.max = (rd->u.max + rs->u.max) & msk; - rv.s.min = (rd->s.min + rs->s.min) & msk; - rv.s.max = (rd->s.max + rs->s.max) & msk; + rv.s.min = ((uint64_t)rd->s.min + (uint64_t)rs->s.min) & msk; + rv.s.max = ((uint64_t)rd->s.max + (uint64_t)rs->s.max) & msk; /* * if at least one of the operands is not constant, @@ -272,8 +272,8 @@ eval_sub(struct bpf_reg_val *rd, const struct bpf_reg_val *rs, uint64_t msk) rv.u.min = (rd->u.min - rs->u.max) & msk; rv.u.max = (rd->u.max - rs->u.min) & msk; - rv.s.min = (rd->s.min - rs->s.max) & msk; - rv.s.max = (rd->s.max - rs->s.min) & msk; + rv.s.min = ((uint64_t)rd->s.min - (uint64_t)rs->s.max) & msk; + rv.s.max = ((uint64_t)rd->s.max - (uint64_t)rs->s.min) & msk; /* * if at least one of the operands is not constant, -- 2.43.0 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2025-11-12 5:24 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-11-10 15:30 [PATCH 0/3] bpf: simple tests and fixes Marat Khalili 2025-11-10 15:30 ` [PATCH 1/3] bpf: fix signed shift overflows in ARM JIT Marat Khalili 2025-11-11 6:25 ` Jerin Jacob 2025-11-11 7:53 ` Morten Brørup 2025-11-11 10:10 ` Marat Khalili 2025-11-11 16:29 ` Jerin Jacob 2025-11-11 16:31 ` Jerin Jacob 2025-11-11 16:39 ` Marat Khalili 2025-11-12 5:23 ` Jerin Jacob 2025-11-10 15:30 ` [PATCH 2/3] bpf: disallow empty program Marat Khalili 2025-11-10 16:40 ` Stephen Hemminger 2025-11-10 16:46 ` Marat Khalili 2025-11-10 15:30 ` [PATCH 3/3] bpf: make add/subtract one program validate Marat Khalili
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).