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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fd51d0bb-8908-ede1-6d7a-37ed82badebf@fb.com>
Date:   Sun, 10 Jul 2022 09:59:55 -0700
From:   Yonghong Song <yhs@...com>
To:     Matthieu Baerts <matthieu.baerts@...sares.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Song Liu <song@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Stanislav Fomichev <sdf@...gle.com>,
        Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
        Eduard Zingerman <eddyz87@...il.com>
Cc:     mptcp@...ts.linux.dev, bpf@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next] bpf: fix 'dubious one-bit signed bitfield'
 warnings



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@...sares.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;