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 patch review saw some really minor things. These can be fixed by new version or by maintainer during merge. It would be good to be consistent about Fixes and Cc: stable. Series-Acked-by: Stephen Hemminger <[email protected]> === Patch Review: bundle-1681-bpf-fixes.mbox (via Claude) === # DPDK Patch Review: bundle-1681-bpf-fixes.mbox ## Overview This patch series (6 patches) addresses BPF-related fixes including signed shift overflows in ARM JIT, validation improvements, and related EAL macro changes. --- ## Patch 1/6: eal: variable first arguments of RTE_SHIFT_VALxx ### Errors None identified. ### Warnings 1. **Missing Cc: [email protected] in commit message body** - The patch CC's `[email protected]` in email headers but should include `Cc: [email protected]` in the commit message body if this is intended for stable backport. ### Info 1. **Good practice**: The parenthetical note about `uint_least64_t` vs `uint64_t` is helpful context for reviewers. 2. **Change is appropriate**: Replacing `UINT32_C`/`UINT64_C` with casts allows variable arguments while maintaining the intended type behavior. --- ## Patch 2/6: bpf: fix signed shift overflows in ARM JIT ### Errors None identified. ### Warnings 1. **Missing Fixes: tag** - This patch fixes a bug (signed shift overflow causing undefined behavior). It should have a `Fixes:` tag with the 12-character SHA of the commit that introduced the problematic code. ``` Fixes: abcdef123456 ("bpf: add JIT support for arm64") ``` 2. **Missing Cc: [email protected] in commit body** - If this is a bug fix intended for stable releases, it should have `Cc: [email protected]` in the commit message. 3. **Trailing whitespace in Acked-by tag** (line with `Acked-by: Konstantin Ananyev`): ``` Acked-by: Konstantin Ananyev <[email protected]> ``` There appears to be a trailing space after the closing `>`. ### Info 1. **Good use of RTE macros**: Using `RTE_BIT32()` and `RTE_SHIFT_VAL32()` is the correct approach to avoid signed integer overflow UB. 2. **Dependency**: This patch depends on Patch 1/6 for `RTE_SHIFT_VAL32` to accept non-constant arguments. --- ## Patch 3/6: bpf: mark ARM opcodes with UINT32_C ### Errors None identified. ### Warnings 1. **Missing Cc: [email protected]** - Similar to patch 2, if this is fixing potential issues, it may warrant stable backport consideration. ### Info 1. **Good practice**: Explicitly marking opcode constants with `UINT32_C()` clarifies intent and ensures consistent unsigned 32-bit operations. 2. **Minor style observation**: The parentheses around macro definitions like `#define A64_CMP (UINT32_C(0x6b00000f))` are good defensive practice. --- ## Patch 4/6: bpf: disallow empty program ### Errors None identified. ### Warnings 1. **Missing Fixes: tag** - The commit message mentions fixing bugs: > Fix found bugs: > * a program with no instructions was accepted; > * a program with no EXIT instruction read outside the buffer. These should have corresponding `Fixes:` tags. 2. **Missing Cc: [email protected]** - Bug fixes should typically be considered for stable backport. 3. **Brace style inconsistency** (app/test/test_bpf.c, line 90-93): ```c if (expected_errno != 0) { ... } else RTE_TEST_ASSERT_NOT_EQUAL(...); ``` While this follows DPDK style for single-statement else, the asymmetry with the braced if-block could be considered less readable. ### Info 1. **Good test coverage**: Adding tests for edge cases (empty program, EXIT-only, no-EXIT, minimal valid) is excellent practice. 2. **Use of RTE_TEST_ASSERT macros**: Good use of the test framework macros. 3. **Comment style**: The function-level comments are well-written and follow proper multi-line comment format. --- ## Patch 5/6: bpf: make add/subtract one program validate ### Errors None identified. ### Warnings 1. **Missing Fixes: tag** - This fixes signed integer overflow UB in `bpf_validate.c`. Should include: ``` Fixes: <sha> ("original commit that introduced the bug") ``` 2. **Missing Cc: [email protected]** - Bug fix should be considered for stable. ### Info 1. **Correct fix approach**: Casting to `uint64_t` before arithmetic is the right way to handle potential signed overflow when the result will be masked anyway. 2. **Good test additions**: Tests for add/subtract operations validate the fix works correctly. --- ## Patch 6/6: bpf: fix BPF validation w/ conditional jump first ### Errors None identified. ### Warnings 1. **Trailing whitespace in Tested-by tag**: ``` Tested-by: Konstantin Ananyev <[email protected]> ``` There appears to be trailing space after `>`. 2. **Subject line could be clearer** - "fix BPF validation w/ conditional jump first" is somewhat abbreviated. Consider: ``` bpf: fix validation when conditional jump is first instruction ``` ### Info 1. **Good Fixes: tag present**: `Fixes: 6e12ec4c4d6d ("bpf: add more checks")` - properly formatted. 2. **Excellent commit message**: Clear explanation of the bug (zero index used as sentinel), the root cause, and the fix approach. 3. **Good test coverage**: Two tests demonstrating the issue - one with jump as first instruction, one with jump as non-first instruction to prevent regression. 4. **Code change is sound**: Switching from index-based to pointer-based tracking for `prev_node` is cleaner and avoids the zero-sentinel problem. --- ## Summary | Patch | Errors | Warnings | Info | |-------|--------|----------|------| | 1/6 (eal) | 0 | 1 | 2 | | 2/6 (bpf ARM) | 0 | 3 | 2 | | 3/6 (bpf opcodes) | 0 | 1 | 2 | | 4/6 (bpf empty) | 0 | 3 | 3 | | 5/6 (bpf add/sub) | 0 | 2 | 2 | | 6/6 (bpf jump) | 0 | 2 | 4 | ### Key Issues to Address 1. **Missing Fixes: tags** on patches 2, 3, 4, and 5 - these are bug fixes and should reference the original commits. 2. **Missing Cc: [email protected]** in commit message bodies for bug fix patches. 3. **Trailing whitespace** in Acked-by/Tested-by lines on patches 2 and 6. ### Positive Aspects - Good use of DPDK macros (`RTE_BIT32`, `RTE_SHIFT_VAL32`, `UINT32_C`) - Comprehensive test additions using proper test framework - Clear commit messages explaining the issues and fixes - Proper patch ordering (dependencies handled correctly) - License/copyright compliance (no changes needed) - Code follows DPDK style guidelines
