[bpf-next] riscv/bpf: Fix truncated immediate warning in rv_s_insn

Commit Message

Luke Nelson July 27, 2023, 2:49 a.m. UTC

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 <[e-mail]>
Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/
In-Reply-To: <[e-mail]>
Signed-off-by: Luke Nelson <[e-mail]>
Cc: Xi Wang <[e-mail]>
Cc: Daniel Borkmann <[e-mail]>
---
 arch/riscv/net/bpf_jit.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
2.34.1

Comments

Pu Lehui July 27, 2023, 7:42 a.m. UTC | #1

On 2023/7/27 10:49, Luke Nelson wrote:
> 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 <[e-mail]>
> Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/
> In-Reply-To: <[e-mail]>
> Signed-off-by: Luke Nelson <[e-mail]>
> Cc: Xi Wang <[e-mail]>
> Cc: Daniel Borkmann <[e-mail]>
> ---
>   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;

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;
> --
> 2.34.1
> 
>

Pu Lehui July 27, 2023, 8:08 a.m. UTC | #2

Nit: maybe use "riscv, bpf" for the subject will look nice for the 
riscv-bpf git log tree.

On 2023/7/27 10:49, Luke Nelson wrote:
> 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 <[e-mail]>
> Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/
> In-Reply-To: <[e-mail]>
> Signed-off-by: Luke Nelson <[e-mail]>
> Cc: Xi Wang <[e-mail]>
> Cc: Daniel Borkmann <[e-mail]>
> ---
>   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
> 
>

Luke Nelson Aug. 8, 2023, 4:08 p.m. UTC | #3

>>  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

Pu Lehui Aug. 9, 2023, 2:32 a.m. UTC | #4

On 2023/8/9 0:08, Luke Nelson wrote:
> 
>>>   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;
> 

Hi Luke,

Thank for more detailed explanation.

> 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.
> 

Agree. But this inconsistency looks weird, i.e. imm11_5 change to u32 
while rs2, rs1 and funct3 still u8. Anyway, our primary goal is to 
silence the sparse warning, and the current patch looks good to me. 
Let's go ahead.

Feel free to add:
Reviewed-by: Pu Lehui <[e-mail]>

>> 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

Björn Töpel Aug. 14, 2023, 12:34 p.m. UTC | #5

Luke Nelson <[e-mail]> 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 <[e-mail]>
> Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/
> In-Reply-To: <[e-mail]>
> Signed-off-by: Luke Nelson <[e-mail]>
> Cc: Xi Wang <[e-mail]>
> Cc: Daniel Borkmann <[e-mail]>


Thanks, Luke!

Acked-by: Björn Töpel <[e-mail]>

Patch

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;