Re: [PATCH v3 0/6] bpf: simple tests and fixes

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 17 Dec 2025 18:01:33 +0000
Marat Khalili <marat.khalili@huawei.com> 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 <stephen@networkplumber.org>

=== 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: stable@dpdk.org in commit message body**
   - The patch CC's `stable@dpdk.org` in email headers but should include `Cc: stable@dpdk.org` 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: stable@dpdk.org in commit body** - If this is a bug fix intended for stable releases, it should have `Cc: stable@dpdk.org` in the commit message.

3. **Trailing whitespace in Acked-by tag** (line with `Acked-by: Konstantin Ananyev`):
   ```
   Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com> 
   ```
   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: stable@dpdk.org** - 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: stable@dpdk.org** - 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: stable@dpdk.org** - 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 <konstantin.ananyev@huawei.com> 
   ```
   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: stable@dpdk.org** 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