On Wed, 17 Dec 2025 18:01:33 +0000 Marat Khalili <[email protected]> wrote:
> Add simple unit-style tests for rte_bpf_load, and fix some minor > discovered bugs. > > 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 BPF validation w/ conditional jump first > > 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(-) > AI review of this patch series identified some minor things. They could either be addressed by Thomas during merge or you could send a new version. ## DPDK BPF Test Patchset Review (v3, 6 patches) **Submitter:** Marat Khalili <[email protected]> **Series overview:** Fixes various issues in the BPF library including sanitizer-detected undefined behaviors (signed shift overflows, integer overflows), validation bugs, and adds comprehensive test coverage. --- ### Patch 1/6: `eal: variable first arguments of RTE_SHIFT_VALxx` **Subject line:** ✅ 48 characters, correct format, lowercase **Commit message:** - ✅ Signed-off-by present - ✅ Reviewed-by and Acked-by present - ✅ Good explanation of the change and rationale - ⚠️ **Warning:** Missing `Cc: [email protected]` - this changes the behavior of public macros **Code changes:** ```c -#define RTE_SHIFT_VAL32(val, nr) (UINT32_C(val) << (nr)) +#define RTE_SHIFT_VAL32(val, nr) ((uint32_t)(val) << (nr)) ``` - ✅ Clean, minimal change - ✅ Allows variables as arguments (enables patch 2) **Verdict:** Acceptable. Consider adding `Cc: [email protected]` if this fixes user-visible limitations. --- ### Patch 2/6: `bpf: fix signed shift overflows in ARM JIT` **Subject line:** ✅ 41 characters, correct format **Commit message:** - ✅ Signed-off-by present - ✅ Acked-by present - ✅ Cc: [email protected] included - ⚠️ **Warning:** Missing `Fixes:` tag - sanitizer errors indicate this is a regression/bug - ❌ **Error:** Trailing whitespace on line 176: `Acked-by: Konstantin Ananyev <[email protected]> ` (space before `---`) **Code review:** - ✅ Comprehensive fix for UBSan-detected issues - ✅ Correct use of `RTE_BIT32()` and `RTE_SHIFT_VAL32()` macros - ℹ️ **Info:** Some shifts are no-ops (e.g., `RTE_SHIFT_VAL32(0, 30)` = 0) but kept for consistency - acceptable **Verdict:** Fix the trailing whitespace error. Should add `Fixes:` tag identifying when the UB was introduced. --- ### Patch 3/6: `bpf: mark ARM opcodes with UINT32_C` **Subject line:** ✅ 35 characters, correct format **Commit message:** - ✅ Signed-off-by present - ✅ Acked-by present - ⚠️ **Warning:** Has `Cc: [email protected]` but this is a style/clarity change, not a bug fix. Consider removing unless there's a functional issue. **Code changes:** ```c -#define A64_INVALID_OP_CODE (0xffffffff) +#define A64_INVALID_OP_CODE (UINT32_C(0xffffffff)) ``` - ✅ Consistent use of UINT32_C for ARM opcode constants - ✅ Improves code clarity about integer types **Verdict:** Good cleanup. Questionable whether `Cc: [email protected]` is appropriate for a style change. --- ### Patch 4/6: `bpf: disallow empty program` **Subject line:** ✅ 26 characters, correct format **Commit message:** - ✅ Signed-off-by present - ✅ Acked-by present - ✅ Cc: [email protected] included (appropriate for bug fixes) - ⚠️ **Warning:** Missing `Fixes:` tag - this fixes actual bugs (empty program accepted, buffer over-read) **Code review:** - ✅ `bpf_load_test()` helper function is well-documented - ✅ Good test coverage for edge cases - ✅ Boundary check fix: `nidx > bvf->prm->nb_ins` → `nidx >= bvf->prm->nb_ins` (correct) - ✅ Empty program check added to `validate()` - ✅ Loop changed from `while` to `do-while` with assertion - good defensive coding **Minor observations:** - Line 817-818: Brace style `} else` without braces on else is inconsistent but acceptable per DPDK style **Verdict:** Good patch. Add `Fixes:` tag to identify when these bugs were introduced. --- ### Patch 5/6: `bpf: make add/subtract one program validate` **Subject line:** ✅ 42 characters, correct format **Commit message:** - ✅ Signed-off-by present - ✅ Acked-by present - ✅ Cc: [email protected] included - ✅ Good documentation of the UBSan errors being fixed - ⚠️ **Warning:** Missing `Fixes:` tag - this fixes signed integer overflow UB **Code review:** ```c - rv.s.min = (rd->s.min + rs->s.min) & msk; + rv.s.min = ((uint64_t)rd->s.min + (uint64_t)rs->s.min) & msk; ``` - ✅ Minimal fix for UB by casting to unsigned before arithmetic - ✅ Tests correctly exercise the boundary conditions **Verdict:** Good minimal fix. Add `Fixes:` tag. --- ### Patch 6/6: `bpf: fix BPF validation w/ conditional jump first` **Subject line:** ✅ 48 characters, correct format **Commit message:** - ✅ Signed-off-by present - ✅ Acked-by and Tested-by present - ✅ Cc: [email protected] present (from CC header) - ✅ **Correct `Fixes:` tag:** `Fixes: 6e12ec4c4d6d ("bpf: add more checks")` - ✅ Excellent description of the root cause and fix **Code review:** ```c - uint32_t prev_node; + struct inst_node *prev_node; ``` - ✅ Clean refactoring from index to pointer for prev_node tracking - ✅ Fixes the bug where index 0 was used as termination signal - ✅ Two tests covering the exact bug scenario (first vs non-first instruction) - ✅ `get_prev_node()` function removed as it's no longer needed **Verdict:** Excellent patch. Good fix with proper testing and documentation. --- ## Summary | Patch | Status | Action Required | |-------|--------|-----------------| | 1/6 | ⚠️ | Consider adding `Cc: [email protected]` | | 2/6 | ❌ | Fix trailing whitespace; add `Fixes:` tag | | 3/6 | ⚠️ | Consider removing `Cc: [email protected]` (style change) | | 4/6 | ⚠️ | Add `Fixes:` tag | | 5/6 | ⚠️ | Add `Fixes:` tag | | 6/6 | ✅ | Ready | ### Required Fixes (Errors) 1. **Patch 2/6, line 176:** Remove trailing whitespace after `[email protected]` ### Recommended Improvements (Warnings) 1. **Patches 2, 4, 5:** Add `Fixes:` tags to identify the commits that introduced the bugs 2. **Patch 3:** Reconsider `Cc: [email protected]` - UINT32_C annotation is a style improvement, not a bug fix ### Overall Assessment This is a high-quality patch series that fixes real bugs (UB detected by sanitizers, validation logic errors) and adds valuable test coverage. The code changes are well-reasoned and minimal. The series structure properly separates enabling changes (patch 1) from fixes (patches 2-6). **Recommendation:** After fixing the trailing whitespace in patch 2 and adding missing `Fixes:` tags, this series should be ready for merge.
