diff mbox series

[bpf-next] bpf: avoid uninitialized value in BPF_CORE_READ_BITFIELD

Message ID 20240507113950.28208-1-jose.marchesi@oracle.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [bpf-next] bpf: avoid uninitialized value in BPF_CORE_READ_BITFIELD | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18

Commit Message

Jose E. Marchesi May 7, 2024, 11:39 a.m. UTC 
GCC warns that `val' may be used uninitialized in the
BPF_CORE_READ_BITFIELD macro, defined in bpf_core_read.h as:

	[...]
	unsigned long long val;						      \
	[...]								      \
	switch (__CORE_RELO(s, field, BYTE_SIZE)) {			      \
	case 1: val = *(const unsigned char *)p; break;			      \
	case 2: val = *(const unsigned short *)p; break;		      \
	case 4: val = *(const unsigned int *)p; break;			      \
	case 8: val = *(const unsigned long long *)p; break;		      \
        }       							      \
	[...]
	val;								      \
	}								      \

This patch initializes `val' to zero in order to avoid the warning,
and random values to be used in case __builtin_preserve_field_info
returns unexpected values for BPF_FIELD_BYTE_SIZE.

Tested in bpf-next master.
No regressions.

Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com>
Cc: david.faust@oracle.com
Cc: cupertino.miranda@oracle.com
Cc: Eduard Zingerman <eddyz87@gmail.com>
Cc: Yonghong Song <yonghong.song@linux.dev>
---
 tools/lib/bpf/bpf_core_read.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Yonghong Song May 7, 2024, 4:23 p.m. UTC | #1 
On 5/7/24 4:39 AM, Jose E. Marchesi wrote:
> GCC warns that `val' may be used uninitialized in the
> BPF_CORE_READ_BITFIELD macro, defined in bpf_core_read.h as:
>
> 	[...]
> 	unsigned long long val;						      \
> 	[...]								      \
> 	switch (__CORE_RELO(s, field, BYTE_SIZE)) {			      \
> 	case 1: val = *(const unsigned char *)p; break;			      \
> 	case 2: val = *(const unsigned short *)p; break;		      \
> 	case 4: val = *(const unsigned int *)p; break;			      \
> 	case 8: val = *(const unsigned long long *)p; break;		      \
>          }       							      \
> 	[...]
> 	val;								      \
> 	}								      \
>
> This patch initializes `val' to zero in order to avoid the warning,
> and random values to be used in case __builtin_preserve_field_info
> returns unexpected values for BPF_FIELD_BYTE_SIZE.

In clang, __builtin_preserve_field_info either returns correct value
or caused compilation error. Do you mean for gcc __builtin_preserve_field_info
might return an unexpected value here?

BTW, your change makes sense to silent this warning. So Ack below.

>
> Tested in bpf-next master.
> No regressions.
>
> Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com>
> Cc: david.faust@oracle.com
> Cc: cupertino.miranda@oracle.com
> Cc: Eduard Zingerman <eddyz87@gmail.com>
> Cc: Yonghong Song <yonghong.song@linux.dev>

Acked-by: Yonghong Song <yonghong.song@linux.dev>

