diff mbox series

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

Message ID 20240221191725.17586-1-dthaler1968@gmail.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series [bpf-next,v4] bpf, docs: Add callx instructions in new conformance group | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 20 maintainers not CCed: daniel@iogearbox.net john.fastabend@gmail.com nathan@kernel.org morbo@google.com kpsingh@kernel.org llvm@lists.linux.dev ast@kernel.org jolsa@kernel.org linux-doc@vger.kernel.org yonghong.song@linux.dev andrii@kernel.org song@kernel.org martin.lau@linux.dev sdf@google.com justinstitt@google.com void@manifault.com corbet@lwn.net eddyz87@gmail.com ndesaulniers@google.com haoluo@google.com
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch warning WARNING: From:/Signed-off-by: email address mismatch: 'From: Dave Thaler <dthaler1968@googlemail.com>' != 'Signed-off-by: Dave Thaler <dthaler1968@gmail.com>'
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-7 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-8 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-10 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-PR fail merge-conflict

Commit Message

Dave Thaler Feb. 21, 2024, 7:17 p.m. UTC 
* Add a "callx" conformance group
* Add callx row to table
* Update helper function to section to be agnostic between BPF_K vs
  BPF_X
* Rename "legacy" conformance group to "packet"

Based on mailing list discussion at
https://mailarchive.ietf.org/arch/msg/bpf/l5tNEgL-Wo7qSEuaGssOl5VChKk/

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). Since src=0
semantics are agreed upon by both and is already implemented, src=0
is documented as implemented.

v1->v2: Incorporated feedback from Will Hawkins

v2->v3: Use "dst" not "imm" field

v3->v4: Only use src=0

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
---
 .../bpf/standardization/instruction-set.rst   | 29 ++++++++++++-------
 1 file changed, 18 insertions(+), 11 deletions(-)

Comments

David Vernet Feb. 21, 2024, 9:18 p.m. UTC | #1 
On Wed, Feb 21, 2024 at 11:17:25AM -0800, Dave Thaler wrote:
> * Add a "callx" conformance group
> * Add callx row to table
> * Update helper function to section to be agnostic between BPF_K vs
>   BPF_X
> * Rename "legacy" conformance group to "packet"
> 
> Based on mailing list discussion at
> https://mailarchive.ietf.org/arch/msg/bpf/l5tNEgL-Wo7qSEuaGssOl5VChKk/
> 
> 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). Since src=0
> semantics are agreed upon by both and is already implemented, src=0
> is documented as implemented.

If the semantics for src=0 were already decided for both clang and gcc,
then this seems fine to me. Agreed as well with leaving src > 0 for
later, as Alexei said on the v3 thread. We can decide how to best deal
with indirect calls at a later time.

Alexei -- is this acceptable?

> v1->v2: Incorporated feedback from Will Hawkins
> 
> v2->v3: Use "dst" not "imm" field
> 
> v3->v4: Only use src=0
> 
> Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
> ---
>  .../bpf/standardization/instruction-set.rst   | 29 ++++++++++++-------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/bpf/standardization/instruction-set.rst b/Documentation/bpf/standardization/instruction-set.rst
> index bdfe0cd0e..a68445899 100644
> --- a/Documentation/bpf/standardization/instruction-set.rst
> +++ b/Documentation/bpf/standardization/instruction-set.rst
> @@ -127,7 +127,7 @@ This document defines the following conformance groups:
>  * divmul32: includes 32-bit division, multiplication, and modulo instructions.
>  * divmul64: includes divmul32, plus 64-bit division, multiplication,
>    and modulo instructions.
> -* legacy: deprecated packet access instructions.
> +* packet: deprecated packet access instructions.
>  
>  Instruction encoding
>  ====================
> @@ -404,9 +404,10 @@ BPF_JSET  0x4    any  PC += offset if dst & src
>  BPF_JNE   0x5    any  PC += offset if dst != src
>  BPF_JSGT  0x6    any  PC += offset if dst > src        signed
>  BPF_JSGE  0x7    any  PC += offset if dst >= src       signed
> -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
>  BPF_CALL  0x8    0x1  call PC += imm                   BPF_JMP | BPF_K only, see `Program-local functions`_
> -BPF_CALL  0x8    0x2  call helper function by BTF ID   BPF_JMP | BPF_K only, see `Helper functions`_
> +BPF_CALL  0x8    0x2  call_by_btfid(imm)               BPF_JMP | BPF_K only
>  BPF_EXIT  0x9    0x0  return                           BPF_JMP | BPF_K only
>  BPF_JLT   0xa    any  PC += offset if dst < src        unsigned
>  BPF_JLE   0xb    any  PC += offset if dst <= src       unsigned
> @@ -414,6 +415,11 @@ BPF_JSLT  0xc    any  PC += offset if dst < src        signed
>  BPF_JSLE  0xd    any  PC += offset if dst <= src       signed
>  ========  =====  ===  ===============================  =============================================
>  
> +where
> +
> +* call_by_address(value) means to call a helper function by the address specified by 'value' (see `Helper functions`_ for details)
> +* call_by_btfid(value) means to call a helper function by the BTF ID specified by 'value' (see `Helper functions`_ for details)
> +
>  The BPF program needs to store the return value into register R0 before doing a
>  ``BPF_EXIT``.
>  
> @@ -438,8 +444,9 @@ specified by the 'imm' field. A > 16-bit conditional jump may be
>  converted to a < 16-bit conditional jump plus a 32-bit unconditional
>  jump.
>  
> -All ``BPF_CALL`` and ``BPF_JA`` instructions belong to the
> -base32 conformance group.
> +The ``BPF_CALL | BPF_X`` instruction belongs to the callx
> +conformance group.  All other ``BPF_CALL`` instructions and all
> +``BPF_JA`` instructions belong to the base32 conformance group.
>  
>  Helper functions
>  ~~~~~~~~~~~~~~~~
> @@ -447,13 +454,13 @@ Helper functions
>  Helper functions are a concept whereby BPF programs can call into a
>  set of function calls exposed by the underlying platform.
>  
> -Historically, each helper function was identified by an address
> -encoded in the imm field.  The available helper functions may differ
> -for each program type, but address values are unique across all program types.
> +Historically, each helper function was identified by an address.
> +The available helper functions may differ for each program type,
> +but address values are unique across all program types.
>  
>  Platforms that support the BPF Type Format (BTF) support identifying
> -a helper function by a BTF ID encoded in the imm field, where the BTF ID
> -identifies the helper name and type.
> +a helper function by a BTF ID, where the BTF ID identifies the helper
> +name and type.
>  
>  Program-local functions
>  ~~~~~~~~~~~~~~~~~~~~~~~
> @@ -660,4 +667,4 @@ carried over from classic BPF. These instructions used an instruction
>  class of BPF_LD, a size modifier of BPF_W, BPF_H, or BPF_B, and a
>  mode modifier of BPF_ABS or BPF_IND.  However, these instructions are
>  deprecated and should no longer be used.  All legacy packet access
> -instructions belong to the "legacy" conformance group.
> +instructions belong to the "packet" conformance group.
> -- 
> 2.40.1
> 
> -- 
> Bpf mailing list
> Bpf@ietf.org
> https://www.ietf.org/mailman/listinfo/bpf
Alexei Starovoitov Feb. 22, 2024, 5:28 p.m. UTC | #2 
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.

