[PATCH bpf-next] bpf: fix 'dubious one-bit signed bitfield' warnings

Matthieu Baerts posted 1 patch 1 year, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/20220710083523.1620722-1-matthieu.baerts@tessares.net
Maintainers: Stanislav Fomichev <sdf@google.com>, Martin KaFai Lau <martin.lau@linux.dev>, KP Singh <kpsingh@kernel.org>, Song Liu <song@kernel.org>, John Fastabend <john.fastabend@gmail.com>, Yonghong Song <yhs@fb.com>, Hao Luo <haoluo@google.com>, Daniel Borkmann <daniel@iogearbox.net>, Jiri Olsa <jolsa@kernel.org>, Andrii Nakryiko <andrii@kernel.org>, Eduard Zingerman <eddyz87@gmail.com>, Alexei Starovoitov <ast@kernel.org>
include/linux/bpf_verifier.h | 8 ++++----
kernel/bpf/verifier.c        | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)
[PATCH bpf-next] bpf: fix 'dubious one-bit signed bitfield' warnings
Posted by Matthieu Baerts 1 year, 8 months ago
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


Re: [PATCH bpf-next] bpf: fix 'dubious one-bit signed bitfield' warnings
Posted by Yonghong Song 1 year, 8 months ago

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;

Re: [PATCH bpf-next] bpf: fix 'dubious one-bit signed bitfield' warnings
Posted by Matthieu Baerts 1 year, 8 months ago
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

Re: [PATCH bpf-next] bpf: fix 'dubious one-bit signed bitfield' warnings
Posted by Yonghong Song 1 year, 8 months ago

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
Re: bpf: fix 'dubious one-bit signed bitfield' warnings: Tests Results
Posted by MPTCP CI 1 year, 8 months ago
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)