> ---
>   tools/lib/bpf/bpf_core_read.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/bpf_core_read.h b/tools/lib/bpf/bpf_core_read.h
> index b5c7ce5c243a..88d129b5f0a1 100644
> --- a/tools/lib/bpf/bpf_core_read.h
> +++ b/tools/lib/bpf/bpf_core_read.h
> @@ -89,7 +89,7 @@ enum bpf_enum_value_kind {
>    */
>   #define BPF_CORE_READ_BITFIELD(s, field) ({				      \
>   	const void *p = (const void *)s + __CORE_RELO(s, field, BYTE_OFFSET); \
> -	unsigned long long val;						      \
> +	unsigned long long val = 0;					      \
>   									      \
>   	/* This is a so-called barrier_var() operation that makes specified   \
>   	 * variable "a black box" for optimizing compiler.		      \
Andrii Nakryiko May 7, 2024, 4:41 p.m. UTC | #2 
On Tue, May 7, 2024 at 4:40 AM Jose E. Marchesi
<jose.marchesi@oracle.com> wrote:
>
> GCC warns that `val' may be used uninitialized in the
> BPF_CORE_READ_BITFIELD macro, defined in bpf_core_read.h as:
>
>         [...]
>         unsigned long long val;                                               \
>         [...]                                                                 \
>         switch (__CORE_RELO(s, field, BYTE_SIZE)) {                           \
>         case 1: val = *(const unsigned char *)p; break;                       \
>         case 2: val = *(const unsigned short *)p; break;                      \
>         case 4: val = *(const unsigned int *)p; break;                        \
>         case 8: val = *(const unsigned long long *)p; break;                  \
>         }                                                                     \
>         [...]
>         val;                                                                  \
>         }                                                                     \
>
> This patch initializes `val' to zero in order to avoid the warning,
> and random values to be used in case __builtin_preserve_field_info
> returns unexpected values for BPF_FIELD_BYTE_SIZE.
>
> Tested in bpf-next master.
> No regressions.
>
> Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com>
> Cc: david.faust@oracle.com
> Cc: cupertino.miranda@oracle.com
> Cc: Eduard Zingerman <eddyz87@gmail.com>
> Cc: Yonghong Song <yonghong.song@linux.dev>
> ---
>  tools/lib/bpf/bpf_core_read.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/bpf_core_read.h b/tools/lib/bpf/bpf_core_read.h
> index b5c7ce5c243a..88d129b5f0a1 100644
> --- a/tools/lib/bpf/bpf_core_read.h
> +++ b/tools/lib/bpf/bpf_core_read.h
> @@ -89,7 +89,7 @@ enum bpf_enum_value_kind {
>   */
>  #define BPF_CORE_READ_BITFIELD(s, field) ({                                  \
>         const void *p = (const void *)s + __CORE_RELO(s, field, BYTE_OFFSET); \
> -       unsigned long long val;                                               \
> +       unsigned long long val = 0;                                           \

let's add instead `default: val = 0; break;`

as Yonghong mentioned, it's not expected to have invalid byte size
value in the relocation

pw-bot: cr

>                                                                               \
>         /* This is a so-called barrier_var() operation that makes specified   \
>          * variable "a black box" for optimizing compiler.                    \
> --
> 2.30.2
>
>
Jose E. Marchesi May 7, 2024, 6:13 p.m. UTC | #3 
> On 5/7/24 4:39 AM, Jose E. Marchesi wrote:
>> GCC warns that `val' may be used uninitialized in the
>> BPF_CORE_READ_BITFIELD macro, defined in bpf_core_read.h as:
>>
>> 	[...]
>> 	unsigned long long val;						      \
>> 	[...]								      \
>> 	switch (__CORE_RELO(s, field, BYTE_SIZE)) {			      \
>> 	case 1: val = *(const unsigned char *)p; break;			      \
>> 	case 2: val = *(const unsigned short *)p; break;		      \
>> 	case 4: val = *(const unsigned int *)p; break;			      \
>> 	case 8: val = *(const unsigned long long *)p; break;		      \
>>          }       							      \
>> 	[...]
>> 	val;								      \
>> 	}								      \
>>
>> This patch initializes `val' to zero in order to avoid the warning,
>> and random values to be used in case __builtin_preserve_field_info
>> returns unexpected values for BPF_FIELD_BYTE_SIZE.
>
> In clang, __builtin_preserve_field_info either returns correct value
> or caused compilation error. Do you mean for gcc __builtin_preserve_field_info
> might return an unexpected value here?

The __builtin_preserve_field_info implementation in GCC will emit an
error if the size of the bitfield is not a power of two.  It doesn't
check that the bitfield is 64-bit or smaller, but that should not be a
problem.

So I would say we are ok there.

> BTW, your change makes sense to silent this warning. So Ack below.
>
>
>>
>> Tested in bpf-next master.
>> No regressions.
>>
>> Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com>
>> Cc: david.faust@oracle.com
>> Cc: cupertino.miranda@oracle.com
>> Cc: Eduard Zingerman <eddyz87@gmail.com>
>> Cc: Yonghong Song <yonghong.song@linux.dev>
>
> Acked-by: Yonghong Song <yonghong.song@linux.dev>
>
>> ---
>>   tools/lib/bpf/bpf_core_read.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/lib/bpf/bpf_core_read.h b/tools/lib/bpf/bpf_core_read.h
>> index b5c7ce5c243a..88d129b5f0a1 100644
>> --- a/tools/lib/bpf/bpf_core_read.h
>> +++ b/tools/lib/bpf/bpf_core_read.h
>> @@ -89,7 +89,7 @@ enum bpf_enum_value_kind {
>>    */
>>   #define BPF_CORE_READ_BITFIELD(s, field) ({				      \
>>   	const void *p = (const void *)s + __CORE_RELO(s, field, BYTE_OFFSET); \
>> -	unsigned long long val;						      \
>> +	unsigned long long val = 0;					      \
>>   									      \
>>   	/* This is a so-called barrier_var() operation that makes specified   \
>>   	 * variable "a black box" for optimizing compiler.		      \
Jose E. Marchesi May 7, 2024, 6:14 p.m. UTC | #4 
> On Tue, May 7, 2024 at 4:40 AM Jose E. Marchesi
> <jose.marchesi@oracle.com> wrote:
>>
>> GCC warns that `val' may be used uninitialized in the
>> BPF_CORE_READ_BITFIELD macro, defined in bpf_core_read.h as:
>>
>>         [...]
>>         unsigned long long val;                                               \
>>         [...]                                                                 \
>>         switch (__CORE_RELO(s, field, BYTE_SIZE)) {                           \
>>         case 1: val = *(const unsigned char *)p; break;                       \
>>         case 2: val = *(const unsigned short *)p; break;                      \
>>         case 4: val = *(const unsigned int *)p; break;                        \
>>         case 8: val = *(const unsigned long long *)p; break;                  \
>>         }                                                                     \
>>         [...]
>>         val;                                                                  \
>>         }                                                                     \
>>
>> This patch initializes `val' to zero in order to avoid the warning,
>> and random values to be used in case __builtin_preserve_field_info
>> returns unexpected values for BPF_FIELD_BYTE_SIZE.
>>
>> Tested in bpf-next master.
>> No regressions.
>>
>> Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com>
>> Cc: david.faust@oracle.com
>> Cc: cupertino.miranda@oracle.com
>> Cc: Eduard Zingerman <eddyz87@gmail.com>
>> Cc: Yonghong Song <yonghong.song@linux.dev>
>> ---
>>  tools/lib/bpf/bpf_core_read.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/lib/bpf/bpf_core_read.h b/tools/lib/bpf/bpf_core_read.h
>> index b5c7ce5c243a..88d129b5f0a1 100644
>> --- a/tools/lib/bpf/bpf_core_read.h
>> +++ b/tools/lib/bpf/bpf_core_read.h
>> @@ -89,7 +89,7 @@ enum bpf_enum_value_kind {
>>   */
>>  #define BPF_CORE_READ_BITFIELD(s, field) ({                                  \
>>         const void *p = (const void *)s + __CORE_RELO(s, field, BYTE_OFFSET); \
>> -       unsigned long long val;                                               \
>> +       unsigned long long val = 0;                                           \
>
> let's add instead `default: val = 0; break;`
>
> as Yonghong mentioned, it's not expected to have invalid byte size
> value in the relocation

Ok.  I will send a V2 with that change.

> pw-bot: cr
>
>>                                                                               \
>>         /* This is a so-called barrier_var() operation that makes specified   \
>>          * variable "a black box" for optimizing compiler.                    \
>> --
>> 2.30.2
>>
>>
diff mbox series

Patch

diff --git a/tools/lib/bpf/bpf_core_read.h b/tools/lib/bpf/bpf_core_read.h
index b5c7ce5c243a..88d129b5f0a1 100644
--- a/tools/lib/bpf/bpf_core_read.h
+++ b/tools/lib/bpf/bpf_core_read.h
@@ -89,7 +89,7 @@  enum bpf_enum_value_kind {
  */
 #define BPF_CORE_READ_BITFIELD(s, field) ({				      \
 	const void *p = (const void *)s + __CORE_RELO(s, field, BYTE_OFFSET); \
-	unsigned long long val;						      \
+	unsigned long long val = 0;					      \
 									      \
 	/* This is a so-called barrier_var() operation that makes specified   \
 	 * variable "a black box" for optimizing compiler.		      \