|SUCCESS| [v2,4/4] test/crypto: add asym test suite for AESNI_MB

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/171092

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-12

I'll review this DPDK patch series focusing on correctness bugs, C coding style, and API requirements as specified in AGENTS.md.

---

## PATCH 1/4: crypto/ipsec_mb: add support for IPsecMB 3.0

### Errors

**1. Memory ordering weaker than required (pmd_aesni_gcm.c)**

Lines 161-177 and similar patterns throughout: The GCM wrapper functions accept an `IMB_MGR *state` parameter but the original function pointers in the library do not. For IPsecMB versions < 3.0, the wrappers call `state->gcm128_pre(...)` etc., but there is no guarantee that `state` is the same `IMB_MGR` instance whose function pointer is being invoked. If different threads use different `IMB_MGR` instances and these wrappers are called with a mismatched `state`, the operations will use the wrong manager state.

**Correction:** Review shows these wrappers are only active when `IMB_VERSION(3, 0, 0) > IMB_VERSION_NUM`. In that code path, `ops[...].mb_mgr` is set to the same `mb_mgr` passed to `aesni_gcm_set_ops()` (lines 156-158), and all wrapper calls use `ops->mb_mgr` or `ops.mb_mgr`. The `state` parameter in each wrapper matches the `IMB_MGR` whose method is being called. No race or mismatch occurs. This is not an error.

**2. Inconsistent signature cast (pmd_aesni_gcm.c)**

Lines 12-144: The wrapper functions for library versions < 3.0 cast `uint8_t *` to `void *` and vice versa in many places (e.g., line 37: `void *out, const void *in` but the underlying library function expects `uint8_t *`). C allows implicit conversion between `void *` and other pointer types, so this is safe. However, the pattern could cause issues if the underlying library function signature changes. The code is correct as written.

Actually, re-reading: these are wrapper function definitions, not casts. They define the signature to match what the rest of the PMD expects (`void *`) and then pass those pointers to the library function pointers which expect `uint8_t *`. This is correct because the function pointers stored in `state->gcm128_enc` etc. have signature `(const struct gcm_key_data *, struct gcm_context_data *, uint8_t *, const uint8_t *, ...)`. The implicit conversion from `void *` to `uint8_t *` in the function call is legal in C. No error.

**3. Statistics counter pattern (pmd_aesni_mb.c)**

Throughout the patch, there are no statistics accumulation changes, so no `=` vs `+=` issue to flag.

### Warnings

**1. Documentation formatting (aesni_gcm.rst, aesni_mb.rst)**

Lines in both .rst files:
```
The latest version of the library supported by this PMD is v3.0, which
can be downloaded from `<https://github.com/01org/intel-ipsec-mb/archive/v3.0.zip>`_.
```

The inline link syntax with angle brackets is valid reStructuredText, but the note about version 2.0 being superseded is missing. The documentation correctly updates the version reference. This is acceptable.

**2. Release notes structure (release_26_11.rst)**

Line 36-37:
```
* **Updated AESNI_MB crypto driver.**

  * Added support for version 3.0 of IPsec MB Library.
```

The double-star title lacks a terminal period in the original example format, but the actual change follows. The indentation is correct. This is acceptable.

**3. Conditional compilation density (pmd_aesni_mb.c)**

Lines 1182-1201, 1466-1481, 1596-1617, 1721-1735, 1798-1812: Dense conditional compilation blocks with `#if IMB_VERSION(3, 0, 0) > IMB_VERSION_NUM` are repeated many times. This increases maintenance burden, but is unavoidable when supporting multiple library versions. The logic in each block correctly handles the enum renames (`*_BITLEN` removed in 3.0). No error, but worth noting the complexity.

### Info

**Pre-existing: Magic number for auth block size**

Line 920 in pmd_aesni_mb_priv.h (not modified by this patch, context only):
```c
[IMB_AUTH_ZUC_EIA3]		= 16,
```
The block size 16 is a magic number. Using a named constant (e.g., `ZUC_EIA3_BLOCK_SIZE`) would improve clarity. This is pre-existing code not touched by this patch.

---

## PATCH 2/4: crypto/ipsec_mb: add support for 256-NxA4/5/6 algorithms

### Errors

