Re: [PATCH bpf-next 1/2] bpf: add bpf_get_hw_counter kfunc

[Eduard Zingerman <eddyz87@gmail.com>]

On Thu, 2024-10-24 at 14:11 +0100, Vadim Fedorenko wrote:

[...]

> > > @@ -20396,6 +20398,15 @@ static int fixup_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
> > >                     desc->func_id == special_kfunc_list[KF_bpf_rdonly_cast]) {
> > >                  insn_buf[0] = BPF_MOV64_REG(BPF_REG_0, BPF_REG_1);
> > >                  *cnt = 1;
> > > +       } else if (IS_ENABLED(CONFIG_X86) &&
> > 
> > It's better to introduce bpf_jit_inlines_kfunc_call()
> > similar to bpf_jit_inlines_helper_call().
> 
> Yep, I thought about introducing it while adding more architectures, but
> can do it from the beginning.

After thinking a bit more, I think I messed up in a private discussion
with Vadim. It is necessary to introduce bpf_jit_inlines_kfunc_call()
and use it in the mark_fastcall_pattern_for_call(),
otherwise the following situation is possible:

- the program is executed on the arch where inlining for
  bpf_get_hw_counter() is not implemented;
- there is a pattern in the code:

    r1 = *(u64 *)(r10 - 256);
    call bpf_get_hw_fastcall
    *(u64 *)(r10 - 256) = r1;
    ... r10 - 8 is not used ...
    ... r1 is read ...

- mark_fastcall_pattern_for_call() would mark spill and fill as
  members of the pattern;
- fastcall contract is not violated, because reserved stack slots are
  used as expected;
- remove_fastcall_spills_fills() would remove spill and fill:

    call bpf_get_hw_fastcall
    ... r1 is read ...

- since call is not transformed to instructions by a specific jit the
  value of r1 would be clobbered, making the program invalid.
  
Vadim, sorry I did not point this out earlier, I thought that fastcall
contract ensuring logic would handle everything.

[...]