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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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;
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
http://www.tessares.net
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