Support configurable instruction limits beyond 65,536#750
Support configurable instruction limits beyond 65,536#750
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds support for configurable instruction limits beyond the previous hard limit of 65,536 instructions. The changes address a hard-coded limitation where the num_insts field was a uint16_t, preventing programs with more than 65,535 instructions from being loaded.
Changes:
- Changed
num_instsfromuint16_ttouint32_tinstruct ubpf_vmand added a configurablemax_instsfield - Updated function signatures to use
uint32_tfor program counter parameters (fetch/store instructions, stack usage calculator) - Added new API
ubpf_set_max_instructions()to configure limits at runtime - Modified JIT array allocations to be dynamically sized based on program size (2x + 64) instead of fixed
UBPF_MAX_INSTS - Added overflow protection for large program JIT compilation
- Added comprehensive tests for API boundary conditions and large program loading
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| vm/ubpf_vm.c | Updated allocations to use vm->max_insts; changed validation to check vm->max_insts; updated fetch/store/stack function signatures |
| vm/ubpf_jit.c | Added ubpf_set_max_instructions() API implementation with reallocation logic |
| vm/ubpf_int.h | Changed num_insts to uint32_t, added max_insts field; updated internal function signatures |
| vm/inc/ubpf.h | Updated stack_usage_calculator_t signature; added ubpf_set_max_instructions() API documentation |
| vm/ubpf_jit_support.c | Implemented dynamic JIT array sizing with overflow checks; added vm parameter to initialize_jit_state_result |
| vm/ubpf_jit_support.h | Added max_insts field to jit_state; updated initialize_jit_state_result signature |
| vm/ubpf_jit_x86_64.c | Updated comparisons to use state->max_insts; passed vm to initialize_jit_state_result |
| vm/ubpf_jit_arm64.c | Passed vm to initialize_jit_state_result |
| vm/test.c | Updated stack_usage_calculator signature to use uint32_t pc |
| custom_tests/srcs/ubpf_test_large_program.cc | New test validating loading of 66,000 instruction programs |
| custom_tests/srcs/ubpf_test_max_insts_api.cc | New test validating API boundary conditions and behavior |
| custom_tests/srcs/ubpf_test_*.cc | Updated stack_usage_calculator signatures in existing tests |
| custom_tests/descrs/*.md | Added test descriptions for new tests |
|
@copilot address and reply to each comment with what you fixed |
Co-authored-by: Alan-Jowett <[email protected]>
Co-authored-by: Alan-Jowett <[email protected]>
Co-authored-by: Alan-Jowett <[email protected]>
Co-authored-by: Alan-Jowett <[email protected]>
…gram test - Add overflow checks for calloc in ubpf_create, ubpf_unload_code, and ubpf_set_max_instructions - Update ubpf_test_large_program to actually JIT compile and execute large programs - Initialize r0 to 0 in test program and verify result - Set appropriate JIT buffer size for large programs - Update documentation to reflect actual test behavior Co-authored-by: Alan-Jowett <[email protected]>
79ad7b2 to
f81315b
Compare
| // Check for overflow on 32-bit systems where size_t might be 32-bit | ||
| if (vm->max_insts > 0 && vm->max_insts <= SIZE_MAX / sizeof(struct ubpf_stack_usage)) { | ||
| vm->local_func_stack_usage = calloc(vm->max_insts, sizeof(struct ubpf_stack_usage)); | ||
| } |
There was a problem hiding this comment.
Potential NULL pointer dereference after code unload. If the calloc fails or if the overflow check prevents allocation, vm->local_func_stack_usage will be NULL. If code is subsequently loaded, the validate function will attempt to access vm->local_func_stack_usage[pc] causing a crash.
The overflow check should prevent ubpf_unload_code from succeeding if max_insts is too large. Additionally, the allocation failure should be checked. Consider either:
- Failing ubpf_unload_code if allocation fails (but this changes API behavior)
- Checking for NULL in validation and reporting an error
- Deferring allocation until next ubpf_load call
Note that if max_insts is so large that it would overflow, this VM is already in an invalid state and should probably not be reused.
| state->jumps = calloc(array_size, sizeof(state->jumps[0])); | ||
| state->loads = calloc(array_size, sizeof(state->loads[0])); | ||
| state->leas = calloc(array_size, sizeof(state->leas[0])); | ||
| state->local_calls = calloc(array_size, sizeof(state->local_calls[0])); |
There was a problem hiding this comment.
Missing overflow checks for calloc on 32-bit systems. On 32-bit platforms where size_t is 32 bits, the multiplication array_size * sizeof(struct patchable_relative) could overflow size_t, causing calloc to allocate less memory than expected. This could lead to buffer overflows when the arrays are accessed later.
Add checks before each calloc to ensure array_size <= SIZE_MAX / sizeof(...). For example:
if (array_size > SIZE_MAX / sizeof(state->jumps[0])) {
*errmsg = ubpf_error("Program too large for JIT compilation");
return -1;
}Apply similar checks for jumps, loads, leas, and local_calls allocations.
| } | ||
|
|
||
| state->max_insts = array_size; | ||
| state->pc_locs = calloc(vm->num_insts + 1, sizeof(state->pc_locs[0])); |
There was a problem hiding this comment.
Missing overflow check for pc_locs calloc on 32-bit systems. On 32-bit platforms where size_t is 32 bits, the multiplication (vm->num_insts + 1) * sizeof(uint32_t) could overflow size_t if num_insts is large enough.
While the check at line 98 prevents num_insts from being UINT32_MAX, this doesn't prevent the calloc overflow. Add a check:
if (vm->num_insts + 1 > SIZE_MAX / sizeof(state->pc_locs[0])) {
*errmsg = ubpf_error("Program too large for JIT compilation");
return -1;
}
Summary
This PR implements support for configurable instruction count limits in ubpf, allowing programs with more than 65,536 instructions to be loaded and executed.
Implementation Complete ✅
Phase 1: Type Changes (ABI-Breaking) ✅
num_instsfromuint16_ttouint32_tinstruct ubpf_vmmax_instsfield tostruct ubpf_vmfor runtime configurationuint16_tfor instruction counts/PC touint32_tstack_usage_calculator_ttypedef to useuint32_tfor PC parameterPhase 2: Runtime Configuration API ✅
ubpf_set_max_instructions()function to allow runtime configurationPhase 3: Dynamic Memory Management ✅
Phase 4: Testing ✅
ubpf_test_large_program: Validates loading, JIT compilation, and execution of 66,000 instruction programsubpf_test_max_insts_api: Tests boundary conditions (65,535/65,536), API behavior, and limit enforcementPhase 5: Code Review ✅
Key Features
ubpf_set_max_instructions()Testing
Backward Compatibility
UBPF_MAX_INSTScan still be overriddenOriginal prompt
This section details on the original issue you should resolve
<issue_title>Maximum ebpf instruction count</issue_title>
<issue_description>Linux apparently has a limit of INT_MAX instructions since 5.2.
Earlier versions had a limit of 4096 instructions.
The ubpf jitter is currently capped at 64k of machine code.
The ubpf interpreter is currently capped at UINT32_MAX of bytes (i.e., UINT32_MAX / 8 instructions).
These are higher than the old Linux limit of 4096, but lower than the newer limit of INT_MAX instructions.</issue_description>
Comments on the Issue (you are @copilot in this section)
@Alan-Jowett ## Feature Specification: Configurable Instruction Count LimitsProblem Statement
The current ubpf implementation has several hardcoded limits related to instruction counts that may not align with Linux kernel BPF limits or user requirements:
UBPF_MAX_INSTS)INT_MAX)DEFAULT_JITTER_BUFFER_SIZE)num_instsfielduint16_t(max 65,535)Key Limitation: The
num_instsfield instruct ubpf_vmis auint16_t, which fundamentally caps the instruction count at 65,535 regardless of other settings.Background: Linux Kernel Limits
INT_MAX)Real-world BPF programs:
Goals
Proposed Changes
1. Change
num_inststouint32_tFile:
vm/ubpf_int.hImpact: This is an ABI-breaking change but necessary to support > 64K instructions.
2. Make
UBPF_MAX_INSTSConfigurableFile:
vm/inc/ubpf.h(already partially done)Add runtime configuration:
3. Dynamic JIT Buffer Sizing
Current code:
vm/ubpf_vm.c:125Proposed approach:
Alternative: Allow retry with larger buffer if initial allocation fails.
4. Update Data Structures Using
UBPF_MAX_INSTSSeveral arrays are sized based on
UBPF_MAX_INSTS:File:
vm/ubpf_jit_support.cProposed: Size based on actual
num_instsfrom loaded program:File:
vm/ubpf_vm.cProposed: Size based on actual program, or use sparse storage.
API Changes