Skip to content

Commit

Permalink
Address PR feedback
Browse files Browse the repository at this point in the history 
Signed-off-by: Dave Thaler <[email protected]>
  • Loading branch information
@dthaler
dthaler committed Feb 27, 2024
1 parent 7a8a0c4 commit 0e96fca
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 36 deletions.
1 change: 1 addition & 0 deletions src/ebpf_vm_isa.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ struct ebpf_inst {
std::uint8_t src : 4; //< Source register
std::int16_t offset;
std::int32_t imm; //< Immediate constant
constexpr bool operator==(const ebpf_inst&) const = default;
};

enum {
Expand Down
2 changes: 1 addition & 1 deletion src/test/test_conformance.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ void test_conformance(std::string filename, bpf_conformance_test_result_t expect
test_path.remove_filename().append("conformance_check" + extension.string()).string();
std::map<std::filesystem::path, std::tuple<bpf_conformance_test_result_t, std::string>> result = bpf_conformance(
test_files, plugin_path, {}, {}, {}, bpf_conformance_test_CPU_version_t::v4,
_bpf_conformance_groups::default_groups, bpf_conformance_list_instructions_t::LIST_INSTRUCTIONS_NONE, true);
bpf_conformance_groups_t::default_groups, bpf_conformance_list_instructions_t::LIST_INSTRUCTIONS_NONE, true);
for (auto file : test_files) {
auto& [file_result, reason] = result[file];
REQUIRE(file_result == expected_result);
Expand Down
75 changes: 40 additions & 35 deletions src/test/test_marshal.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,16 @@
// for the 'offset' field of an instruction. Any non-sentinel values
// in an instruction template are treated as literals.

#define MEM_OFFSET 3 // Any valid memory offset value.
#define JMP_OFFSET 5 // Any valid jump offset value.
#define DST 7 // Any destination register number.
#define SRC 9 // Any source register number.
#define IMM -1 // Any imm value.
constexpr int MEM_OFFSET = 3; // Any valid memory offset value.
constexpr int JMP_OFFSET = 5; // Any valid jump offset value.
constexpr int DST = 7; // Any destination register number.
constexpr int SRC = 9; // Any source register number.
constexpr int IMM = -1; // Any imm value.
constexpr int INVALID_REGISTER = R10_STACK_POINTER + 1; // Not a valid register.

// The following table is derived from the table in the Appendix of the
// BPF ISA specification (https://datatracker.ietf.org/doc/draft-ietf-bpf-isa/).
ebpf_inst instruction_template[] = {
static const ebpf_inst instruction_template[] = {
// opcode, dst, src, offset, imm.
{0x04, DST, 0, 0, IMM},
{0x05, 0, 0, JMP_OFFSET, 0},
Expand Down Expand Up @@ -251,9 +252,9 @@ static void compare_unmarshal_marshal(const ebpf_inst& ins1, const ebpf_inst& in

// Verify that if we marshal an instruction and then unmarshal it,
// we get the original.
static void compare_marshal_unmarshal(const Instruction& ins, bool double_cmd = false, const ebpf_platform_t* platform = &g_ebpf_platform_linux) {
program_info info{.platform = platform,
.type = platform->get_program_type("unspec", "unspec")};
static void compare_marshal_unmarshal(const Instruction& ins, bool double_cmd = false, const ebpf_platform_t& platform = g_ebpf_platform_linux) {
program_info info{.platform = &platform,
.type = platform.get_program_type("unspec", "unspec")};
InstructionSeq parsed = std::get<InstructionSeq>(unmarshal(raw_program{"", "", marshal(ins, 0), info}));
REQUIRE(parsed.size() == 1);
auto [_, single, _2] = parsed.back();
Expand All @@ -262,28 +263,28 @@ static void compare_marshal_unmarshal(const Instruction& ins, bool double_cmd =
REQUIRE(single == ins);
}

static void check_marshal_unmarshal_fail(const Instruction& ins, std::string expected_error_message, const ebpf_platform_t* platform = &g_ebpf_platform_linux) {
program_info info{.platform = platform,
.type = platform->get_program_type("unspec", "unspec")};
static void check_marshal_unmarshal_fail(const Instruction& ins, std::string expected_error_message, const ebpf_platform_t& platform = g_ebpf_platform_linux) {
program_info info{.platform = &platform,
.type = platform.get_program_type("unspec", "unspec")};
std::string error_message = std::get<std::string>(unmarshal(raw_program{"", "", marshal(ins, 0), info}));
REQUIRE(error_message == expected_error_message);
}

static void check_unmarshal_fail(ebpf_inst inst, std::string expected_error_message, const ebpf_platform_t* platform = &g_ebpf_platform_linux) {
program_info info{.platform = platform,
.type = platform->get_program_type("unspec", "unspec")};
static void check_unmarshal_fail(ebpf_inst inst, std::string expected_error_message, const ebpf_platform_t& platform = g_ebpf_platform_linux) {
program_info info{.platform = &platform,
.type = platform.get_program_type("unspec", "unspec")};
std::vector<ebpf_inst> insns = {inst};
auto result = unmarshal(raw_program{"", "", insns, info});
REQUIRE(std::holds_alternative<std::string>(result));
std::string error_message = std::get<std::string>(result);
REQUIRE(error_message == expected_error_message);
}

static void check_unmarshal_fail_goto(ebpf_inst inst, std::string expected_error_message) {
static void check_unmarshal_fail_goto(ebpf_inst inst, const std::string& expected_error_message) {
program_info info{.platform = &g_ebpf_platform_linux,
.type = g_ebpf_platform_linux.get_program_type("unspec", "unspec")};
const ebpf_inst exit{.opcode = INST_OP_EXIT};
std::vector<ebpf_inst> insns = {inst, exit, exit};
std::vector<ebpf_inst> insns{inst, exit, exit};
auto result = unmarshal(raw_program{"", "", insns, info});
REQUIRE(std::holds_alternative<std::string>(result));
std::string error_message = std::get<std::string>(result);
Expand Down Expand Up @@ -517,7 +518,7 @@ TEST_CASE("unmarshal extension opcodes", "[disasm][marshal]") {
}

// Check that unmarshaling an invalid instruction fails with a given message.
static void check_unmarshal_instruction_fail(ebpf_inst& inst, const char* message) {
static void check_unmarshal_instruction_fail(ebpf_inst& inst, const std::string& message) {
if (inst.offset == JMP_OFFSET) {
inst.offset = 1;
check_unmarshal_fail_goto(inst, message);
Expand All @@ -528,16 +529,16 @@ static void check_unmarshal_instruction_fail(ebpf_inst& inst, const char* messag
}

// Check that various 'dst' variations between two valid instruction templates fail.
static void check_instruction_dst_variations(const ebpf_inst& previous_template, const ebpf_inst* next_template) {
static void check_instruction_dst_variations(const ebpf_inst& previous_template, std::optional<const ebpf_inst> next_template) {
ebpf_inst inst = previous_template;
if (inst.dst == DST) {
inst.dst = R10_STACK_POINTER + 1; // Not a valid register.
inst.dst = INVALID_REGISTER;
check_unmarshal_instruction_fail(inst, "0: Bad register\n");
} else {
// This instruction doesn't put a register number in the 'dst' field.
// Just try the next value unless that's what the next template has.
inst.dst++;
if (!next_template || memcmp(&inst, next_template, sizeof(inst)) != 0) {
if (inst != next_template) {
std::ostringstream oss;
if (inst.dst == 1)
oss << "0: nonzero dst for register op 0x" << std::hex << (int)inst.opcode << std::endl;
Expand All @@ -549,16 +550,16 @@ static void check_instruction_dst_variations(const ebpf_inst& previous_template,
}

// Check that various 'src' variations between two valid instruction templates fail.
static void check_instruction_src_variations(const ebpf_inst& previous_template, const ebpf_inst* next_template) {
static void check_instruction_src_variations(const ebpf_inst& previous_template, std::optional<const ebpf_inst> next_template) {
ebpf_inst inst = previous_template;
if (inst.src == SRC) {
inst.src = R10_STACK_POINTER + 1; // Not a valid register.
inst.src = INVALID_REGISTER;
check_unmarshal_instruction_fail(inst, "0: Bad register\n");
} else {
// This instruction doesn't put a register number in the 'src' field.
// Just try the next value unless that's what the next template has.
inst.src++;
if (!next_template || memcmp(&inst, next_template, sizeof(inst)) != 0) {
if (inst != next_template) {
std::ostringstream oss;
oss << "0: Bad instruction op 0x" << std::hex << (int)inst.opcode << std::endl;
check_unmarshal_instruction_fail(inst, oss.str().c_str());
Expand All @@ -567,7 +568,7 @@ static void check_instruction_src_variations(const ebpf_inst& previous_template,
}

// Check that various 'offset' variations between two valid instruction templates fail.
static void check_instruction_offset_variations(const ebpf_inst& previous_template, const ebpf_inst* next_template) {
static void check_instruction_offset_variations(const ebpf_inst& previous_template, std::optional<const ebpf_inst> next_template) {
ebpf_inst inst = previous_template;
if (inst.offset == JMP_OFFSET) {
inst.offset = 0; // Not a valid jump offset.
Expand All @@ -576,7 +577,7 @@ static void check_instruction_offset_variations(const ebpf_inst& previous_templa
// This instruction limits what can appear in the 'offset' field.
// Just try the next value unless that's what the next template has.
inst.offset++;
if (!next_template || memcmp(&inst, next_template, sizeof(inst)) != 0) {
if (inst != next_template) {
std::ostringstream oss;
if (inst.offset == 1 && (!next_template || next_template->opcode != inst.opcode || next_template->offset == 0))
oss << "0: nonzero offset for op 0x" << std::hex << (int)inst.opcode << std::endl;
Expand All @@ -588,7 +589,7 @@ static void check_instruction_offset_variations(const ebpf_inst& previous_templa
}

// Check that various 'imm' variations between two valid instruction templates fail.
static void check_instruction_imm_variations(const ebpf_inst& previous_template, const ebpf_inst* next_template) {
static void check_instruction_imm_variations(const ebpf_inst& previous_template, std::optional<const ebpf_inst> next_template) {
ebpf_inst inst = previous_template;
if (inst.imm == JMP_OFFSET) {
inst.imm = 0; // Not a valid jump offset.
Expand All @@ -597,7 +598,7 @@ static void check_instruction_imm_variations(const ebpf_inst& previous_template,
// This instruction limits what can appear in the 'imm' field.
// Just try the next value unless that's what the next template has.
inst.imm++;
if (!next_template || memcmp(&inst, next_template, sizeof(inst)) != 0) {
if (inst != next_template) {
std::ostringstream oss;
if (inst.imm == 1)
oss << "0: nonzero imm for op 0x" << std::hex << (int)inst.opcode << std::endl;
Expand All @@ -619,7 +620,7 @@ static void check_instruction_imm_variations(const ebpf_inst& previous_template,
}

// Check that various variations between two valid instruction templates fail.
static void check_instruction_variations(const ebpf_inst* previous_template, const ebpf_inst* next_template) {
static void check_instruction_variations(std::optional<const ebpf_inst> previous_template, std::optional<const ebpf_inst> next_template) {
if (previous_template) {
check_instruction_dst_variations(*previous_template, next_template);
check_instruction_src_variations(*previous_template, next_template);
Expand All @@ -628,8 +629,8 @@ static void check_instruction_variations(const ebpf_inst* previous_template, con
}

// Check any invalid opcodes in between the previous and next templates.
int previous_opcode = (previous_template) ? previous_template->opcode : -1;
int next_opcode = (next_template) ? next_template->opcode : 0x100;
int previous_opcode = previous_template ? previous_template->opcode : -1;
int next_opcode = next_template ? next_template->opcode : 0x100;
for (int opcode = previous_opcode + 1; opcode < next_opcode; opcode++) {
ebpf_inst inst{.opcode = (uint8_t)opcode};
std::ostringstream oss;
Expand All @@ -640,11 +641,15 @@ static void check_instruction_variations(const ebpf_inst* previous_template, con

TEST_CASE("fail unmarshal bad instructions", "[disasm][marshal]") {
size_t template_count = std::size(instruction_template);
for (int index = 0; index < template_count; index++)
check_instruction_variations((index > 0) ? &instruction_template[index - 1] : nullptr, &instruction_template[index]);

// Check any variations before the first template.
check_instruction_variations({}, instruction_template[0]);

for (int index = 1; index < template_count; index++)
check_instruction_variations(instruction_template[index - 1], instruction_template[index]);

// Check any remaining variations after the last template.
check_instruction_variations(&instruction_template[template_count - 1], nullptr);
check_instruction_variations(instruction_template[template_count - 1], {});
}

TEST_CASE("check unmarshal legacy opcodes", "[disasm][marshal]") {
Expand All @@ -660,7 +665,7 @@ TEST_CASE("check unmarshal legacy opcodes", "[disasm][marshal]") {
for (uint8_t opcode : supported_legacy_opcodes) {
std::ostringstream oss;
oss << "0: Bad instruction op 0x" << std::hex << (int)opcode << std::endl;
check_unmarshal_fail(ebpf_inst{.opcode = opcode}, oss.str().c_str(), &platform);
check_unmarshal_fail(ebpf_inst{.opcode = opcode}, oss.str().c_str(), platform);
}
}

Expand Down

0 comments on commit 0e96fca

Please sign in to comment.