From: "Morten Brørup" <mb@smartsharesystems.com>
To: "Marat Khalili" <marat.khalili@huawei.com>,
<stephen@networkplumber.org>, <jerinjacobk@gmail.com>,
"Wathsala Vithanage" <wathsala.vithanage@arm.com>,
"Konstantin Ananyev" <konstantin.ananyev@huawei.com>
Cc: <dev@dpdk.org>, <stable@dpdk.org>
Subject: RE: [PATCH v2 2/5] bpf: fix signed shift overflows in ARM JIT
Date: Wed, 17 Dec 2025 10:49:57 +0100 [thread overview]
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35F655F1@smartserver.smartshare.dk> (raw)
In-Reply-To: <20251216182036.77869-3-marat.khalili@huawei.com>
> From: Marat Khalili [mailto:marat.khalili@huawei.com]
> Sent: Tuesday, 16 December 2025 19.21
>
> 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).
Yes, multiple online sources [1] confirms that the bool type can only hold two values, 0 and 1, and converting any non-zero value to bool results in 1.
So, using !!x where x is a bool is literally a no-op, since x - being a bool - could already only hold the values 0 and 1.
[1]: E.g.
https://www.gnu.org/software/c-intro-and-ref/manual/html_node/Boolean-Type.html
https://embedded.fm/blog/2016/5/3/ew-bools
> 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 | 162 ++++++++++++++++++++--------------------
> 1 file changed, 81 insertions(+), 81 deletions(-)
>
> diff --git a/lib/bpf/bpf_jit_arm64.c b/lib/bpf/bpf_jit_arm64.c
> index 96b8cd2e03..76df1e4ba1 100644
> --- a/lib/bpf/bpf_jit_arm64.c
> +++ b/lib/bpf/bpf_jit_arm64.c
> @@ -28,7 +28,7 @@
> #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))
>
> struct ebpf_a64_map {
> uint32_t off; /* eBPF to arm64 insn offset mapping for jump */
> @@ -238,12 +238,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 = RTE_SHIFT_VAL32(is64, 31);
> + insn |= RTE_SHIFT_VAL32(sub, 30);
As mentioned above, !!is64 and !!sub are literally no-ops, so it is safe to remove them.
> insn |= 0x11000000;
> insn |= rd;
> - insn |= rn << 5;
> - insn |= imm << 10;
> + insn |= RTE_SHIFT_VAL32(rn, 5);
> + insn |= RTE_SHIFT_VAL32(imm, 10);
>
> emit_insn(ctx, insn,
> check_reg(rd) || check_reg(rn) || check_imm(12, imm12));
> @@ -279,16 +279,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 = RTE_SHIFT_VAL32(load, 22);
> + insn |= RTE_SHIFT_VAL32(pre_index, 24);
> insn |= 0xa8800000;
This constant has the high bit set, consider:
insn |= UINT32_C(0xa8800000);
And for consistency, consider also doing it for constants where the high bit isn't set.
Throughout the file.
> insn |= rt;
> - insn |= rn << 5;
> - insn |= rt2 << 10;
> + insn |= RTE_SHIFT_VAL32(rn, 5);
> + insn |= RTE_SHIFT_VAL32(rt2, 10);
> if (push)
> - insn |= 0x7e << 15; /* 0x7e means -2 with imm7 */
> + insn |= RTE_SHIFT_VAL32(0x7e, 15); /* 0x7e means -2 with
> imm7 */
> else
> - insn |= 0x2 << 15;
> + insn |= RTE_SHIFT_VAL32(0x2, 15);
>
> emit_insn(ctx, insn, check_reg(rn) || check_reg(rt) ||
> check_reg(rt2));
>
> @@ -317,11 +317,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 = RTE_SHIFT_VAL32(is64, 31);
> + insn |= RTE_SHIFT_VAL32(type, 29);
> + insn |= RTE_SHIFT_VAL32(0x25, 23);
> + insn |= RTE_SHIFT_VAL32(shift/16, 21);
> + insn |= RTE_SHIFT_VAL32(imm16, 5);
> insn |= rd;
>
> emit_insn(ctx, insn, check_reg(rd) || check_mov_hw(is64, shift));
> @@ -334,7 +334,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 +393,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 = RTE_SHIFT_VAL32(0x1c1, 21);
> if (load)
> - insn |= 1 << 22;
> + insn |= RTE_BIT32(22);
> if (sz == BPF_B)
> - insn |= 0 << 30;
> + insn |= RTE_SHIFT_VAL32(0, 30);
> else if (sz == BPF_H)
> - insn |= 1 << 30;
> + insn |= RTE_SHIFT_VAL32(1, 30);
> else if (sz == BPF_W)
> - insn |= 2 << 30;
> + insn |= RTE_SHIFT_VAL32(2, 30);
> else if (sz == EBPF_DW)
> - insn |= 3 << 30;
> + insn |= RTE_SHIFT_VAL32(3, 30);
>
> - insn |= rm << 16;
> - insn |= 0x1a << 10; /* LSL and S = 0 */
> - insn |= rn << 5;
> + insn |= RTE_SHIFT_VAL32(rm, 16);
> + insn |= RTE_SHIFT_VAL32(0x1a, 10); /* LSL and S = 0 */
> + insn |= RTE_SHIFT_VAL32(rn, 5);
> insn |= rt;
>
> emit_insn(ctx, insn, check_reg(rt) || check_reg(rn) ||
> check_reg(rm) ||
> @@ -436,10 +436,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 = RTE_SHIFT_VAL32(is64, 31);
> + insn |= RTE_SHIFT_VAL32(op, 21); /* shift == 0 */
> + insn |= RTE_SHIFT_VAL32(rm, 16);
> + insn |= RTE_SHIFT_VAL32(rn, 5);
> insn |= rd;
>
> emit_insn(ctx, insn, check_reg(rd) || check_reg(rm));
> @@ -468,11 +468,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 = RTE_SHIFT_VAL32(is64, 31);
> + insn |= RTE_SHIFT_VAL32(0xd8, 21);
> + insn |= RTE_SHIFT_VAL32(rm, 16);
> + insn |= RTE_SHIFT_VAL32(A64_ZR, 10);
> + insn |= RTE_SHIFT_VAL32(rd, 5);
> insn |= rd;
>
> emit_insn(ctx, insn, check_reg(rd) || check_reg(rm));
> @@ -489,11 +489,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 = RTE_SHIFT_VAL32(is64, 31);
> + insn |= RTE_SHIFT_VAL32(0xd6, 21);
> + insn |= RTE_SHIFT_VAL32(rm, 16);
> + insn |= RTE_SHIFT_VAL32(op, 10);
> + insn |= RTE_SHIFT_VAL32(rn, 5);
> insn |= rd;
>
> emit_insn(ctx, insn, check_reg(rd) || check_reg(rm));
> @@ -532,14 +532,14 @@ emit_bitfield(struct a64_jit_ctx *ctx, bool is64,
> uint8_t rd, uint8_t rn,
> {
> uint32_t insn;
>
> - insn = (!!is64) << 31;
> + insn = RTE_SHIFT_VAL32(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 |= RTE_SHIFT_VAL32(op, 29);
> + insn |= RTE_SHIFT_VAL32(0x26, 23);
> + insn |= RTE_SHIFT_VAL32(immr, 16);
> + insn |= RTE_SHIFT_VAL32(imms, 10);
> + insn |= RTE_SHIFT_VAL32(rn, 5);
> insn |= rd;
>
> emit_insn(ctx, insn, check_reg(rd) || check_reg(rn) ||
> @@ -578,11 +578,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 = RTE_SHIFT_VAL32(is64, 31);
> + insn |= RTE_SHIFT_VAL32(op, 29);
> + insn |= RTE_SHIFT_VAL32(0x50, 21);
> + insn |= RTE_SHIFT_VAL32(rm, 16);
> + insn |= RTE_SHIFT_VAL32(rd, 5);
> insn |= rd;
>
> emit_insn(ctx, insn, check_reg(rd) || check_reg(rm));
> @@ -612,12 +612,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 = RTE_SHIFT_VAL32(is64, 31);
> + insn |= RTE_SHIFT_VAL32(0xd8, 21);
> + insn |= RTE_SHIFT_VAL32(rm, 16);
> + insn |= RTE_SHIFT_VAL32(0x1, 15);
> + insn |= RTE_SHIFT_VAL32(ra, 10);
> + insn |= RTE_SHIFT_VAL32(rn, 5);
> insn |= rd;
>
> emit_insn(ctx, insn, check_reg(rd) || check_reg(rn) ||
> check_reg(rm) ||
> @@ -638,7 +638,7 @@ emit_blr(struct a64_jit_ctx *ctx, uint8_t rn)
> uint32_t insn;
>
> insn = 0xd63f0000;
> - insn |= rn << 5;
> + insn |= RTE_SHIFT_VAL32(rn, 5);
>
> emit_insn(ctx, insn, check_reg(rn));
> }
> @@ -669,22 +669,22 @@ emit_rev(struct a64_jit_ctx *ctx, uint8_t rd,
> int32_t imm)
> uint32_t insn;
>
> insn = 0xdac00000;
> - insn |= rd << 5;
> + insn |= RTE_SHIFT_VAL32(rd, 5);
> insn |= rd;
>
> switch (imm) {
> case 16:
> - insn |= 1 << 10;
> + insn |= RTE_SHIFT_VAL32(1, 10);
> emit_insn(ctx, insn, check_reg(rd));
> emit_zero_extend(ctx, rd, 16);
> break;
> case 32:
> - insn |= 2 << 10;
> + insn |= RTE_SHIFT_VAL32(2, 10);
> emit_insn(ctx, insn, check_reg(rd));
> /* Upper 32 bits already cleared */
> break;
> case 64:
> - insn |= 3 << 10;
> + insn |= RTE_SHIFT_VAL32(3, 10);
> emit_insn(ctx, insn, check_reg(rd));
> break;
> default:
> @@ -933,9 +933,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 = RTE_SHIFT_VAL32(is64, 31);
> + insn |= RTE_SHIFT_VAL32(0x35, 24);
> + insn |= RTE_SHIFT_VAL32(imm, 5);
> insn |= rt;
>
> emit_insn(ctx, insn, check_reg(rt) || check_imm(19, imm19));
> @@ -947,7 +947,7 @@ emit_b(struct a64_jit_ctx *ctx, int32_t imm26)
> uint32_t insn, imm;
>
> imm = mask_imm(26, imm26);
> - insn = 0x5 << 26;
> + insn = RTE_SHIFT_VAL32(0x5, 26);
> insn |= imm;
>
> emit_insn(ctx, insn, check_imm(26, imm26));
> @@ -971,9 +971,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 |= RTE_SHIFT_VAL32(is64, 30);
> + insn |= RTE_SHIFT_VAL32(rs, 16);
> + insn |= RTE_SHIFT_VAL32(rn, 5);
>
> emit_insn(ctx, insn, check_reg(rs) || check_reg(rn));
> }
> @@ -984,8 +984,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 |= RTE_SHIFT_VAL32(is64, 30);
> + insn |= RTE_SHIFT_VAL32(rn, 5);
> insn |= rt;
>
> emit_insn(ctx, insn, check_reg(rt) || check_reg(rn));
> @@ -998,9 +998,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 |= RTE_SHIFT_VAL32(is64, 30);
> + insn |= RTE_SHIFT_VAL32(rs, 16);
> + insn |= RTE_SHIFT_VAL32(rn, 5);
> insn |= rt;
>
> emit_insn(ctx, insn, check_reg(rs) || check_reg(rt) ||
> check_reg(rn));
> @@ -1051,9 +1051,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 |= RTE_SHIFT_VAL32(is64, 31);
> + insn |= RTE_SHIFT_VAL32(rm, 16);
> + insn |= RTE_SHIFT_VAL32(rn, 5);
>
> emit_insn(ctx, insn, check_reg(rn) || check_reg(rm));
> }
> @@ -1076,8 +1076,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 = RTE_SHIFT_VAL32(0x15, 26);
> + insn |= RTE_SHIFT_VAL32(imm, 5);
> insn |= cond;
>
> emit_insn(ctx, insn, check_cond(cond) || check_imm(19, imm19));
> @@ -1301,7 +1301,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 = RTE_SHIFT_VAL64(ins[1].imm, 32) |
> (uint32_t)imm;
> emit_mov_imm(ctx, 1, dst, u64);
> i++;
> break;
> --
> 2.43.0
With or without suggested changes,
Acked-by: Morten Brørup <mb@smartsharesystems.com>
next prev parent reply other threads:[~2025-12-17 9:50 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20251110153046.63518-1-marat.khalili@huawei.com>
2025-12-16 18:20 ` [PATCH v2 0/5] bpf: simple tests and fixes Marat Khalili
2025-12-16 18:20 ` [PATCH v2 1/5] eal: variable first arguments of RTE_SHIFT_VALxx Marat Khalili
2025-12-17 9:25 ` Morten Brørup
2025-12-16 18:20 ` [PATCH v2 2/5] bpf: fix signed shift overflows in ARM JIT Marat Khalili
2025-12-17 9:49 ` Morten Brørup [this message]
2025-12-16 18:20 ` [PATCH v2 3/5] bpf: disallow empty program Marat Khalili
2025-12-18 0:54 ` Stephen Hemminger
2025-12-17 8:58 ` Marat Khalili
2025-12-16 18:20 ` [PATCH v2 4/5] bpf: make add/subtract one program validate Marat Khalili
2025-12-16 18:20 ` [PATCH v2 5/5] bpf: fix BPF validation w/ conditional jump first Marat Khalili
2025-12-17 18:01 ` [PATCH v3 0/6] bpf: simple tests and fixes Marat Khalili
2025-12-17 18:01 ` [PATCH v3 1/6] eal: variable first arguments of RTE_SHIFT_VALxx Marat Khalili
2025-12-17 18:01 ` [PATCH v3 2/6] bpf: fix signed shift overflows in ARM JIT Marat Khalili
2025-12-17 18:01 ` [PATCH v3 3/6] bpf: mark ARM opcodes with UINT32_C Marat Khalili
2025-12-17 18:01 ` [PATCH v3 4/6] bpf: disallow empty program Marat Khalili
2025-12-17 18:01 ` [PATCH v3 5/6] bpf: make add/subtract one program validate Marat Khalili
2025-12-17 18:01 ` [PATCH v3 6/6] bpf: fix BPF validation w/ conditional jump first Marat Khalili
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=98CBD80474FA8B44BF855DF32C47DC35F655F1@smartserver.smartshare.dk \
--to=mb@smartsharesystems.com \
--cc=dev@dpdk.org \
--cc=jerinjacobk@gmail.com \
--cc=konstantin.ananyev@huawei.com \
--cc=marat.khalili@huawei.com \
--cc=stable@dpdk.org \
--cc=stephen@networkplumber.org \
--cc=wathsala.vithanage@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).