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

[Cupertino Miranda <cupertino.miranda@oracle.com>]

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.

>
> +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.

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.