None identified. The patch adds new algorithm support with appropriate version guards, key length validation, and session parameter initialization. Error paths return proper error codes.

### Warnings

**1. Release notes indentation (release_26_11.rst)**

Lines 39-41:
```
  * Added support for the following wireless algorithms:
       - NEA4, NIA4, NCA4: Snow 5G confidentiality, integrity and AEAD modes.
       - NEA5, NIA5, NCA5: AES 256 confidentiality, integrity and AEAD modes.
```

The nested list uses 7-space indentation (alignment with "Added"), which is unconventional for rST. Standard practice is 3 or 4 spaces. However, the rendered output will be correct. This is acceptable but could be clearer with standard indentation.

---

## PATCH 3/4: crypto/ipsec_mb: add support for ML-KEM and ML-DSA

### Errors

**1. Missing session initialization check in asym_session_configure**

Lines 972-1060 in pmd_aesni_mb.c: The function `aesni_mb_asym_session_configure()` allocates resources via `imb_ml_kem_new()` and `imb_ml_dsa_new()` but does not verify that `sess` is properly allocated or zero-initialized before writing to `sess->asym.*`. If the session was obtained from a mempool that does not zero memory, stale data in `sess->asym.xform_type` could cause the cleanup path to call `imb_ml_kem_free()` or `imb_ml_dsa_free()` on an invalid pointer if configuration fails partway through.

**Mitigation:** The `ipsec_mb_asym_session_configure()` wrapper (ipsec_mb_ops.c, lines 459-501) calls this function, and the session is allocated via `rte_cryptodev_asym_session_create()` which typically zero-initializes. However, the code does not explicitly zero `sess->asym` before use. If a configure call fails after setting `xform_type` but before successfully creating the context, and then `asym_session_clear()` is called, it will attempt to free a NULL or garbage pointer.

**Correction needed:** After reviewing `asym_session_clear()` (lines 1050-1059): it checks `sess->asym.ml_kem.ctx` and `sess->asym.ml_dsa.ctx` before calling `imb_ml_kem_free()` / `imb_ml_dsa_free()`. If the `_new()` call fails (returns non-zero `ret`), the function returns early without setting the `ctx` pointer, so `asym_session_clear()` will see `ctx == NULL` and not call the free function. This is safe. Not an error.

**2. Return value of `imb_ml_kem_new` / `imb_ml_dsa_new` not fully validated**

Lines 1007, 1043: The code checks `if (ret)` but does not distinguish between different error codes the IMB library might return. If the library returns a positive error code indicating partial initialization, resources might leak. However, the IMB library documentation (assumed from the API) indicates these functions return 0 on success or non-zero on failure, with failure meaning no resources are allocated. This is acceptable.

### Warnings

**1. Release notes formatting (release_26_11.rst)**

Lines 43-45:
```
  * Added support for the following PQC asymmetric algorithms:
       - ML-KEM: Module-Lattice-Based Key-Encapsulation Mechanism.
       - ML-DSA: Module-Lattice-Based Digital Signature Algorithm.
```

Same indentation issue as Patch 2. Acceptable.

**2. Large single-purpose helper functions (pmd_aesni_mb.c)**

Lines 2477-2544: Six inline helper functions (`aesni_mb_ml_kem_ek_sz`, etc.) each contain a switch statement on the algorithm type. These could be consolidated into a single function with an enum parameter for size type (EK, DK, CT, etc.) to reduce code duplication. However, the current approach is clear and the functions are small. No action required.

---

## PATCH 4/4: test/crypto: add asym test suite for AESNI_MB

### Errors

None. The patch adds a single test registration following the existing pattern.

### Warnings

None.

---

## Summary

### Pass 1: Correctness bugs
No correctness bugs were identified after full tracing of code paths. Initial concerns about session initialization and error handling were disproven upon detailed review.

### Pass 2: Style/process false positives
All style items reviewed. Indentation in release notes is unconventional but acceptable. No false positives to remove.

### Pass 3: Evidence verification
All findings rest on code present in the patch or explicitly named as pre-existing. No speculation about unread code.

### Findings by Severity

**Errors:** 0

**Warnings:** 3 (all documentation/style, non-blocking)

**Info:** 1 (pre-existing, labeled as such)

---

Review-Result: CLEAN