Message-ID: <20201007051335.z6lwkinpsyxmpfam@ast-mbp>
Date:   Tue, 6 Oct 2020 22:13:35 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Daniel Borkmann <daniel@...earbox.net>,
        john fastabend <john.fastabend@...il.com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next 1/3] bpf: Propagate scalar ranges through
 register assignments.

On Tue, Oct 06, 2020 at 09:42:18PM -0700, Andrii Nakryiko wrote:
> > I see it differently.
> > I don't like moving if (reg->id) into find_equal_scalars(). Otherwise it would
> > have to be named something like try_find_equal_scalars(). And even with such
> > "try_" prefix it's still not clean. It's my general dislike of defensive
> > programming. I prefer all functions to be imperative: "do" vs "try_do".
> > There are exception from the rule, of course. Like kfree() that accepts NULL.
> > That's fine.
> > In this case I think if (type == SCALAR && id != 0) should be done by the caller.
> 
> There is no need to do (type == SCALAR) check, see pseudo-code above.
> In all cases where find_equal_scalars() is called we know already that
> register is SCALAR.
> 
> As for `if (reg->id)` being moved inside find_equal_scalars(). I
> didn't mean it as a defensive measure. It just allows to keep
> higher-level logic in check_cond_jmp_op() a bit more linear.
> 
> Also, regarding "try_find_equal_scalars". It's not try/attempt to do
> this, it's do it, similarly to __update_reg_bounds() you explained
> below. It's just known_reg->id == 0 is a guarantee that there are no
> other equal registers, so we can skip the work. But of course one can
> look at this differently. I just prefer less nested ifs, if it's
> possible to avoid them.
> 
> But all this is not that important. I suggested, you declined, let's move on.
> 
> > Note that's different from __update_reg_bounds().
> > There the bounds may or may not change, but the action is performed.
> > What you're proposing it to make find_equal_scalars() accept any kind
> > of register and do the action only if argument is actual scalar
> > and its "id != 0". That's exactly the defensive programming
> > that I feel make programmers sloppier.
> 
> :) I see a little bit of an irony between this anti-defensive
> programming manifesto and "safety net in case id assignment goes
> wrong" above.
> 
> > Note that's not the same as mark_reg_unknown() doing
> > if (WARN_ON(regno >= MAX_BPF_REG)) check. I hope the difference is clear.

Looks like the difference between defensive programming and safety net checks
was not clear. The safety net in mark_reg_unknown() will be triggered when
things really go wrong. I don't think I've ever seen in production code. I only
saw it during the development when my code was badly broken. That check is to
prevent security issues in case a bug sneaks in. The defensive programming lets
a function accept incorrect arguments. That's normal behavior of such function.
Because of such design choice the programers will routinely pass invalid args.
That's kfree() checking for NULL and the only exception I can remember in the
kernel code base. Arguably NULL is not an invalid value in this case. When
people talk about defensive programming the NULL check is brought up as an
example, but I think it's important to understand it at deeper level.
Letting function accept any register only to
> prefer less nested ifs, if it's possible to avoid them
is the same thing. It's making code sloppier for esthetics of less nested if-s.
There are plenty of projects and people that don't mind such coding style and
find it easier to program. That's a disagreement in coding philosophy. It's ok
to disagree, but it's important to understand those coding differences.

Powered by blists - more mailing lists