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

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 27 Jan 2026 11:49:36 +0000
Marat Khalili <marat.khalili@huawei.com> wrote:

> Add simple unit-style tests for rte_bpf_load, and fix some minor
> discovered bugs.
> 
> v4:
> * Add Fixes: and Cc: stable@dpdk.org tags to all patches except 3. Try
>   to make sure stable@dpdk.org 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: stable@dpdk.org`, `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: stable@dpdk.org` 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: stable@dpdk.org` 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: stable@dpdk.org`  
- 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.