* [PATCH bpf-next] bpf: avoid uninitialized value in BPF_CORE_READ_BITFIELD
@ 2024-05-07 11:39 Jose E. Marchesi
2024-05-07 16:23 ` Yonghong Song
2024-05-07 16:41 ` Andrii Nakryiko
0 siblings, 2 replies; 5+ messages in thread
From: Jose E. Marchesi @ 2024-05-07 11:39 UTC (permalink / raw
To: bpf
Cc: Jose E . Marchesi, david.faust, cupertino.miranda,
Eduard Zingerman, Yonghong Song
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; \
\
/* This is a so-called barrier_var() operation that makes specified \
* variable "a black box" for optimizing compiler. \
--
2.30.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] bpf: avoid uninitialized value in BPF_CORE_READ_BITFIELD
2024-05-07 11:39 [PATCH bpf-next] bpf: avoid uninitialized value in BPF_CORE_READ_BITFIELD Jose E. Marchesi
@ 2024-05-07 16:23 ` Yonghong Song
2024-05-07 18:13 ` Jose E. Marchesi
2024-05-07 16:41 ` Andrii Nakryiko
1 sibling, 1 reply; 5+ messages in thread
From: Yonghong Song @ 2024-05-07 16:23 UTC (permalink / raw
To: Jose E. Marchesi, bpf; +Cc: david.faust, cupertino.miranda, Eduard Zingerman
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. \
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] bpf: avoid uninitialized value in BPF_CORE_READ_BITFIELD
2024-05-07 11:39 [PATCH bpf-next] bpf: avoid uninitialized value in BPF_CORE_READ_BITFIELD Jose E. Marchesi
2024-05-07 16:23 ` Yonghong Song
@ 2024-05-07 16:41 ` Andrii Nakryiko
2024-05-07 18:14 ` Jose E. Marchesi
1 sibling, 1 reply; 5+ messages in thread
From: Andrii Nakryiko @ 2024-05-07 16:41 UTC (permalink / raw
To: Jose E. Marchesi
Cc: bpf, david.faust, cupertino.miranda, Eduard Zingerman,
Yonghong Song
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
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] bpf: avoid uninitialized value in BPF_CORE_READ_BITFIELD
2024-05-07 16:23 ` Yonghong Song
@ 2024-05-07 18:13 ` Jose E. Marchesi
0 siblings, 0 replies; 5+ messages in thread
From: Jose E. Marchesi @ 2024-05-07 18:13 UTC (permalink / raw
To: Yonghong Song; +Cc: bpf, david.faust, cupertino.miranda, Eduard Zingerman
> 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. \
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] bpf: avoid uninitialized value in BPF_CORE_READ_BITFIELD
2024-05-07 16:41 ` Andrii Nakryiko
@ 2024-05-07 18:14 ` Jose E. Marchesi
0 siblings, 0 replies; 5+ messages in thread
From: Jose E. Marchesi @ 2024-05-07 18:14 UTC (permalink / raw
To: Andrii Nakryiko
Cc: bpf, david.faust, cupertino.miranda, Eduard Zingerman,
Yonghong Song
> 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
>>
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-05-07 18:14 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-07 11:39 [PATCH bpf-next] bpf: avoid uninitialized value in BPF_CORE_READ_BITFIELD Jose E. Marchesi
2024-05-07 16:23 ` Yonghong Song
2024-05-07 18:13 ` Jose E. Marchesi
2024-05-07 16:41 ` Andrii Nakryiko
2024-05-07 18:14 ` Jose E. Marchesi
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 read-only IMAP folder(s) and NNTP newsgroup(s).