On Thu, Feb 22, 2024 at 09:28:47AM -0800, Alexei Starovoitov wrote: > On Wed, Feb 21, 2024 at 11:17 AM Dave Thaler <dthaler1968@xxxxxxxxxxxxxx> wrote: > > > > -BPF_CALL 0x8 0x0 call helper function by address BPF_JMP | BPF_K only, see `Helper functions`_ > > +BPF_CALL 0x8 0x0 call_by_address(imm) BPF_JMP | BPF_K only > > +BPF_CALL 0x8 0x0 call_by_address(dst) BPF_JMP | BPF_X only > > ... > > > +* call_by_address(value) means to call a helper function by the address specified by 'value' (see `Helper functions`_ for details) > > > Sorry, we're not going to take this path in the kernel verifier. > I understand that you went with this semantics in PREVAIL verifier, > but this is user space and I suspect once PREVAIL folks realize > that it's not that useful you will change that. > User space has a luxury to change. The kernel doesn't > and we won't be able to change such things in the standard either. > > Essentially what you're proposing is to treat > callx dst_reg > as calling any of the existing helpers by a number. > Let's look at the first ~6: > id = 1 void *bpf_map_lookup_elem(struct bpf_map *map, const void *key) > id = 2 long bpf_map_update_elem(struct bpf_map *map, const void *key, > const void *value, u64 flags) > ... > id = 6 long bpf_trace_printk(const char *fmt, u32 fmt_size, ...) > > They have almost nothing in common. > In C that would be an indirect call of "long (*fn)(...)" > just call anything and hope it works. > This is not useful in practice. > > Also commit log is wrong: > > > Only src=0 is currently listed for callx. Neither clang nor gcc > > use src=1 or src=2, and both use exactly the same semantics for > > src=0 which was agreed between them (Yonghong and Jose). > > this is not at all what gcc and clang are doing. > They emit "callx dst_reg" when they need to compile a normal indirect call > which address is in dst_reg. > It's the real address of the function and not a helper ID. > > Hence these two: > > +BPF_CALL 0x8 0x0 call_by_address(imm) BPF_JMP | BPF_K only > > +BPF_CALL 0x8 0x0 call_by_address(dst) BPF_JMP | BPF_X only > > are not correct. > call imm is a call of helper with a given ID. > callx dst_reg is a call of a function by its real address. > > This is _prelminary_ definition of callx dst_reg from compiler pov, > but there is no implementation of it in the kernel, so > it's way too early to hard code such semantics in the standard. Dave -- are you OK with us just reserving the semantics for all callx instructions, including src=0? At this point I think it's probably just best for us to boot the whole thing to an extension. I'm happy to send a patch for that if you agree (or please feel free to send a v5 of this series which just reserves the group). Thanks, David
Attachment:
signature.asc
Description: PGP signature
