Rationalize parameters to helper functions (#515)

The external helper functions should get 6 parameters no matter whether
the eBPF program is interpeted or JIT'd. The 6 parameter, the one in
addition to the 5 specified by the eBPF standard, is the context
parameter.

Add tests to make sure that the 6th parameter is properly passed to the
external helper functions.

Signed-off-by: Will Hawkins <[email protected]>

custom_tests/data/ubpf_test_default_dispatcher_helper_context.input

@@ -0,0 +1 @@
+85 00 00 00 01 00 00 00 95 00 00 00 00 00 00 00

custom_tests/descrs/ubpf_test_default_dispatcher_helper_context.md

@@ -0,0 +1,4 @@
+##Test Description
+
+    This custom test program tests whether JIT'd and interpreted eBPF programs properly pass the context to external
+        helper functions.

custom_tests/srcs/ubpf_test_default_dispatcher_helper_context.cc

@@ -0,0 +1,81 @@
+// Copyright (c) Will Hawkins
+// SPDX-License-Identifier: Apache-2.0
+
+#include <cstdint>
+#include <iostream>
+#include <memory>
+#include <vector>
+#include <string>
+
+extern "C"
+{
+#include "ubpf.h"
+}
+
+#include "ubpf_custom_test_support.h"
+
+uint64_t
+simple_helper(uint64_t p0, uint64_t p1, uint64_t p2, uint64_t p3, uint64_t p4, void* cookie)
+{
+    UNREFERENCED_PARAMETER(p0);
+    UNREFERENCED_PARAMETER(p1);
+    UNREFERENCED_PARAMETER(p2);
+    UNREFERENCED_PARAMETER(p3);
+    UNREFERENCED_PARAMETER(p4);
+    uint64_t* ccookie = (uint64_t*)cookie;
+    return *ccookie;
+}
+
+int
+main(int argc, char** argv)
+{
+    std::string program_string{};
+    std::string error{};
+    ubpf_jit_fn jit_fn;
+    uint64_t memory{0x123456789};
+
+    // The test program invokes an external function at index 1 (which is invoked via the
+    // default external helper dispatcher). The result of that external function is given as the
+    // result of the eBPF's program execution. Therefore, ...
+    if (!get_program_string(argc, argv, program_string, error)) {
+        std::cerr << error << std::endl;
+        return 1;
+    }
+
+    std::unique_ptr<ubpf_vm, decltype(&ubpf_destroy)> vm(ubpf_create(), ubpf_destroy);
+    if (!ubpf_setup_custom_test(
+            vm,
+            program_string,
+            [](ubpf_vm_up& vm, std::string& error) {
+                if (ubpf_register(vm.get(), 1, "simple helper", as_external_function_t((void*)simple_helper)) < 0) {
+                    error = "Failed to register external helper function at index 1.";
+                    return false;
+                }
+                return true;
+            },
+            jit_fn,
+            error)) {
+        std::cerr << "Problem setting up custom test: " << error << std::endl;
+        return 1;
+    }
+
+    [[maybe_unused]] auto result = jit_fn(&memory, sizeof(uint64_t));
+
+    // ... because of the semantics of external helper functions, the result of the eBPF
+    // program execution should point to the same place to which &memory points.
+    if (result != memory) {
+        std::cerr << "result and memory are not equal (JIT version).\n";
+        return 1;
+    }
+
+    if (ubpf_exec(vm.get(), &memory, sizeof(uint64_t), &result)) {
+        std::cerr << "There was an error interpreting the test program.\n";
+        return 1;
+    }
+
+    if (result != memory) {
+        std::cerr << "result and memory are not equal (interpreter version).\n";
+        return 1;
+    }
+    return 0;
+}

vm/inc/ubpf.h

@@ -130,14 +130,20 @@ extern "C"
     ubpf_set_error_print(struct ubpf_vm* vm, int (*error_printf)(FILE* stream, const char* format, ...));
 
     /**
-     * @brief The type of an external function.
+     * @brief The type of an external helper function.
+     *
+     * Note: There is an implicit <tt>void *</tt>-typed 6th parameter that users can access if they choose. That
+     * sixth parameter's value is the value of the pointer given by the user of \ref ubpf_exec, \ref ubpf_exec_ex, and
+     * the function generated by \ref ubpf_translate as the first argument.
      */
     typedef uint64_t (*external_function_t)(uint64_t p0, uint64_t p1, uint64_t p2, uint64_t p3, uint64_t p4);
 
     /**
-     * @brief Cast an external function to external_function_t
-     * Some external functions may not use all the parameters and, therefore,
-     * not match the external_function_t typedef. Use this for a conversion.
+     * @brief Cast an external function to \ref external_function_t
+     *
+     * Some external functions may not use all the parameters (or may use the implicit
+     * 6th parameter) and, therefore, not match the \ref external_function_t typedef.
+     * Use this for a conversion.
      *
      * @param[in] f The function to cast to match the signature of an
      *              external function.

vm/ubpf_int.h

@@ -29,7 +29,8 @@
 #define UNUSED_PARAMETER(x) ((void)x)
 
 struct ebpf_inst;
-typedef uint64_t (*ext_func)(uint64_t arg0, uint64_t arg1, uint64_t arg2, uint64_t arg3, uint64_t arg4);
+typedef uint64_t (*extended_external_helper_t)(
+    uint64_t arg0, uint64_t arg1, uint64_t arg2, uint64_t arg3, uint64_t arg4, void* cookie);
 
 typedef enum
 {
@@ -63,7 +64,7 @@ struct ubpf_vm
     size_t jitter_buffer_size;
     struct ubpf_jit_result jitted_result;
 
-    ext_func* ext_funcs;
+    extended_external_helper_t* ext_funcs;
     bool* int_funcs;
     const char** ext_func_names;
 
@@ -85,7 +86,12 @@ struct ubpf_vm
         size_t size,
         uint32_t offset);
     bool (*jit_update_helper)(
-        struct ubpf_vm* vm, ext_func new_helper, unsigned int idx, uint8_t* buffer, size_t size, uint32_t offset);
+        struct ubpf_vm* vm,
+        extended_external_helper_t new_helper,
+        unsigned int idx,
+        uint8_t* buffer,
+        size_t size,
+        uint32_t offset);
     int unwind_stack_extension_index;
     uint64_t pointer_secret;
     ubpf_data_relocation data_relocation_function;
@@ -115,7 +121,12 @@ ubpf_jit_update_dispatcher_arm64(
     struct ubpf_vm* vm, external_function_dispatcher_t new_dispatcher, uint8_t* buffer, size_t size, uint32_t offset);
 bool
 ubpf_jit_update_helper_arm64(
-    struct ubpf_vm* vm, ext_func new_helper, unsigned int idx, uint8_t* buffer, size_t size, uint32_t offset);
+    struct ubpf_vm* vm,
+    extended_external_helper_t new_helper,
+    unsigned int idx,
+    uint8_t* buffer,
+    size_t size,
+    uint32_t offset);
 
 // x86_64
 struct ubpf_jit_result
@@ -125,7 +136,12 @@ ubpf_jit_update_dispatcher_x86_64(
     struct ubpf_vm* vm, external_function_dispatcher_t new_dispatcher, uint8_t* buffer, size_t size, uint32_t offset);
 bool
 ubpf_jit_update_helper_x86_64(
-    struct ubpf_vm* vm, ext_func new_helper, unsigned int idx, uint8_t* buffer, size_t size, uint32_t offset);
+    struct ubpf_vm* vm,
+    extended_external_helper_t new_helper,
+    unsigned int idx,
+    uint8_t* buffer,
+    size_t size,
+    uint32_t offset);
 
 // uhm, hello?
 struct ubpf_jit_result
@@ -135,7 +151,12 @@ ubpf_jit_update_dispatcher_null(
     struct ubpf_vm* vm, external_function_dispatcher_t new_dispatcher, uint8_t* buffer, size_t size, uint32_t offset);
 bool
 ubpf_jit_update_helper_null(
-    struct ubpf_vm* vm, ext_func new_helper, unsigned int idx, uint8_t* buffer, size_t size, uint32_t offset);
+    struct ubpf_vm* vm,
+    extended_external_helper_t new_helper,
+    unsigned int idx,
+    uint8_t* buffer,
+    size_t size,
+    uint32_t offset);
 
 char*
 ubpf_error(const char* fmt, ...);

vm/ubpf_jit.c

@@ -77,7 +77,12 @@ ubpf_jit_update_dispatcher_null(
 
 bool
 ubpf_jit_update_helper_null(
-    struct ubpf_vm* vm, ext_func new_helper, unsigned int idx, uint8_t* buffer, size_t size, uint32_t offset)
+    struct ubpf_vm* vm,
+    extended_external_helper_t new_helper,
+    unsigned int idx,
+    uint8_t* buffer,
+    size_t size,
+    uint32_t offset)
 {
     UNUSED_PARAMETER(vm);
     UNUSED_PARAMETER(new_helper);

vm/ubpf_jit_arm64.c

@@ -603,10 +603,10 @@ emit_dispatched_external_helper_call(struct jit_state* state, struct ubpf_vm* vm
     // Check whether temp_register is empty.
     emit_addsub_immediate(state, true, AS_SUBS, temp_register, temp_register, 0);
 
-    // Jump if we are ready to roll (because we are using an external dispatcher).
-    uint32_t jump_source = emit_conditionalbranch_immediate(state, COND_NE, 0);
+    // Jump to the call if we are ready to roll (because we are using an external dispatcher).
+    uint32_t external_dispatcher_jump_source = emit_conditionalbranch_immediate(state, COND_NE, 0);
 
-    // We are not ready to roll. So, load the helper function address by index.
+    // We are not ready to roll. In other words, we are going to load the helper function address by index.
     emit_movewide_immediate(state, true, R5, idx);
     emit_movewide_immediate(state, true, R6, 3);
     emit_dataprocessing_twosource(state, true, DP2_LSLV, R5, R5, R6);
@@ -616,17 +616,31 @@ emit_dispatched_external_helper_call(struct jit_state* state, struct ubpf_vm* vm
     emit_addsub_register(state, true, AS_ADD, temp_register, temp_register, R5);
     emit_loadstore_immediate(state, LS_LDRX, temp_register, temp_register, 0);
 
-    // And now we, too, are ready to roll.
+    // Add the implicit 6th parameter (the context)
+    emit_logical_register(state, true, LOG_ORR, R5, RZ, VOLATILE_CTXT);
 
-    // Both paths meet here where we ...
-    emit_jump_target(state, jump_source);
+    // And now we, too, are ready to roll. So, let's jump around the code that sets up the additional
+    // parameters for the external dispatcher. We will end up at the call site where both paths
+    // will rendezvous.
+    uint32_t no_dispatcher_jump_source = emit_unconditionalbranch_immediate(state, UBR_B, 0);
 
-    // ... set up the final two parameters.
+    // Mark the landing spot for the jump around the code that sets up a call to a helper function
+    // when no external dispatcher is present.
+    emit_jump_target(state, external_dispatcher_jump_source);
+
+    // ... set up the final two arguments for the external dispatcher.
+
+    // The index of the helper to be invoked.
     emit_movewide_immediate(state, true, R5, idx);
+
+    // The context.
     // Use a sneaky way to copy the context register into the R6 register (as the final parameter).
     emit_logical_register(state, true, LOG_ORR, R6, RZ, VOLATILE_CTXT);
 
-    // Now, all that's left is to call!
+    // Mark the landing spot for the jump around the external-dispatcher-argument-setup code.
+    emit_jump_target(state, no_dispatcher_jump_source);
+
+    // Both paths meet here -- all that's left is to call!
     emit_unconditionalbranch_register(state, BR_BLR, temp_register);
 
     /* On exit need to move result from r0 to whichever register we've mapped EBPF r0 to.  */
@@ -1459,7 +1473,12 @@ ubpf_jit_update_dispatcher_arm64(
 
 bool
 ubpf_jit_update_helper_arm64(
-    struct ubpf_vm* vm, ext_func new_helper, unsigned int idx, uint8_t* buffer, size_t size, uint32_t offset)
+    struct ubpf_vm* vm,
+    extended_external_helper_t new_helper,
+    unsigned int idx,
+    uint8_t* buffer,
+    size_t size,
+    uint32_t offset)
 {
     UNUSED_PARAMETER(vm);
     uint64_t jit_upper_bound = (uint64_t)buffer + size;

vm/ubpf_jit_x86_64.c

@@ -149,7 +149,7 @@ emit_local_call(struct ubpf_vm* vm, struct jit_state* state, uint32_t target_pc)
     emit_pop(state, map_register(BPF_REG_7));
     emit_pop(state, map_register(BPF_REG_6));
 
-    // Because the top of the host tack holds the stack usage of the currently-executing
+    // Because the top of the host stack holds the stack usage of the currently-executing
     // function, we adjust the eBPF base pointer back up by that value!
     // add r15, [rsp]
     emit1(state, 0x4c);
@@ -1170,7 +1170,12 @@ ubpf_jit_update_dispatcher_x86_64(
 
 bool
 ubpf_jit_update_helper_x86_64(
-    struct ubpf_vm* vm, ext_func new_helper, unsigned int idx, uint8_t* buffer, size_t size, uint32_t offset)
+    struct ubpf_vm* vm,
+    extended_external_helper_t new_helper,
+    unsigned int idx,
+    uint8_t* buffer,
+    size_t size,
+    uint32_t offset)
 {
     UNUSED_PARAMETER(vm);
     uint64_t jit_upper_bound = (uint64_t)buffer + size;

vm/ubpf_jit_x86_64.h

@@ -554,9 +554,7 @@ emit_dispatched_external_helper_call(struct jit_state* state, unsigned int idx)
 #endif
 
     // jmp call_label
-    emit1(state, 0xe9);
-    uint32_t skip_external_dispatcher_source = state->offset;
-    emit_4byte_offset_placeholder(state);
+    uint32_t skip_external_dispatcher_source = emit_jmp(state, 0);
 
     // External dispatcher:
 

vm/ubpf_vm.c

@@ -73,19 +73,6 @@ ubpf_set_error_print(struct ubpf_vm* vm, int (*error_printf)(FILE* stream, const
         vm->error_printf = fprintf;
 }
 
-static uint64_t
-ubpf_default_external_dispatcher(
-    uint64_t arg1,
-    uint64_t arg2,
-    uint64_t arg3,
-    uint64_t arg4,
-    uint64_t arg5,
-    unsigned int index,
-    external_function_t* external_fns)
-{
-    return external_fns[index](arg1, arg2, arg3, arg4, arg5);
-}
-
 struct ubpf_vm*
 ubpf_create(void)
 {
@@ -158,7 +145,7 @@ ubpf_register(struct ubpf_vm* vm, unsigned int idx, const char* name, external_f
         return -1;
     }
 
-    vm->ext_funcs[idx] = (ext_func)fn;
+    vm->ext_funcs[idx] = (extended_external_helper_t)fn;
     vm->ext_func_names[idx] = name;
 
     int success = 0;
@@ -170,7 +157,12 @@ ubpf_register(struct ubpf_vm* vm, unsigned int idx, const char* name, external_f
 
         // Now, update!
         if (!vm->jit_update_helper(
-                vm, fn, idx, (uint8_t*)vm->jitted, vm->jitted_size, vm->jitted_result.external_helper_offset)) {
+                vm,
+                (extended_external_helper_t)fn,
+                idx,
+                (uint8_t*)vm->jitted,
+                vm->jitted_size,
+                vm->jitted_result.external_helper_offset)) {
             // Can't immediately stop here because we have unprotected memory!
             success = -1;
         }
@@ -1229,8 +1221,8 @@ ubpf_exec_ex(
                     reg[0] =
                         vm->dispatcher(reg[1], reg[2], reg[3], reg[4], reg[5], inst.imm, external_dispatcher_cookie);
                 } else {
-                    reg[0] = ubpf_default_external_dispatcher(
-                        reg[1], reg[2], reg[3], reg[4], reg[5], inst.imm, vm->ext_funcs);
+                    reg[0] =
+                        vm->ext_funcs[inst.imm](reg[1], reg[2], reg[3], reg[4], reg[5], external_dispatcher_cookie);
                 }
                 if (inst.imm == vm->unwind_stack_extension_index && reg[0] == 0) {
                     *bpf_return_value = reg[0];