netdev - Re: [PATCH/RFC bpf-next 01/16] bpf: turn "enum bpf_reg

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: <20190327154410.zpeeal6nh6qbvg7f@ast-mbp>
Date:   Wed, 27 Mar 2019 08:44:12 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Jiong Wang <jiong.wang@...ronome.com>
Cc:     daniel@...earbox.net, bpf@...r.kernel.org, netdev@...r.kernel.org,
        oss-drivers@...ronome.com
Subject: Re: [PATCH/RFC bpf-next 01/16] bpf: turn "enum bpf_reg_liveness"
 into bit representation

On Tue, Mar 26, 2019 at 06:05:24PM +0000, Jiong Wang wrote:
> "enum bpf_reg_liveness" is actually used as bit instead of integer. For
> example:
> 
>         if (live & (REG_LIVE_READ | REG_LIVE_WRITTEN | REG_LIVE_DONE))
> 
> Using enum to represent bit is error prone, better to use explicit bit
> macros.
> 
> Signed-off-by: Jiong Wang <jiong.wang@...ronome.com>
> ---
>  include/linux/bpf_verifier.h | 16 +++++++++-------
>  kernel/bpf/verifier.c        |  5 ++---
>  2 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 7d8228d..f03c86a 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -34,12 +34,14 @@
>   * but of the link between it and its parent.  See mark_reg_read() and
>   * mark_stack_slot_read() in kernel/bpf/verifier.c.
>   */
> -enum bpf_reg_liveness {
> -	REG_LIVE_NONE = 0, /* reg hasn't been read or written this branch */
> -	REG_LIVE_READ, /* reg was read, so we're sensitive to initial value */
> -	REG_LIVE_WRITTEN, /* reg was written first, screening off later reads */
> -	REG_LIVE_DONE = 4, /* liveness won't be updating this register anymore */
> -};

yes. it is enum that is used as a bitfield, but I prefer to keep it as enum
because clang -Wassign-enum can do at least some type checking.
I also find it easier to review the code when it has
'enum bpf_reg_liveness' instead of 'u8'