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 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 <marat.khalili@huawei.com>  
**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: stable@dpdk.org` - 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: stable@dpdk.org` 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: stable@dpdk.org included
- ⚠️ **Warning:** Missing `Fixes:` tag - sanitizer errors indicate this is a regression/bug
- ❌ **Error:** Trailing whitespace on line 176: `Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com> ` (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: stable@dpdk.org` 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: stable@dpdk.org` 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: stable@dpdk.org 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: stable@dpdk.org 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: stable@dpdk.org 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: stable@dpdk.org` |
| 2/6 | ❌ | Fix trailing whitespace; add `Fixes:` tag |
| 3/6 | ⚠️ | Consider removing `Cc: stable@dpdk.org` (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 `konstantin.ananyev@huawei.com`

### Recommended Improvements (Warnings)

1. **Patches 2, 4, 5:** Add `Fixes:` tags to identify the commits that introduced the bugs
2. **Patch 3:** Reconsider `Cc: stable@dpdk.org` - 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.