| Message ID | 20220710083523.1620722-1-matthieu.baerts@tessares.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpf: fix 'dubious one-bit signed bitfield' warnings | expand |
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
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
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;
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(-)