Andrii Nakryiko writes:
> On Sat, Apr 27, 2024 at 3:51 PM Cupertino Miranda
> <cupertino.miranda@xxxxxxxxxx> wrote:
>>
>>
>> Alexei Starovoitov writes:
>>
>> > On Fri, Apr 26, 2024 at 9:12 AM Andrii Nakryiko
>> > <andrii.nakryiko@xxxxxxxxx> wrote:
>> >>
>> >> On Fri, Apr 26, 2024 at 3:20 AM Cupertino Miranda
>> >> <cupertino.miranda@xxxxxxxxxx> wrote:
>> >> >
>> >> >
>> >> > Andrii Nakryiko writes:
>> >> >
>> >> > > On Wed, Apr 24, 2024 at 3:41 PM Cupertino Miranda
>> >> > > <cupertino.miranda@xxxxxxxxxx> 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@xxxxxxxxxx>
>> >> > >> Cc: Yonghong Song <yonghong.song@xxxxxxxxx>
>> >> > >> Cc: Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx>
>> >> > >> Cc: David Faust <david.faust@xxxxxxxxxx>
>> >> > >> Cc: Jose Marchesi <jose.marchesi@xxxxxxxxxx>
>> >> > >> Cc: Elena Zannoni <elena.zannoni@xxxxxxxxxx>
>> >> > >> ---
>> >> > >> 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.
>
Your right, pointer is better. I should have validated that myself.
Apologies for the noise. Please disregard all I said.
>>
>> >
>> > +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.
