include/linux/bpf_verifier.h | 8 ++++---- kernel/bpf/verifier.c | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-)
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
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;
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
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
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)
© 2016 - 2024 Red Hat, Inc.