* [PATCH bpf-next] bpf: fix 'dubious one-bit signed bitfield' warnings
@ 2022-07-10 8:35 Matthieu Baerts
2022-07-10 10:22 ` bpf: fix 'dubious one-bit signed bitfield' warnings: Tests Results MPTCP CI
2022-07-10 16:59 ` [PATCH bpf-next] bpf: fix 'dubious one-bit signed bitfield' warnings Yonghong Song
0 siblings, 2 replies; 5+ messages in thread
From: Matthieu Baerts @ 2022-07-10 8:35 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Eduard Zingerman
Cc: mptcp, Matthieu Baerts, bpf, linux-kernel
Our CI[1] reported these warnings when using Sparse:
$ touch net/mptcp/bpf.c
$ make C=1 net/mptcp/bpf.o
net/mptcp/bpf.c: note: in included file:
include/linux/bpf_verifier.h:348:26: error: dubious one-bit signed bitfield
include/linux/bpf_verifier.h:349:29: error: dubious one-bit signed bitfield
These two fields from the new 'bpf_loop_inline_state' structure are used
as booleans. Instead of declaring two 'unsigned int', we can declare
them as 'bool'.
While at it, also set 'state->initialized' to 'true' instead of '1' to
make it clearer it is linked to a 'bool' type.
[1] https://github.com/multipath-tcp/mptcp_net-next/actions/runs/2643588487
Fixes: 1ade23711971 ("bpf: Inline calls to bpf_loop when callback is known")
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
include/linux/bpf_verifier.h | 8 ++++----
kernel/bpf/verifier.c | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 81b19669efba..2ac424641cc3 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -345,10 +345,10 @@ struct bpf_verifier_state_list {
};
struct bpf_loop_inline_state {
- int initialized:1; /* set to true upon first entry */
- int fit_for_inline:1; /* true if callback function is the same
- * at each call and flags are always zero
- */
+ bool initialized; /* set to true upon first entry */
+ bool fit_for_inline; /* true if callback function is the same
+ * at each call and flags are always zero
+ */
u32 callback_subprogno; /* valid when fit_for_inline is true */
};
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 328cfab3af60..4fa49d852a59 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7144,7 +7144,7 @@ static void update_loop_inline_state(struct bpf_verifier_env *env, u32 subprogno
struct bpf_loop_inline_state *state = &cur_aux(env)->loop_inline_state;
if (!state->initialized) {
- state->initialized = 1;
+ state->initialized = true;
state->fit_for_inline = loop_flag_is_zero(env);
state->callback_subprogno = subprogno;
return;
--
2.36.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: bpf: fix 'dubious one-bit signed bitfield' warnings: Tests Results
2022-07-10 8:35 [PATCH bpf-next] bpf: fix 'dubious one-bit signed bitfield' warnings Matthieu Baerts
@ 2022-07-10 10:22 ` MPTCP CI
2022-07-10 16:59 ` [PATCH bpf-next] bpf: fix 'dubious one-bit signed bitfield' warnings Yonghong Song
1 sibling, 0 replies; 5+ messages in thread
From: MPTCP CI @ 2022-07-10 10:22 UTC (permalink / raw)
To: Matthieu Baerts; +Cc: mptcp
Hi Matthieu,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal:
- Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
- Task: https://cirrus-ci.com/task/4727107317137408
- Summary: https://api.cirrus-ci.com/v1/artifact/task/4727107317137408/summary/summary.txt
- KVM Validation: debug:
- Success! ✅:
- Task: https://cirrus-ci.com/task/5853007223980032
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5853007223980032/summary/summary.txt
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/d7dfacbb3f1c
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-debug
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] bpf: fix 'dubious one-bit signed bitfield' warnings
2022-07-10 8:35 [PATCH bpf-next] bpf: fix 'dubious one-bit signed bitfield' warnings Matthieu Baerts
2022-07-10 10:22 ` bpf: fix 'dubious one-bit signed bitfield' warnings: Tests Results MPTCP CI
@ 2022-07-10 16:59 ` Yonghong Song
2022-07-10 20:19 ` Matthieu Baerts
1 sibling, 1 reply; 5+ messages in thread
From: Yonghong Song @ 2022-07-10 16:59 UTC (permalink / raw)
To: Matthieu Baerts, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Eduard Zingerman
Cc: mptcp, bpf, linux-kernel
On 7/10/22 1:35 AM, Matthieu Baerts wrote:
> Our CI[1] reported these warnings when using Sparse:
>
> $ touch net/mptcp/bpf.c
> $ make C=1 net/mptcp/bpf.o
> net/mptcp/bpf.c: note: in included file:
> include/linux/bpf_verifier.h:348:26: error: dubious one-bit signed bitfield
> include/linux/bpf_verifier.h:349:29: error: dubious one-bit signed bitfield
>
> These two fields from the new 'bpf_loop_inline_state' structure are used
> as booleans. Instead of declaring two 'unsigned int', we can declare
> them as 'bool'.
>
> While at it, also set 'state->initialized' to 'true' instead of '1' to
> make it clearer it is linked to a 'bool' type.
>
> [1] https://github.com/multipath-tcp/mptcp_net-next/actions/runs/2643588487
>
> Fixes: 1ade23711971 ("bpf: Inline calls to bpf_loop when callback is known")
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
> include/linux/bpf_verifier.h | 8 ++++----
> kernel/bpf/verifier.c | 2 +-
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 81b19669efba..2ac424641cc3 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -345,10 +345,10 @@ struct bpf_verifier_state_list {
> };
>
> struct bpf_loop_inline_state {
> - int initialized:1; /* set to true upon first entry */
> - int fit_for_inline:1; /* true if callback function is the same
> - * at each call and flags are always zero
> - */
> + bool initialized; /* set to true upon first entry */
> + bool fit_for_inline; /* true if callback function is the same
> + * at each call and flags are always zero
> + */
I think changing 'int' to 'unsigned' is a better alternative for
potentially adding more bitfields in the future. This is also a pattern
for many other kernel data structures.
> u32 callback_subprogno; /* valid when fit_for_inline is true */
> };
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 328cfab3af60..4fa49d852a59 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -7144,7 +7144,7 @@ static void update_loop_inline_state(struct bpf_verifier_env *env, u32 subprogno
> struct bpf_loop_inline_state *state = &cur_aux(env)->loop_inline_state;
>
> if (!state->initialized) {
> - state->initialized = 1;
> + state->initialized = true;
> state->fit_for_inline = loop_flag_is_zero(env);
> state->callback_subprogno = subprogno;
> return;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] bpf: fix 'dubious one-bit signed bitfield' warnings
2022-07-10 16:59 ` [PATCH bpf-next] bpf: fix 'dubious one-bit signed bitfield' warnings Yonghong Song
@ 2022-07-10 20:19 ` Matthieu Baerts
2022-07-11 0:38 ` Yonghong Song
0 siblings, 1 reply; 5+ messages in thread
From: Matthieu Baerts @ 2022-07-10 20:19 UTC (permalink / raw)
To: Yonghong Song, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Eduard Zingerman
Cc: mptcp, bpf, linux-kernel
Hi Yonghong,
Thank you for the review!
On 10/07/2022 18:59, Yonghong Song wrote:> On 7/10/22 1:35 AM, Matthieu
Baerts wrote:
>> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
>> index 81b19669efba..2ac424641cc3 100644
>> --- a/include/linux/bpf_verifier.h
>> +++ b/include/linux/bpf_verifier.h
>> @@ -345,10 +345,10 @@ struct bpf_verifier_state_list {
>> };
>> struct bpf_loop_inline_state {
>> - int initialized:1; /* set to true upon first entry */
>> - int fit_for_inline:1; /* true if callback function is the same
>> - * at each call and flags are always zero
>> - */
>> + bool initialized; /* set to true upon first entry */
>> + bool fit_for_inline; /* true if callback function is the same
>> + * at each call and flags are always zero
>> + */
>
> I think changing 'int' to 'unsigned' is a better alternative for
> potentially adding more bitfields in the future. This is also a pattern
> for many other kernel data structures.
There was room, I was not sure if it would be OK but I saw 'bool' were
often used in structures from this bpf_verifier.h file.
I can of course switch to an unsigned one. I would have picked 'u8' when
looking at the structures around but any preferences from you?
'unsigned', 'unsigned int', 'u8', 'u32'?
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next] bpf: fix 'dubious one-bit signed bitfield' warnings
2022-07-10 20:19 ` Matthieu Baerts
@ 2022-07-11 0:38 ` Yonghong Song
0 siblings, 0 replies; 5+ messages in thread
From: Yonghong Song @ 2022-07-11 0:38 UTC (permalink / raw)
To: Matthieu Baerts, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Eduard Zingerman
Cc: mptcp, bpf, linux-kernel
On 7/10/22 1:19 PM, Matthieu Baerts wrote:
> Hi Yonghong,
>
> Thank you for the review!
>
> On 10/07/2022 18:59, Yonghong Song wrote:> On 7/10/22 1:35 AM, Matthieu
> Baerts wrote:
>>> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
>>> index 81b19669efba..2ac424641cc3 100644
>>> --- a/include/linux/bpf_verifier.h
>>> +++ b/include/linux/bpf_verifier.h
>>> @@ -345,10 +345,10 @@ struct bpf_verifier_state_list {
>>> };
>>> struct bpf_loop_inline_state {
>>> - int initialized:1; /* set to true upon first entry */
>>> - int fit_for_inline:1; /* true if callback function is the same
>>> - * at each call and flags are always zero
>>> - */
>>> + bool initialized; /* set to true upon first entry */
>>> + bool fit_for_inline; /* true if callback function is the same
>>> + * at each call and flags are always zero
>>> + */
>>
>> I think changing 'int' to 'unsigned' is a better alternative for
>> potentially adding more bitfields in the future. This is also a pattern
>> for many other kernel data structures.
>
> There was room, I was not sure if it would be OK but I saw 'bool' were
> often used in structures from this bpf_verifier.h file.
>
> I can of course switch to an unsigned one. I would have picked 'u8' when
> looking at the structures around but any preferences from you?
> 'unsigned', 'unsigned int', 'u8', 'u32'?
The original data structure is
struct bpf_loop_inline_state {
int initialized:1; /* set to true upon first entry */
int fit_for_inline:1; /* true if callback function is the same
* at each call and flags are always zero
*/
u32 callback_subprogno; /* valid when fit_for_inline is true */
};
So 'initialized' and 'fit_for_inline' and additional padding will take
4 bytes, so 'unsigned', 'unsigned int', 'u32' should be appropriate
here. Later, if people want to add a u8 or u16 to utilize the padding,
the type of 'initialized' and 'fit_for_inlined' might be changed to
u8 or u16.
For which of 'unsigned', 'unsigned int', 'u32', checking with
$ [~/work/bpf-next/include/linux] grep ":1" *.h
both 'unsigned' and 'unsigned int' are used in many places. I don't have
a preference. I saw one instance 'unsigned int' is used in this file,
so 'unsigned int' should be okay here.
>
> Cheers,
> Matt
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-07-11 0:39 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-10 8:35 [PATCH bpf-next] bpf: fix 'dubious one-bit signed bitfield' warnings Matthieu Baerts
2022-07-10 10:22 ` bpf: fix 'dubious one-bit signed bitfield' warnings: Tests Results MPTCP CI
2022-07-10 16:59 ` [PATCH bpf-next] bpf: fix 'dubious one-bit signed bitfield' warnings Yonghong Song
2022-07-10 20:19 ` Matthieu Baerts
2022-07-11 0:38 ` Yonghong Song
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).