pw-bot: cr
David Vernet Feb. 23, 2024, 7:33 p.m. UTC | #3 
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
diff mbox series

Patch

diff --git a/Documentation/bpf/standardization/instruction-set.rst b/Documentation/bpf/standardization/instruction-set.rst
index bdfe0cd0e..a68445899 100644
--- a/Documentation/bpf/standardization/instruction-set.rst
+++ b/Documentation/bpf/standardization/instruction-set.rst
@@ -127,7 +127,7 @@  This document defines the following conformance groups:
 * divmul32: includes 32-bit division, multiplication, and modulo instructions.
 * divmul64: includes divmul32, plus 64-bit division, multiplication,
   and modulo instructions.
-* legacy: deprecated packet access instructions.
+* packet: deprecated packet access instructions.
 
 Instruction encoding
 ====================
@@ -404,9 +404,10 @@  BPF_JSET  0x4    any  PC += offset if dst & src
 BPF_JNE   0x5    any  PC += offset if dst != src
 BPF_JSGT  0x6    any  PC += offset if dst > src        signed
 BPF_JSGE  0x7    any  PC += offset if dst >= src       signed
-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
 BPF_CALL  0x8    0x1  call PC += imm                   BPF_JMP | BPF_K only, see `Program-local functions`_
-BPF_CALL  0x8    0x2  call helper function by BTF ID   BPF_JMP | BPF_K only, see `Helper functions`_
+BPF_CALL  0x8    0x2  call_by_btfid(imm)               BPF_JMP | BPF_K only
 BPF_EXIT  0x9    0x0  return                           BPF_JMP | BPF_K only
 BPF_JLT   0xa    any  PC += offset if dst < src        unsigned
 BPF_JLE   0xb    any  PC += offset if dst <= src       unsigned
@@ -414,6 +415,11 @@  BPF_JSLT  0xc    any  PC += offset if dst < src        signed
 BPF_JSLE  0xd    any  PC += offset if dst <= src       signed
 ========  =====  ===  ===============================  =============================================
 
+where
+
+* call_by_address(value) means to call a helper function by the address specified by 'value' (see `Helper functions`_ for details)
+* call_by_btfid(value) means to call a helper function by the BTF ID specified by 'value' (see `Helper functions`_ for details)
+
 The BPF program needs to store the return value into register R0 before doing a
 ``BPF_EXIT``.
 
@@ -438,8 +444,9 @@  specified by the 'imm' field. A > 16-bit conditional jump may be
 converted to a < 16-bit conditional jump plus a 32-bit unconditional
 jump.
 
-All ``BPF_CALL`` and ``BPF_JA`` instructions belong to the
-base32 conformance group.
+The ``BPF_CALL | BPF_X`` instruction belongs to the callx
+conformance group.  All other ``BPF_CALL`` instructions and all
+``BPF_JA`` instructions belong to the base32 conformance group.
 
 Helper functions
 ~~~~~~~~~~~~~~~~
@@ -447,13 +454,13 @@  Helper functions
 Helper functions are a concept whereby BPF programs can call into a
 set of function calls exposed by the underlying platform.
 
-Historically, each helper function was identified by an address
-encoded in the imm field.  The available helper functions may differ
-for each program type, but address values are unique across all program types.
+Historically, each helper function was identified by an address.
+The available helper functions may differ for each program type,
+but address values are unique across all program types.
 
 Platforms that support the BPF Type Format (BTF) support identifying
-a helper function by a BTF ID encoded in the imm field, where the BTF ID
-identifies the helper name and type.
+a helper function by a BTF ID, where the BTF ID identifies the helper
+name and type.
 
 Program-local functions
 ~~~~~~~~~~~~~~~~~~~~~~~
@@ -660,4 +667,4 @@  carried over from classic BPF. These instructions used an instruction
 class of BPF_LD, a size modifier of BPF_W, BPF_H, or BPF_B, and a
 mode modifier of BPF_ABS or BPF_IND.  However, these instructions are
 deprecated and should no longer be used.  All legacy packet access
-instructions belong to the "legacy" conformance group.
+instructions belong to the "packet" conformance group.