netdev - Re: [PATCH bpf-next 03/15] bpf: Refactor btf_check_func_arg

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: <8de72618-22fc-ba88-686b-301e46f40dd3@fb.com>
Date:   Fri, 19 Mar 2021 17:10:21 -0700
From:   Alexei Starovoitov <ast@...com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Martin KaFai Lau <kafai@...com>
CC:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>,
        Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next 03/15] bpf: Refactor btf_check_func_arg_match

On 3/19/21 2:51 PM, Andrii Nakryiko wrote:
> 
> It's a matter of taste, I suppose. I'd probably disagree with you on
> the readability of those verifier parts ;) So it's up to you, of
> course, but for me this code pattern:
> 
> for (...) {
>      if (A) {
>          handleA;
>      } else if (B) {
>          handleB;
>      } else {
>          return -EINVAL;
>      }
> }
> 
> is much harder to follow than more linear (imo)
> 
> for (...) {
>      if (A) {
>          handleA;
>          continue;
>      }
> 
>      if (!B)
>          return -EINVAL;
> 
>      handleB;
> }
> 
> especially if handleA and handleB are quite long and complicated.
> Because I have to jump back and forth to validate that C is not
> allowed/handled later, and that there is no common subsequent logic
> for both A and B (or even C). In the latter code pattern there are
> clear "only A" and "only B" logic and it's quite obvious that no C is
> allowed/handled.

my .02. I like the former (Martin's case) much better than the later.
We had few patterns like the later in the past and had to turn them
into the former because "case C" appeared.
In other words:
if (A)
else if (B)
else
   return

is much easier to extend for C and later convert to 'switch' with 'D':
less code churn, easier to refactor.