On Tue, 27 Jan 2026 11:49:36 +0000 Marat Khalili <[email protected]> wrote:
> Add simple unit-style tests for rte_bpf_load, and fix some minor > discovered bugs. > > v4: > * Add Fixes: and Cc: [email protected] tags to all patches except 3. Try > to make sure [email protected] does not receive patch 3. > * Ensure no (possibly hallucinated) trailing whitespace after Acked-by. > * Improve subject line for the "conditional jump first" patch. > * Update REGISTER_FAST_TEST() calls in app/test/test_bpf.c to use new > constants (NOHUGE_OK, ASAN_OK) instead of old boolean values. > > v3: > * Fixed unfortunate typos in the last patch commit message. > * Marked all ARM opcodes with UINT32_C for clarity as suggested by > Morten Brørup, in a separate commit. > > v2: > * Moved new tests back into test_bpf.c > * Changed library macros RTE_SHIFT_VALxx to handle variable first > arguments instead of introducing new macros. > * Added another test and fix, for conditional jump as first instruction. > > Marat Khalili (6): > eal: variable first arguments of RTE_SHIFT_VALxx > bpf: fix signed shift overflows in ARM JIT > bpf: mark ARM opcodes with UINT32_C > bpf: disallow empty program > bpf: make add/subtract one program validate > bpf: fix validation when conditional jump is first instruction > > app/test/test_bpf.c | 254 +++++++++++++++++++++++++++++++++++ > lib/bpf/bpf_jit_arm64.c | 184 ++++++++++++------------- > lib/bpf/bpf_load.c | 2 +- > lib/bpf/bpf_validate.c | 48 ++++--- > lib/eal/include/rte_bitops.h | 4 +- > 5 files changed, 375 insertions(+), 117 deletions(-) > This is what an automated review looks like when everything is good: # DPDK BPF Patch Series Review (6 patches, v4) **Author:** Marat Khalili **Series:** BPF fixes addressing sanitizer-detected issues and validation bugs ## Summary | Severity | Count | |----------|-------| | Error | 0 | | Warning | 0 | | Info | 2 | **Overall Assessment:** This is a well-crafted patch series. All patches follow DPDK guidelines correctly with proper commit message formatting, appropriate Fixes/Cc:stable tags, and good test coverage. --- ## Patch 1/6: eal: variable first arguments of RTE_SHIFT_VALxx **Status:** ✓ OK Good fix to allow variable arguments in `RTE_SHIFT_VAL32` and `RTE_SHIFT_VAL64` macros by replacing `UINT32_C`/`UINT64_C` with casts. The commit message explains the rationale clearly, including the technical note about `uint_least64_t` vs `uint64_t`. Proper tags: `Fixes:`, `Cc: [email protected]`, `Signed-off-by`, multiple `Acked-by`. --- ## Patch 2/6: bpf: fix signed shift overflows in ARM JIT **Status:** ✓ OK Addresses undefined behavior when left-shifting into the sign bit (e.g., `(!!is64) << 31`). Correctly replaces problematic patterns with `RTE_SHIFT_VAL32()` and `RTE_BIT32()` macros. The commit message includes the actual sanitizer diagnostic, which is excellent for documenting the issue. Proper tags present. Depends on patch 1/6 for `RTE_SHIFT_VAL32` to accept variable arguments. --- ## Patch 3/6: bpf: mark ARM opcodes with UINT32_C **Status:** ✓ OK Stylistic improvement to explicitly mark ARM opcode constants with `UINT32_C()` for clarity about signedness and width. **Note:** No `Fixes:` or `Cc: [email protected]` tags, which is correct since this is a cleanup/improvement rather than a bug fix. --- ## Patch 4/6: bpf: disallow empty program **Status:** ✓ OK Excellent defensive fix that: 1. Rejects programs with zero instructions 2. Fixes out-of-bounds read when no EXIT instruction present 3. Adds comprehensive test cases for edge cases Test coverage includes: - Empty program (no instructions) - EXIT-only program (undefined return value) - No EXIT instruction - Minimal valid program Proper `Fixes:` and `Cc: [email protected]` tags. --- ## Patch 5/6: bpf: make add/subtract one program validate **Status:** ✓ OK Fixes signed integer overflow undefined behavior in `eval_add()` and `eval_sub()` by casting to `uint64_t` before arithmetic. The fix is minimal and correct: ```c // Before (UB when overflow occurs): rv.s.min = (rd->s.min + rs->s.min) & msk; // After (well-defined wraparound): rv.s.min = ((uint64_t)rd->s.min + (uint64_t)rs->s.min) & msk; ``` Includes test cases that trigger the issue. Proper tags present. --- ## Patch 6/6: bpf: fix validation when conditional jump is first instruction **Status:** ✓ OK Fixes a security-relevant validation bug where a conditional jump at instruction 0 would skip validation of the false branch. The root cause (using index 0 as sentinel) is well-explained. The fix changes from tracking previous instruction index to tracking previous instruction pointer, which elegantly solves the problem since the first instruction will have `prev_node = NULL`. Test cases demonstrate both the bug (jump-over-invalid at position 0) and a control case (same pattern at position 1 to confirm it was always invalid). Proper tags present. --- ## Series-Level Observations **Info (minor observations):** 1. **Patch ordering is correct** - Patch 1 must come before Patch 2 since patch 2 uses `RTE_SHIFT_VAL32()` with variable arguments. Good dependency handling. 2. **Test infrastructure** - The `bpf_load_test()` helper function introduced in patch 4 is reused effectively in patches 5 and 6. Consider whether this helper could be useful in the existing BPF test infrastructure beyond this series. **Code Quality:** - All patches compile independently (can be verified) - Tests are added alongside fixes - No forbidden tokens or style violations detected - Proper use of `RTE_` prefixed macros **Tags Verification:** - All bug fixes have `Fixes:` with 12-char SHA and exact subject - All bug fixes have `Cc: [email protected]` - All patches have `Signed-off-by:` - Tag order is correct throughout --- ## Verdict **Ready to merge.** No blocking issues found. The series addresses real bugs found by sanitizers and adds valuable test coverage for edge cases in BPF validation.
