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: <CAEf4BzbA+mB7ZU-eBCWg+JCXXHYLmqRH995F7QrRMRX4nD3fcQ@mail.gmail.com>
Date:   Sat, 20 Mar 2021 10:13:00 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <ast@...com>
Cc:     Martin KaFai Lau <kafai@...com>, 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 Fri, Mar 19, 2021 at 5:10 PM Alexei Starovoitov <ast@...com> wrote:
>
> 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.

I think code structure should reflect current logic, not be in
preparation for further potential extension, which might not even
happen. If there are only A and B possible, then code should make it
as clear as possible. But if we anticipate another case C, then

if (A) {
    handleA;
    continue;
}
if (B) {
    handle B;
    continue;
}
return -EINVAL;

Is still easier to follow and is easy to extend.

My original point was that `if () {} else if () {}` code structure
implies that there is or might be some common handling logic after
if/else, so at least my brain constantly worries about that and jumps
around in the code to validate that there isn't actually anything
else. And that gets progressively more harder with longer or more
complicated logic inside handleA and handleB.

Anyways, I'm not trying to enforce my personal style, I tried to show
that it's objectively superior from my brain's point of view. That
`continue` is "a pruning point", if you will. But I'm not trying to
convert anyone. Please proceed with whatever code structure you feel
is better.