mptcp.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [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).