Re: [PATCH bpf-next v3 2/6] bpf/verifier: refactor checks for range computation

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Sat, Apr 27, 2024 at 3:51 PM Cupertino Miranda
<cupertino.miranda@oracle.com> wrote:
>
>
> Alexei Starovoitov writes:
>
> > On Fri, Apr 26, 2024 at 9:12 AM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> >>
> >> On Fri, Apr 26, 2024 at 3:20 AM Cupertino Miranda
> >> <cupertino.miranda@oracle.com> wrote:
> >> >
> >> >
> >> > Andrii Nakryiko writes:
> >> >
> >> > > On Wed, Apr 24, 2024 at 3:41 PM Cupertino Miranda
> >> > > <cupertino.miranda@oracle.com> wrote:
> >> > >>
> >> > >> Split range computation checks in its own function, isolating pessimitic
> >> > >> range set for dst_reg and failing return to a single point.
> >> > >>
> >> > >> Signed-off-by: Cupertino Miranda <cupertino.miranda@oracle.com>
> >> > >> Cc: Yonghong Song <yonghong.song@linux.dev>
> >> > >> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> >> > >> Cc: David Faust <david.faust@oracle.com>
> >> > >> Cc: Jose Marchesi <jose.marchesi@oracle.com>
> >> > >> Cc: Elena Zannoni <elena.zannoni@oracle.com>
> >> > >> ---
> >> > >>  kernel/bpf/verifier.c | 141 +++++++++++++++++++++++-------------------
> >> > >>  1 file changed, 77 insertions(+), 64 deletions(-)
> >> > >>
> >> > >
> >> > > I know you are moving around pre-existing code, so a bunch of nits
> >> > > below are to pre-existing code, but let's use this as an opportunity
> >> > > to clean it up a bit.
> >> > >
> >> > >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >> > >> index 6fe641c8ae33..829a12d263a5 100644
> >> > >> --- a/kernel/bpf/verifier.c
> >> > >> +++ b/kernel/bpf/verifier.c
> >> > >> @@ -13695,6 +13695,82 @@ static void scalar_min_max_arsh(struct bpf_reg_state *dst_reg,
> >> > >>         __update_reg_bounds(dst_reg);
> >> > >>  }
> >> > >>
> >> > >> +static bool is_const_reg_and_valid(struct bpf_reg_state reg, bool alu32,
> >> > >
> >> > > hm.. why passing reg_state by value? Use pointer?
> >> > >
> >> > Someone mentioned this in a review already and I forgot to change it.
> >> > Apologies if I did not reply on this.
> >> >
> >> > The reason why I pass by value, is more of an approach to programming.
> >> > I do it as guarantee to the caller that there is no mutation of
> >> > the value.
> >> > If it is better or worst from a performance point of view it is
> >> > arguable, since although it might appear to copy the value it also provides
> >> > more information to the compiler of the intent of the callee function,
> >> > allowing it to optimize further.
> >> > I personally would leave the copy by value, but I understand if you want
> >> > to keep having the same code style.
> >>
> >> It's a pretty big 120-byte structure, so maybe the compiler can
> >> optimize it very well, but I'd still be concerned. Hopefully it can
> >> optimize well even with (const) pointer, if inlining.
> >>
> >> But I do insist, if you look at (most? I haven't checked every single
> >> function, of course) other uses in verifier.c, we pass things like
> >> that by pointer. I understand the desire to specify the intent to not
> >> modify it, but that's why you are passing `const struct bpf_reg_state
> >> *reg`, so I think you don't lose anything with that.
> Well, the const will only guard the pointer from mutating, not the data
> pointed by it.

I didn't propose marking pointer const, but mark pointee type as const:

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 4e474ef44e9c..de2bc6fa15da 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -363,12 +363,14 @@ __printf(2, 3) static void verbose(void
*private_data, const char *fmt, ...)
 }

 static void verbose_invalid_scalar(struct bpf_verifier_env *env,
-                                  struct bpf_reg_state *reg,
+                                  const struct bpf_reg_state *reg,
                                   struct bpf_retval_range range,
const char *ctx,
                                   const char *reg_name)
 {
        bool unknown = true;

+       reg->smin_value = 0x1234;
+
        verbose(env, "%s the register %s has", ctx, reg_name);
        if (reg->smin_value > S64_MIN) {
                verbose(env, " smin=%lld", reg->smin_value);

$ make

...

/data/users/andriin/linux/kernel/bpf/verifier.c: In function
‘verbose_invalid_scalar’:
/data/users/andriin/linux/kernel/bpf/verifier.c:372:25: error:
assignment of member ‘smin_value’ in read-only object
  372 |         reg->smin_value = 0x1234;
      |                         ^

...

Works as it logically should.

>
> >
> > +1
> > that "struct bpf_reg_state src_reg" code was written 7 years ago
> > when bpf_reg_state was small.
> > We definitely need to fix it. It might even bring
> > a noticeable runtime improvement.
>
> I forgot to reply to Andrii.
>
> I will change the function prototype to pass the pointer instead.
> In any case, please allow me to express my concerns once again, and
> explain why I do it.
>
> As a general practice, I personally will only copy a pointer to a
> function if there is the intent to allow the function to change the
> content of the pointed data.

I'm not sure why you have this preconception that passing something by
pointer is only for mutation. C language has a straightforward way to
express "this is not going to be changed" with const. You can
circumvent this, of course, but that's an entirely different story.

>
> In my understanding, it is easier for the compiler to optimize both the
> caller and the callee when there are less side-effects from that
> function call such as a possible memory clobbering.
>
> Since these particular functions are leaf functions (not calling anywhere),
> it should be relatively easy for the compiler to infer that the actual
> copy is not needed and will likely just inline those calls, resulting in
> lots of code being eliminated, which will remove any apparent copies.
>
> I checked the asm file for verifier.c and everything below
> adjust_scalar_min_max_vals including itself is inlined, making it
> totally irrelevant if you copy the data or the pointer, since the
> compiler will identify that the content refers to the same data and all
> copies will be classified and removed as dead-code.
>
> All the pointer passing in any context in verifier.c, to my eyes, is more
> of a software defect then a virtue.
> When there is an actual proven benefit, I am all for it, but not in all
> cases.
>
> I had to express my concerns on this and will never speak of it again.
> :)
>
> Thanks you all for the reviews. I will prepare a new version on Monday.