Re: [Bpf] [PATCH bpf-next v4] bpf, docs: Add callx instructions in new conformance group

From: David Vernet <void@manifault.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Dave Thaler <dthaler1968@googlemail.com>,
	"Jose E. Marchesi" <jose.marchesi@oracle.com>,
	Yonghong Song <yonghong.song@linux.dev>,
	bpf <bpf@vger.kernel.org>,
	bpf@ietf.org, Dave Thaler <dthaler1968@gmail.com>
Subject: Re: [Bpf] [PATCH bpf-next v4] bpf, docs: Add callx instructions in new conformance group
Date: Fri, 23 Feb 2024 13:33:47 -0600	[thread overview]
Message-ID: <20240223193347.GA2026@maniforge> (raw)
In-Reply-To: <CAADnVQJq0aG2kF2KN1SCM9cZtRLqxKG=UkF=5-XWjFBbvLZhhQ@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2905 bytes --]

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@googlemail.com> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]