Sparse warns that a cast in rv_s_insn truncates bits from the constant
0x7ff to 0xff. The warning originates from the use of a constant offset
of -8 in a store instruction in bpf_jit_comp64.c:
emit(rv_sd(RV_REG_SP, -8, RV_REG_RA), &ctx);
rv_sd then calls rv_s_insn, with imm11_0 equal to (u16)(-8), or 0xfff8.
Here's the current implementation of rv_s_insn:
static inline u32 rv_s_insn(u16 imm11_0, u8 rs2, u8 rs1, u8 funct3, u8 opcode)
{
u8 imm11_5 = imm11_0 >> 5, imm4_0 = imm11_0 & 0x1f;
return (imm11_5 << 25) | (rs2 << 20) | (rs1 << 15) | (funct3 << 12) |
(imm4_0 << 7) | opcode;
}
imm11_0 is a signed 12-bit immediate offset of the store instruction. The
instruction encoding requires splitting the immediate into bits 11:5 and
bits 4:0. In this case, imm11_0 >> 5 = 0x7ff, which then gets truncated
to 0xff when cast to u8, causing the warning from sparse. However, this is
not actually an issue because the immediate offset is signed---truncating
upper bits that are all set to 1 has no effect on the value of the
immediate.
There is another subtle quirk with this code, which is imm11_5 is
supposed to be the upper 7 bits of the 12-bit signed immediate, but its
type is u8 with no explicit mask to select out only the bottom 7 bits.
This happens to be okay here because imm11_5 is the left-most field in
the instruction and the "extra" bit will be shifted out when imm11_5 is
shifted left by 25.
This commit fixes the warning by changing the type of imm11_5 and imm4_0
to be u32 instead of u8, and adding an explicit mask to compute imm11_5
instead of relying on truncation + shifting.
Reported-by: kernel test robot <[email protected]>
Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
In-Reply-To: <[email protected]>
Signed-off-by: Luke Nelson <[email protected]>
Cc: Xi Wang <[email protected]>
Cc: Daniel Borkmann <[email protected]>
---
arch/riscv/net/bpf_jit.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h
index 2717f5490428..e159c6e3ff43 100644
--- a/arch/riscv/net/bpf_jit.h
+++ b/arch/riscv/net/bpf_jit.h
@@ -238,7 +238,7 @@ static inline u32 rv_i_insn(u16 imm11_0, u8 rs1, u8 funct3, u8 rd, u8 opcode)
static inline u32 rv_s_insn(u16 imm11_0, u8 rs2, u8 rs1, u8 funct3, u8 opcode)
{
- u8 imm11_5 = imm11_0 >> 5, imm4_0 = imm11_0 & 0x1f;
+ u32 imm11_5 = (imm11_0 >> 5) & 0x7f, imm4_0 = imm11_0 & 0x1f;
return (imm11_5 << 25) | (rs2 << 20) | (rs1 << 15) | (funct3 << 12) |
(imm4_0 << 7) | opcode;
--
2.34.1
>> static inline u32 rv_s_insn(u16 imm11_0, u8 rs2, u8 rs1, u8 funct3, u8 opcode)
>> {
>> - u8 imm11_5 = imm11_0 >> 5, imm4_0 = imm11_0 & 0x1f;
>> + u32 imm11_5 = (imm11_0 >> 5) & 0x7f, imm4_0 = imm11_0 & 0x1f;
>
> Hi Luke,
>
> keep u8 and add 0x7f explicit mask should work. I ran the repro case and it can silence the warning.
>
>>
>> return (imm11_5 << 25) | (rs2 << 20) | (rs1 << 15) | (funct3 << 12) |
>> (imm4_0 << 7) | opcode;
That does fix the warning, but I think explicitly declaring imm11_5
as u32 is more clear here than the current code which relies on
implicit promotion of imm11_5 from u8 to signed int in the expression
(imm11_5 << 25).
Because of the promotion to signed int, (imm11_5 << 25) is technically
signed overflow and undefined behavior whenever the shift changes
the value in the sign bit. In practice, this isn't an issue; both
because the kernel is compiled with -fno-strict-overflow, but also
because GCC documentation explicitly states that "GCC does not use
the latitude given in C99 and C11 only to treat certain aspects of
signed '<<' as undefined" [1].
Though it may not be an issue in practice, since I'm touching this
line anyways to fix the warning, I think it makes sense to update
the type of imm11_5 to be u32 at the same time.
> Nit: maybe use "riscv, bpf" for the subject will look nice for the riscv-bpf git log tree.
Sure, I can send out a new revision with an updated subject line.
- Luke
[1]: https://gcc.gnu.org/onlinedocs/gcc/Integers-implementation.html
Luke Nelson <[email protected]> writes:
> Sparse warns that a cast in rv_s_insn truncates bits from the constant
> 0x7ff to 0xff. The warning originates from the use of a constant offset
> of -8 in a store instruction in bpf_jit_comp64.c:
>
> emit(rv_sd(RV_REG_SP, -8, RV_REG_RA), &ctx);
>
> rv_sd then calls rv_s_insn, with imm11_0 equal to (u16)(-8), or 0xfff8.
>
> Here's the current implementation of rv_s_insn:
>
> static inline u32 rv_s_insn(u16 imm11_0, u8 rs2, u8 rs1, u8 funct3, u8 opcode)
> {
> u8 imm11_5 = imm11_0 >> 5, imm4_0 = imm11_0 & 0x1f;
>
> return (imm11_5 << 25) | (rs2 << 20) | (rs1 << 15) | (funct3 << 12) |
> (imm4_0 << 7) | opcode;
> }
>
> imm11_0 is a signed 12-bit immediate offset of the store instruction. The
> instruction encoding requires splitting the immediate into bits 11:5 and
> bits 4:0. In this case, imm11_0 >> 5 = 0x7ff, which then gets truncated
> to 0xff when cast to u8, causing the warning from sparse. However, this is
> not actually an issue because the immediate offset is signed---truncating
> upper bits that are all set to 1 has no effect on the value of the
> immediate.
>
> There is another subtle quirk with this code, which is imm11_5 is
> supposed to be the upper 7 bits of the 12-bit signed immediate, but its
> type is u8 with no explicit mask to select out only the bottom 7 bits.
> This happens to be okay here because imm11_5 is the left-most field in
> the instruction and the "extra" bit will be shifted out when imm11_5 is
> shifted left by 25.
>
> This commit fixes the warning by changing the type of imm11_5 and imm4_0
> to be u32 instead of u8, and adding an explicit mask to compute imm11_5
> instead of relying on truncation + shifting.
>
> Reported-by: kernel test robot <[email protected]>
> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> In-Reply-To: <[email protected]>
> Signed-off-by: Luke Nelson <[email protected]>
> Cc: Xi Wang <[email protected]>
> Cc: Daniel Borkmann <[email protected]>
Thanks, Luke!
Acked-by: Björn Töpel <[email protected]>